Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): Fix next() and its test #1732

Closed
wants to merge 3 commits into from
Closed

fix(jqLite): Fix next() and its test #1732

wants to merge 3 commits into from

Conversation

Keyamoon
Copy link
Contributor

next() must return the next sibling element and it should ignore text nodes between elements. In the sample element that was prepared for testing, there was no text or space between the two elements (<b>b</b> and <i>i</i>) to check if next() was ignoring text nodes properly.

To get the next sibling element, nextElementSibling must be used instead of nextSibling because nextSibling doesn't ignore text nodes.

next() is supposed to return the next sibling element and it should ignore text nodes. To achieve this, nextElementSibling() should be used instead of nextSibling().
@petebacondarwin
Copy link
Contributor

Much better! Did you say you signed the CLA?

@Keyamoon
Copy link
Contributor Author

@petebacondarwin Yes, I signed it.

@petebacondarwin
Copy link
Contributor

@Keyamoon Great!

Unfortunately I.E. 8 doesn't like your unit test! http://ci.angularjs.org/job/angular.js-pete/21/testReport/
I have a similar problem with the test in a similar change to jqLite.children() - #1725 (http://ci.angularjs.org/job/angular.js-pete/19/testReport/)

If you can fix one you can probably fix both! Good luck.

@Keyamoon
Copy link
Contributor Author

@petebacondarwin My implementation of next() uses nextElementSibling which is not supported in IE8. I didn't know there is a requirement for it to work in IE8. I'll add another commit to address that.

nextElementSibling is not supported in IE8 but nextSibling is. To make next() work in IE8, we can check the nodeType to see whether it's a DOM element or not. If it's not an element, we will move to the next sibling node using nextSibling.
@petebacondarwin
Copy link
Contributor

Great this passes the CI: http://ci.angularjs.org/job/angular.js-pete/22/console

@petebacondarwin
Copy link
Contributor

Added it to the Triaged Milestone and will ping core devs to get it merged.

@Keyamoon
Copy link
Contributor Author

@petebacondarwin Thank you 👍

@IgorMinar
Copy link
Contributor

LGTM. I made a few small changes and squashed the commits into a single commit.
76a6047

waiting for the CI server to verify the changes on all browsers

@IgorMinar
Copy link
Contributor

merged!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants