Class matching fails after moving an element from a quirks-mode document to a standard-mode document
Reported by
a...@scirra.com,
Jun 15 2017
|
||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.86 Safari/537.36 Steps to reproduce the problem: 1. Visit this URL: https://editor.construct.net/r38/#open=ghost-shooter-tut 2. Wait for everything to finish loading - it should open a project after a moment 3. Press F6 4. Double-click the "Web (HTML5)" option What is the expected behavior? At this point in our PWA, we make a querySelector() call for an element, but querySelector() incorrectly returns null and ends up throwing an exception that makes our PWA show a crash dialog. In the console I can verify the element definitely exists, but is not found by querySelector: https://dl.dropboxusercontent.com/u/15217362/chrome-queryselector-bug.png (please excuse the minified code) What went wrong? Chrome's querySelector call fails to find an element that matches the selector. Did this work before? N/A Does this work in other browsers? Yes Chrome version: 59.0.3071.86 Channel: stable OS Version: 10.0 Flash Version: This is an urgent issue for us because Chrome are removing support for HTML imports, and this bug occurs when we switch to our polyfill. This means we have to keep using HTML imports until this is fixed, which then means our PWA will be broken when Chrome removes HTML imports. I tried to create a minimal repro (I know how important that is), but when I make the same call with the same data, querySelector() succeeds. You can see this happening here: https://www.scirra.com/labs/bugs/querySelectorRepro/ So something about our PWA causes the call to fail, but I am at a loss as to how we can further investigate this. Please let me know if you have any suggestions. The equivalent code works correctly in Firefox. We don't currently officially support Firefox, but you can follow the same repro steps at this slightly different URL: https://editor.construct.net/r38/?skip-support-check#open=ghost-shooter-tut In this case the querySelector() call succeeds in Firefox and our PWA works normally. I tested Nightly 56.0a1. I think this proves it really is a Chrome-specific bug. Canary 61.0.3131.0 is also affected. We're really stuck here, I know it's hard to debug full pages but we really need to fix this to get off HTML imports. Any help would be much appreciated!
,
Jun 16 2017
,
Jun 16 2017
,
Jun 16 2017
This bug happens when moving an element with class attribute between a quirks-mode document and a standard-mode document. I guess editor.construct.net contains imported documents without <!DOCTYPE html> and |this.!h_| is an element in an imported document. A smaller repro: <!-- quirks mode --> <body> <dialog> <button class="nextButton"></button> <button class="nextButton"></button> </dialog> <template><div></div></template> <pre></pre> <script> function log(str) { document.querySelector('pre').textContent += str + '\n'; } log('Main document\'s mode: ' + document.compatMode); var dialog = document.querySelector('dialog'); var button = dialog.querySelector('.nextButton'); var tc = document.querySelector('template').content; log('Template document\'s mode: ' + tc.ownerDocument.compatMode); tc.ownerDocument.adoptNode(button); log('Second query result should not be null: ' + dialog.querySelector('.nextButton')); log('Content of context element: ' + dialog.innerHTML); </script> </body> This shows: Main document's mode: BackCompat Template document's mode: CSS1Compat Second query result should not be null: null Content of context element: <button class="nextButton"></button>
,
Jun 16 2017
,
Jun 16 2017
,
Jun 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7 commit 3d8dd287aeb97d6dbb658da989ac4a4c95853dc7 Author: Kent Tamura <tkent@chromium.org> Date: Fri Jun 16 09:16:18 2017 Fix class matching after moving an element from a quirks-mode document to a standard-mode document, or vice versa. We accidentally shared ShareableElementData between documents. Element::DidMoveToNewDocument() should ensure we don't share. Bug: 733682 Change-Id: I6ebb9c033559f6fa42ec92571b529d73d7c74869 Reviewed-on: https://chromium-review.googlesource.com/538514 Reviewed-by: Takayoshi Kochi <kochi@chromium.org> Commit-Queue: Kent Tamura <tkent@chromium.org> Cr-Commit-Position: refs/heads/master@{#479999} [modify] https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/LayoutTests/dom/README.md [add] https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/LayoutTests/dom/element/class_matching_after_adopt.html [modify] https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/Source/core/dom/Element.cpp
,
Jun 16 2017
Amazing work, thanks for looking in to this so quickly! I didn't realise any of our documents were in quirks mode - this must be a gotcha with moving from HTML imports to XHR document fetches. Presumably imports default to standards mode but XHR defaults to quirks mode. The workaround is just to put <!DOCTYPE html> in all our imports. After doing that, everything seems to work OK again. So this gives us a way to start using the polyfill before the patch lands in Chrome. Great stuff!
,
Jun 16 2017
@tkent - is this a good candidate for web-platform-tests?
,
Jun 18 2017
> @tkent - is this a good candidate for web-platform-tests? I'm not sure. This bug strongly depends on a tricky optimization in WebKit/Blink, and I think it's impossible to write this test for one who only knows the DOM specification, and not WebKit/Blink implementation.
,
Jun 18 2017
Filed a WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=173528
,
Jun 19 2017
#10 - I thought web platform tests accept pretty much any test and since this is easily testable, it could prevent anything like this from happening in the future anywhere.
,
Jun 20 2017
We welcome your contribution ;)
,
Jun 20 2017
I will use most of the code from #4. @tkent - do you want attribution?
,
Jun 20 2017
> @tkent - do you want attribution? No. Feel free to use it without the attribution.
,
Jun 25 2017
,
Jun 26 2017
Thanks for your contribution!
,
Jun 28 2017
It was accepted! :O Does that mean you can delete https://crrev.com/3d8dd287aeb97d6dbb658da989ac4a4c95853dc7/third_party/WebKit/LayoutTests/dom/element/class_matching_after_adopt.html or does Chromium not run quirks mode web platform tests at the moment?
,
Jun 28 2017
We import quirks-mode/. So we can delete the test after the next WPT import.
,
Jun 23 2018
Can you explain how this new behavior matches what specs say? Quirks mode is a property of documents, not of elements. So if an element is adopted to a document with a different mode, I would expect it to use the rules of that document. Either revert this or open a spec issue.
,
Jun 24 2018
zcorpan@, > Quirks mode is a property of documents, not of elements. So if an element is adopted to a document with a different mode, I would expect it to use the rules of that document. Right. The new behavior matches to it. Do you see any behavior not matching to it? |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by woxxom@gmail.com
, Jun 15 2017FWIW if you set a conditional breakpoint on the failing querySelector, click the dialog in Elements panel inspector, then run $0.querySelector('.nextButton') it successfully finds the element. Interestingly, this.!h_ != $0, but all properties are equal, including this.!h_.querySelector == $0.querySelector