Bug 85864 – [Shadow] TITLE elements in Shadow DOM should not affect document.title attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dominic Cooney]

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?""

[Takashi Sakamoto]

Created attachment 143268 [details]
Patch

[Shinya Kawanaka]

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.

[Hajime Morrita]

Comment on attachment 143268 [details]
Patch

Basically looks good. r- per Shinya's comment.

[Takashi Sakamoto]

Created attachment 144011 [details]
Patch

[Shinya Kawanaka]

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(...) { ... }.

[Shinya Kawanaka]

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", '""');

[Takashi Sakamoto]

Created attachment 144013 [details]
Patch

[Hajime Morrita]

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.

[Takashi Sakamoto]

Created attachment 144267 [details]
Patch

[Hajime Morrita]

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.

[Ryosuke Niwa]

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.

[Takashi Sakamoto]

Created attachment 146270 [details]
Patch

[Takashi Sakamoto]

(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().

[Takashi Sakamoto]

(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.

[Shinya Kawanaka]

I'm not sure why this is not committed yet.

[Takashi Sakamoto]

Created attachment 180105 [details]
Patch

[WebKit Review Bot]

Comment on attachment 180105 [details]
Patch

Clearing flags on attachment: 180105

Committed r138130: <http://trac.webkit.org/changeset/138130>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.