Bug 85863 – [Shadow] Style elements in Shadow DOM should not be exposed by document.styleSheets attribute

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[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 document's CSSOM extensions" … "assert_equals: style elements in shadow DOM must not be exposed via the document.styleSheets attribute expected 0 but got 1"

[Takashi Sakamoto]

Created attachment 143440 [details]
Patch

[Shinya Kawanaka]

Comment on attachment 143440 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143440&action=review

Basically looks OK for me with a few nit comments.

> LayoutTests/fast/dom/shadow/document-stylesheets-in-shadow.html:5
> +<script>

Why not use LayoutTests/fast/js/resources/js-test-pre.js?

> LayoutTests/fast/dom/shadow/document-stylesheets-in-shadow.html:11
> +  if (actual == expected)

why 2spaces indent here? log has 4 spaces indent though.

[Andreas Kling]

Comment on attachment 143440 [details]
Patch

While I'm no shadow DOM expert, I feel that this patch needs more explanation of what's going on.

Why are we skipping over StyleElement::insertedIntoDocument/removedFromDocument? That will not only prevent document.styleSheets from exposing the sheet, but also prevent StyleElement from parsing the sheet, and StyleResolver from applying any of its style (since it gets its sheet data from there.)

Maybe <style> elements are supposed to just hang dead inside a shadow subtree?

[Hajime Morrita]

(In reply to comment #3)
> (From update of attachment 143440 [details])
> While I'm no shadow DOM expert, I feel that this patch needs more explanation of what's going on.
> 
> Why are we skipping over StyleElement::insertedIntoDocument/removedFromDocument? That will not only prevent document.styleSheets from exposing the sheet, but also prevent StyleElement from parsing the sheet, and StyleResolver from applying any of its style (since it gets its sheet data from there.)
> 

> Maybe <style> elements are supposed to just hang dead inside a shadow subtree?
It's going to be scoped by Shadow DOM, that is, the style is applied 
only for shadow subtrees.
dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles

I agree Kling's point though.
This change could be done as a part of style scoping,
instead of just doing a band-aid fix.

[Takashi Sakamoto]

Created attachment 144338 [details]
Patch

[Takashi Sakamoto]

Created attachment 144340 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 143440 [details] [details])
> > While I'm no shadow DOM expert, I feel that this patch needs more explanation of what's going on.
> > 
> > Why are we skipping over StyleElement::insertedIntoDocument/removedFromDocument? That will not only prevent document.styleSheets from exposing the sheet, but also prevent StyleElement from parsing the sheet, and StyleResolver from applying any of its style (since it gets its sheet data from there.)
> > 
> 
> > Maybe <style> elements are supposed to just hang dead inside a shadow subtree?
> It's going to be scoped by Shadow DOM, that is, the style is applied 
> only for shadow subtrees.
> dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles
> 
> I agree Kling's point though.
> This change could be done as a part of style scoping,
> instead of just doing a band-aid fix.

I agree. My old patch made any styles in shadow tree be disabled. For example,

  <div id='mydiv'></div>
...
<script>
  var div = document.getElementById('mydiv');
  var sr = new WebKitShadowRoot(div);
  sr.innerHTML = '<style>div { background-color: red; }</style><div>RED</div>';
  ...
</script>

The "RED"'s background color is not red.... So I think, I have to modify styleSheets method defined in class Document.
I created a new method allStyleSheets which does the same things the old styleSheets does, and modified styleSheets to return just style sheets that are not in shadow trees.

Best regards,
Takashi Sakamoto

[Hajime Morrita]

Comment on attachment 144340 [details]
Patch

As I mentioned, I think this should be attacked as
a part of style handling of shadow dom,instead of fixing this locally.
http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles

Maybe starting point is pulling m_stylesheets from Document up to TreeScope.

[Takashi Sakamoto]

Created attachment 144766 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #8)
> (From update of attachment 144340 [details])
> As I mentioned, I think this should be attacked as
> a part of style handling of shadow dom,instead of fixing this locally.
> http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#styles
> 
> Maybe starting point is pulling m_stylesheets from Document up to TreeScope.

I looked at Document and I felt that it was very difficult to pull m_stylesheets because Document also has m_styleResolver and the style resolver seems to tightly fit Document's stylesheets...

So I modified StyleResolver to enforce lower-boundary encapsulation for CSS rules.
I also fixed applyAuthorStyles bug, i.e. if applyAuthorStyles is true, all CSS rules declared in ancestors of the shadow DOM subtree will be applied... not only enclosed shadow DOM subtree.

I'm not sure whether I'm taking a correct way to fix these bugs. I would like to ask your advice.

Best regards,
Takashi Sakamoto

[Anne van Kesteren]

This bug has outlived its usefulness.