Bug 77853 – <style scoped>: Allow <style scoped> as a direct child of a ShadowRoot

• Component: CSS
• Status: RESOLVED FIXED

[Roland Steiner]

Currently, registration of scoped style elements expects an Element parent. For the purposes of shadow DOM, it must also work if it is a child of a ShadowRoot node.

[Roland Steiner]

Created attachment 125771 [details]
Patch

patch - contains same forwarding as registration code in bug 67790. This was moved more or less verbatim from Element into ContainerNode. See discussion there regarding cleanup (moving into helper class or some such).

[WebKit Review Bot]

Attachment 125771 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1

Source/WebCore/dom/ContainerNode.cpp:1092:  The return type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Roland Steiner]

Note: the function the style bot complains about is virtual, so I can't change the signature (or at least that should be a separate patch). Will try to fix Win build failure in a follow-up patch, provided the general direction of the patch is OK.

[Dimitri Glazkov (Google)]

Comment on attachment 125771 [details]
Patch

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

> Source/WebCore/html/HTMLStyleElement.cpp:116
> +        ContainerNode* scope = parentNode();

I have an alternative idea, but not sure if it's better. Instead of adding a new type of rare data, which seems rather high-caliber, we could declare a StyleScope interface, and have both element and ShadowRoot implement it. Since the rare data is just an int, we could just make it a member variable of the ShadowRoot. WDYT?

[Roland Steiner]

(In reply to comment #4)
> I have an alternative idea, but not sure if it's better. Instead of adding a new type of rare data, which seems rather high-caliber, we could declare a StyleScope interface, and have both element and ShadowRoot implement it. Since the rare data is just an int, we could just make it a member variable of the ShadowRoot. WDYT?

Hm, but how to get from [HTMLStyleElement]->parentNode() to the StyleScope interface? It seems I'd either need (a) dynamic_cast or (b) have to branch it like "if (parentNode->isElementNode()) { ... } else if (parentNode->isShadowRoot()) { ... }". In the latter case I could just call the respective implementation directly, without going through an interface (?).

Alternatively, (c) I could declare the interface functions virtual on ContainerNode (thus making them directly accessible from parentNode()) and implement them in Element and ShadowRoot. Variant (d) of that: StyleScope could become a super-class of ContainerNode to encapsulate the interface, but implementation would still have to happen in Element & ShadowRoot.

Using ContainerNodeRareData just seemed simpler to me... ;)

[Antti Koivisto]

Comment on attachment 125771 [details]
Patch

This seems wrong. Why isn't ShadowRoot an Element if it is supposed to act like one?

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> (From update of attachment 125771 [details])
> This seems wrong. Why isn't ShadowRoot an Element if it is supposed to act like one?

Other than this behavior, there's really nothing about ShadowRoot that makes it look like an Element. It doesn't receive events, has no attributes, it's never rendered (so no styles or dimension/position information). Also, it can't be cloned into another ShadowRoot or appended to a document tree. (more here: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html).

If we were to make it an Element, we'd have to add a larger set of exceptions in handling the specific case of the ShadowRoot, compared to just this one.

[Roland Steiner]

Created attachment 126253 [details]
Patch

patch, should fix Win bot. Still ContainerNodeRareData version: it is the most straightforward, and I wanted to avoid making hasScopedHTMLStyleChild virtual, as it is used rather heavily in <style scoped> CSS code. However, if you still prefer any of the versions (a) to (d) of comment #5 - or '(e) something completely different' - I'm happy to change the implementation.

[Dimitri Glazkov (Google)]

Comment on attachment 126253 [details]
Patch

The patch doesn't seem to apply. Also -- can we not just reuse NodeRareData? Do we have to introduce the ContainerNodeRareData?

[Roland Steiner]

Created attachment 126451 [details]
Patch

Certainly can do! Updated patch, moved registration to Node / NodeRareData as suggested.

[Roland Steiner]

Created attachment 126456 [details]
Patch

Same patch as above, missed a small bit of cleanup.

[Dimitri Glazkov (Google)]

Comment on attachment 126456 [details]
Patch

This looks great, but can you please land it by hand? I could be superstitious and old-fashioned, but I'd watch the tree after landing this patch :)

[Roland Steiner]

Will do, once the tree goes back to a sane state.

[Roland Steiner]

Committed r107793: <http://trac.webkit.org/changeset/107793>