This manifests as a test failure at <http://w3c-test.org/webapps/ShadowDOM/tests/submissions/Google/tests.html> "Upper-boundary encapsulation: nodes in a shadow DOM subtree are not accessible using the shadow host's document DOM tree accessors defined in HTML" … "assert_equals: title text in shadow DOM must not be exposed via document.title DOM accessor expected "" but got "what's up, doc?""
Created attachment 143268 [details] Patch
Comment on attachment 143268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143268&action=review > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:7 > +{ You should use LayoutTests/fast/js/resources/js-test-pre.js and js-test-post.js Then you can remove shouldBe, log, and layoutTestController.dumpAsText() etc. from this test.
Comment on attachment 143268 [details] Patch Basically looks good. r- per Shinya's comment.
Created attachment 144011 [details] Patch
Comment on attachment 144011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144011&action=review > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:22 > +log("PASS"); I don't think log("PASS") is necessary, since shouldBe() prints "PASS". You can also remove function log(...) { ... }.
Comment on attachment 144011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144011&action=review >> LayoutTests/fast/dom/shadow/title-element-in-shadow.html:22 >> +log("PASS"); > > I don't think log("PASS") is necessary, since shouldBe() prints "PASS". > You can also remove function log(...) { ... }. shouldBe should take strings. So you should call shouldBe("newDocument.title", '""');
Created attachment 144013 [details] Patch
Comment on attachment 144013 [details] Patch The test is really cryptic. It also can be more comprehensive. We can test against a document which has a title - to add newly created <title> to the shadow root - to remove it - to add subtree which contains <title> to the shadow root - to remove it - to add a subtree which has a shadow root, which has a <title> as a child (I guess this case doesn't work) - to remove it.
Created attachment 144267 [details] Patch
Comment on attachment 144267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144267&action=review > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:30 > + Could you test something like: parent = document.createElemenet... child = document.creatElement... childShadow = new ShadowRoot(child); document.head.appendChild(parent); ? In this case, insertionPoint will be !isInShadowTree(), but the title is.
Comment on attachment 144267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144267&action=review Please fix the test before you land & address morrita's comment. > LayoutTests/fast/dom/shadow/title-element-in-shadow-expected.txt:4 > +PASS docForTest.title is "" > +PASS docForTest.title is "" > +PASS docForTest.title is "" > +PASS docForTest.title is "" This output is not helpful. > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:13 > +<!-- add newly created title element to the shadow root. --> Could you at least put these descriptions in debug? > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:18 > +var shadowRoot = new WebKitShadowRoot(docForTest.documentElement); > +var title = document.createElement('title'); > +title.innerHTML = 'shadow title'; > +shadowRoot.appendChild(title); > +shouldBe('docForTest.title', '""'); Or more preferably, add some helper functions so that you can do something along the line of: shouldBe("shadowRoot.appendChild(createTitleElement('shadow title'); shadowRoot.document.title", '""'); > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:23 > +<!-- add subtree which contains title element to the shadow root. --> > +shadowRoot.innerHTML = '<HEAD><TITLE>shadow title</TITLE><HEAD>'; > +shouldBe('docForTest.title', '""'); > +shadowRoot.innerHTML = ''; Ditto. > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:29 > +<!-- add a subtree which has a shadow root, which has a title element as a child. --> > +shadowRoot.innerHTML = '<BODY><DIV id="div1"></DIV></BODY>'; > +var shadowRootForDiv = new WebKitShadowRoot(shadowRoot.getElementById('div1')); > +shadowRootForDiv.innerHTML = '<TITLE>shadow title</TITLE>'; > +shouldBe('docForTest.title', '""'); Ditto.
Created attachment 146270 [details] Patch
(In reply to comment #10) > (From update of attachment 144267 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144267&action=review > > > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:30 > > + > > Could you test something like: > parent = document.createElemenet... > child = document.creatElement... > childShadow = new ShadowRoot(child); > > document.head.appendChild(parent); ? I see. I think, my new patch has the same test as the above. > In this case, insertionPoint will be !isInShadowTree(), but the title is. I think, insertionPoint.isInShadowTree() in insertedInto is a mistake. I changed the condition to just isInShadowTree().
(In reply to comment #11) > (From update of attachment 144267 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144267&action=review > > Please fix the test before you land & address morrita's comment. > > > LayoutTests/fast/dom/shadow/title-element-in-shadow-expected.txt:4 > > +PASS docForTest.title is "" > > +PASS docForTest.title is "" > > +PASS docForTest.title is "" > > +PASS docForTest.title is "" > > This output is not helpful. > > > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:13 > > +<!-- add newly created title element to the shadow root. --> > > Could you at least put these descriptions in debug? I see. I added debug messages to show what the test does. > > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:18 > > +var shadowRoot = new WebKitShadowRoot(docForTest.documentElement); > > +var title = document.createElement('title'); > > +title.innerHTML = 'shadow title'; > > +shadowRoot.appendChild(title); > > +shouldBe('docForTest.title', '""'); > > Or more preferably, add some helper functions so that you can do something along the line of: > shouldBe("shadowRoot.appendChild(createTitleElement('shadow title'); shadowRoot.document.title", '""'); > > > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:23 > > +<!-- add subtree which contains title element to the shadow root. --> > > +shadowRoot.innerHTML = '<HEAD><TITLE>shadow title</TITLE><HEAD>'; > > +shouldBe('docForTest.title', '""'); > > +shadowRoot.innerHTML = ''; > > Ditto. > > > LayoutTests/fast/dom/shadow/title-element-in-shadow.html:29 > > +<!-- add a subtree which has a shadow root, which has a title element as a child. --> > > +shadowRoot.innerHTML = '<BODY><DIV id="div1"></DIV></BODY>'; > > +var shadowRootForDiv = new WebKitShadowRoot(shadowRoot.getElementById('div1')); > > +shadowRootForDiv.innerHTML = '<TITLE>shadow title</TITLE>'; > > +shouldBe('docForTest.title', '""'); > > Ditto. Are the debug messages enough helpful to know what this test does? I think, shouldBe shows helper function names, however the names don't provide any more information comparing with the debug messages.
I'm not sure why this is not committed yet.
Created attachment 180105 [details] Patch
Comment on attachment 180105 [details] Patch Clearing flags on attachment: 180105 Committed r138130: <http://trac.webkit.org/changeset/138130>
All reviewed patches have been landed. Closing bug.