Reinstate active flag for iterators #359
Conversation
|
@dominiccooney @foolip what do you think? |
dom.bs
Outdated
| @@ -8785,6 +8785,10 @@ and {{Range/getBoundingClientRect()}} methods are defined in other specification | |||
| to filter and traverse <a>node</a> | |||
| <a>trees</a>. | |||
|
|
|||
| Each {{NodeIterator}} and {{TreeWalker}} object has an associated <dfn export | |||
| title=concept-traversal-active for=traversal>active flag</dfn> to avoid | |||
There was a problem hiding this comment.
s/title/id/ to preserve the old value.
|
I guess there already exists some tests for this, or how do we know that only Gecko has the flag now? |
|
Didn't have to look too far, https://www.w3.org/Bugs/Public/show_bug.cgi?id=25412 has a Live DOM case used to determine current behavior. I don't have much of an opinion on what behavior makes sense. Is there a recursion check for other cases where a script can call into C++ which synchronously invokes a callback? For dispatchEvent it looks like the limit varies quite a bit: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4628 Looks like an explicit test for the Gecko behavior should be easier than a test for its absence, does a test already exist in the Gecko tree that you could upstream to web-platform-tests? |
Well, there's the document.write recursion limitation various browsers do, though it allows some number of levels of nesting.... |
|
I'm sure we had a wpt test at some point, but I don't see it. It would be easy to write a basic one. |
annevk
added
the
needs tests
This reverts ccf51e8, and fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25412. It brings the spec back in line with Firefox instead of other browsers. Boris argues convincingly that the behavior without the active flag is much harder to spec and test properly, and nobody has a use-case for allowing recursion here, so it's easier all around to converge on Firefox behavior and just ban recursion. He doesn't want Firefox to allow recursion unless someone writes good tests for it, which nobody wants to. Other UAs were asked for comment on the bug and didn't support or oppose.
|
web-platform-tests/wpt#5651 tests this, so would anyone like to review? Firefox passes the tests, and all other browsers presumably fail. (I tested in Chrome and WebKit.) |
|
Tests and this patch LGTM. However, we don't really know what other browsers want to do. @RByers @cdumez @travisleithead? @ayg another way forward here is to file bugs against the other browsers. That sometimes helps reaching the right people. |
|
I'll defer to @tkent-google for this, the failing test would likely show up on his radar first. Edit: Heh, didn't notice I wasn't the one being asked :) |
|
document.write limit is whatwg/html#1954 |
|
I do not see any strong reason to allow recursion either. I am fine trying to align WebKit here if Blink guys are on board. If this lands, please file a bug against WebKit. |
|
Adding the flag looks reasonable. |
|
We have three out of four browsers in agreement here, and have working tests. Only Edge hasn't replied. Can we merge this? |
|
I went ahead and merged the tests. Anyone up for filing browser bugs? (Or have they already been filed?) I can take care of that too I suppose once it's safe to merge things again. |
https://bugs.webkit.org/show_bug.cgi?id=175312 Reviewed by Sam Weinig. LayoutTests/imported/w3c: Resync WPT tests from upstream to gain test coverage. * web-platform-tests/dom/traversal/NodeIterator-expected.txt: * web-platform-tests/dom/traversal/NodeIterator.html: * web-platform-tests/dom/traversal/TreeWalker-expected.txt: * web-platform-tests/dom/traversal/TreeWalker.html: Source/WebCore: NodeIterator / TreeWalker should no longer allow recursive filters after the following change to the DOM specification: - whatwg/dom#359 This patch aligns our behavior with the latest specification. No new tests, updated existing tests. * dom/NodeIterator.cpp: (WebCore::NodeIterator::nextNode): (WebCore::NodeIterator::previousNode): Note that we now also call m_candidateNode.clear() before returning an exception. This was a pre-existing bug that we failed to do so in the exception case but it became more obvious after this change now that we throw. This was causing traversal/moz-bug559526.html to fail otherwise (the filter was called one too many times). The test case is passing in Firefox (The filter is called 4 times and they throw each time). * dom/Traversal.cpp: (WebCore::NodeIteratorBase::NodeIteratorBase): (WebCore::NodeIteratorBase::acceptNode): * dom/Traversal.h: * dom/TreeWalker.cpp: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@220453 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=175312 Reviewed by Sam Weinig. LayoutTests/imported/w3c: Resync WPT tests from upstream to gain test coverage. * web-platform-tests/dom/traversal/NodeIterator-expected.txt: * web-platform-tests/dom/traversal/NodeIterator.html: * web-platform-tests/dom/traversal/TreeWalker-expected.txt: * web-platform-tests/dom/traversal/TreeWalker.html: Source/WebCore: NodeIterator / TreeWalker should no longer allow recursive filters after the following change to the DOM specification: - whatwg/dom#359 This patch aligns our behavior with the latest specification. No new tests, updated existing tests. * dom/NodeIterator.cpp: (WebCore::NodeIterator::nextNode): (WebCore::NodeIterator::previousNode): Note that we now also call m_candidateNode.clear() before returning an exception. This was a pre-existing bug that we failed to do so in the exception case but it became more obvious after this change now that we throw. This was causing traversal/moz-bug559526.html to fail otherwise (the filter was called one too many times). The test case is passing in Firefox (The filter is called 4 times and they throw each time). * dom/Traversal.cpp: (WebCore::NodeIteratorBase::NodeIteratorBase): (WebCore::NodeIteratorBase::acceptNode): * dom/Traversal.h: * dom/TreeWalker.cpp: git-svn-id: http://svn.webkit.org/repository/webkit/releases/WebKitGTK/webkit-2.18@220640 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=175312 Reviewed by Sam Weinig. LayoutTests/imported/w3c: Resync WPT tests from upstream to gain test coverage. * web-platform-tests/dom/traversal/NodeIterator-expected.txt: * web-platform-tests/dom/traversal/NodeIterator.html: * web-platform-tests/dom/traversal/TreeWalker-expected.txt: * web-platform-tests/dom/traversal/TreeWalker.html: Source/WebCore: NodeIterator / TreeWalker should no longer allow recursive filters after the following change to the DOM specification: - whatwg/dom#359 This patch aligns our behavior with the latest specification. No new tests, updated existing tests. * dom/NodeIterator.cpp: (WebCore::NodeIterator::nextNode): (WebCore::NodeIterator::previousNode): Note that we now also call m_candidateNode.clear() before returning an exception. This was a pre-existing bug that we failed to do so in the exception case but it became more obvious after this change now that we throw. This was causing traversal/moz-bug559526.html to fail otherwise (the filter was called one too many times). The test case is passing in Firefox (The filter is called 4 times and they throw each time). * dom/Traversal.cpp: (WebCore::NodeIteratorBase::NodeIteratorBase): (WebCore::NodeIteratorBase::acceptNode): * dom/Traversal.h: * dom/TreeWalker.cpp:
This reverts ccf51e8, and fixes
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25412. It brings the
spec back in line with Firefox instead of other browsers. Boris argues
convincingly that the behavior without the active flag is much harder to
spec and test properly, and nobody has a use-case for allowing recursion
here, so it's easier all around to converge on Firefox behavior and just
ban recursion. He doesn't want Firefox to allow recursion unless
someone writes good tests for it, which nobody wants to. Other UAs were
asked for comment on the bug and didn't support or oppose.
@bzbarsky @travisleithead @domenic @cdumez
Preview | Diff