Bug 77853 - <style scoped>: Allow <style scoped> as a direct child of a ShadowRoot
Summary: <style scoped>: Allow <style scoped> as a direct child of a ShadowRoot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Roland Steiner
URL:
Keywords:
Depends on:
Blocks: 49142
  Show dependency treegraph
 
Reported: 2012-02-05 22:54 PST by Roland Steiner
Modified: 2012-02-15 01:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (33.08 KB, patch)
2012-02-06 23:11 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
Patch (34.93 KB, patch)
2012-02-09 00:46 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
Patch (24.42 KB, patch)
2012-02-09 22:06 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
Patch (23.91 KB, patch)
2012-02-09 22:24 PST, Roland Steiner
dglazkov: review+
dglazkov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2012-02-05 22:54:08 PST
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.
Comment 1 Roland Steiner 2012-02-06 23:11:02 PST
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).
Comment 2 WebKit Review Bot 2012-02-06 23:14:03 PST
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.
Comment 3 Roland Steiner 2012-02-06 23:47:23 PST
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.
Comment 4 Dimitri Glazkov (Google) 2012-02-07 09:46:56 PST
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?
Comment 5 Roland Steiner 2012-02-07 16:31:00 PST
(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... ;)
Comment 6 Antti Koivisto 2012-02-08 09:51:33 PST
Comment on attachment 125771 [details]
Patch

This seems wrong. Why isn't ShadowRoot an Element if it is supposed to act like one?
Comment 7 Dimitri Glazkov (Google) 2012-02-08 10:07:20 PST
(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.
Comment 8 Roland Steiner 2012-02-09 00:46:35 PST
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.
Comment 9 Dimitri Glazkov (Google) 2012-02-09 10:12:10 PST
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?
Comment 10 Roland Steiner 2012-02-09 22:06:44 PST
Created attachment 126451 [details]
Patch

Certainly can do! Updated patch, moved registration to Node / NodeRareData as suggested.
Comment 11 Roland Steiner 2012-02-09 22:24:37 PST
Created attachment 126456 [details]
Patch

Same patch as above, missed a small bit of cleanup.
Comment 12 Dimitri Glazkov (Google) 2012-02-10 09:22:28 PST
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 :)
Comment 13 Roland Steiner 2012-02-12 23:04:55 PST
Will do, once the tree goes back to a sane state.
Comment 14 Roland Steiner 2012-02-15 01:17:38 PST
Committed r107793: <http://trac.webkit.org/changeset/107793>