[Webkit-unassigned] [Bug 87805] [Shadow DOM] <style> inside Shadow subtree should be scoped inside the subtree.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=87805





--- Comment #4 from Takashi Sakamoto <tasak at google.com>  2012-06-05 07:54:14 PST ---
Thank you for reviewing.

(In reply to comment #2)
> (From update of attachment 145543 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145543&action=review
> 
> This looks good overall, I think we need a bit more test coverage:
> 1) at least make sure that all of the state transitions are accounted for
> 2) look at nested shadow dom and multiple shadow roots per element.
> 

I see. I added more tests.
I think, some existing test would probably cover the case: moving styled scope into shadow dom subtree from document or into document from shadow dom subtree.

> > Source/WebCore/html/HTMLStyleElement.cpp:55
> >      , m_isRegisteredWithScopingNode(false)
> > +    , m_scoped(false)
> 
> Can we not combine these two? Is there ever a situation when you are scoped, but have no scoping node?

I see. Done.

> 
> > Source/WebCore/html/HTMLStyleElement.cpp:96
> > +void HTMLStyleElement::updateForStyleScoped()
> 
> Would it work better if we name it as a notification to react and combine into one method?: styleScopedUpdated(attribute.isNull());

Done.

> 
> > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:28
> > +    debug("E: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('color')); /* red */
> > +    debug("E: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('border')); /* blue 1px border */
> > +    debug("F: " + document.defaultView.getComputedStyle(f, null).getPropertyValue('color')); /* black */
> > +    debug("F: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('border')); /* blue 1px border */
> 
> this can be a function.

Done.

> 
> > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:35
> > +    document.body.offsetLeft;
> 
> Why do we need this here?
> 
> > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:42
> > +    document.body.offsetLeft;
> 
> .. or here?

I see. These offestLefts don't make sense. I moved these offsetLefts to the next line of setAttribute or removeAttribute.

Best regards,
Takashi Sakamoto

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.