Bug 78472

Summary:

ShadowRoot needs applyAuthorStyles

Product:

WebKit

Reporter:

Shinya Kawanaka <shinyak>

Component:

DOM

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, dominicc, hayato, kaustubh.ra, macpherson, menard, morrita, ojan, shinyak, tasak, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

63601

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Description Shinya Kawanaka 2012-02-13 01:14:52 PST

Currently ShadowRoot does not have applyAuthorStyle API.
We should have this.

Comment 1 Shinya Kawanaka 2012-02-13 01:17:25 PST

See spec: http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#shadow-root-attributes

Comment 2 Takashi Sakamoto 2012-04-12 22:43:03 PDT

Created attachment 137040 [details]
Patch

Comment 3 Hajime Morrita 2012-04-16 19:19:21 PDT

Comment on attachment 137040 [details]
Patch

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

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:1
> +<!DOCTYPE html>

I guess we can use reftest and which will be much simpler.
http://trac.webkit.org/wiki/Writing%20Reftests

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:17
> +    -webkit-appearance: none;

Could you use some more trivial element like div and better-known styles?
Soem port haven't enabled <progress> element.

Comment 4 Takashi Sakamoto 2012-04-17 09:59:29 PDT

Created attachment 137553 [details]
Patch

Comment 5 Takashi Sakamoto 2012-04-17 10:04:43 PDT

> (From update of attachment 137040 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137040&action=review
> 
> > LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:1
> > +<!DOCTYPE html>
> 
> I guess we can use reftest and which will be much simpler.
> http://trac.webkit.org/wiki/Writing%20Reftests

I see. I rewrote the test.

> 
> > LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:17
> > +    -webkit-appearance: none;
> 
> Could you use some more trivial element like div and better-known styles?

Sure.
Now I'm using <span> element for the test.

> Soem port haven't enabled <progress> element.

I see.

Comment 6 Hajime Morrita 2012-04-18 08:49:02 PDT

Comment on attachment 137553 [details]
Patch

Almost looks good. Could you add another test case for covering the default value of applyAuthorStyles?

Comment 7 Hajime Morrita 2012-04-18 08:52:03 PDT

(In reply to comment #6)
> (From update of attachment 137553 [details])
> Almost looks good. Could you add another test case for covering the default value of applyAuthorStyles?
Yet another case we should cover is for dynamic changes, that is
- Add a ShadowRoot,
- ensuring it laid-out  (by accessing something like offsetLeft), 
- then change the property.

Comment 8 Takashi Sakamoto 2012-04-18 10:29:36 PDT

Created attachment 137719 [details]
Patch

Comment 9 Takashi Sakamoto 2012-04-18 10:32:53 PDT

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 137553 [details] [details])
> > Almost looks good. Could you add another test case for covering the default value of applyAuthorStyles?

I see. Done.

> Yet another case we should cover is for dynamic changes, that is
> - Add a ShadowRoot,
> - ensuring it laid-out  (by accessing something like offsetLeft), 
> - then change the property.

I couldn't find any explanation about whether dynamic change causes style recalculation or not. 
Anyway, I created a new bug https://bugs.webkit.org/show_bug.cgi?id=84251 for style recalculation.

Comment 10 Hajime Morrita 2012-04-18 13:43:23 PDT

(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 137553 [details] [details] [details])
> > > Almost looks good. Could you add another test case for covering the default value of applyAuthorStyles?
> 
> I see. Done.
Which one?

> 
> I couldn't find any explanation about whether dynamic change causes style recalculation or not. 
> Anyway, I created a new bug https://bugs.webkit.org/show_bug.cgi?id=84251 for style recalculation.
Thanks for filing it.

Comment 11 Ryosuke Niwa 2012-04-21 18:26:41 PDT

Comment on attachment 137719 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        seems to work in the same way as applyAuthorStyles, just renamed all applyAuthorSheets and
> +        added applyAuthorStyles to ShadowRoot.idl.

Nit: renamed all applyAuthorSheets to applyAuthorStyles, and added applyAuthorStyles to ShadowRoot.idl.

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles-expected.html:4
> +<head>
> +</head>

We don't need head for this, do we?

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles-expected.html:7
> +<div id="apply-author-style"><span style="margin:2px; border:solid"></span></div>
> +<div id="no-apply-author-style"><span></span></div>

Can we add more test cases like font-size, color, background-color, etc...?
Also, you should definitely test cases where the author style is overridden by inline style declaration or
cases where it overrides UA rules or when it's overridden by important UA rules.
Furthermore, you should test a case where you change the value of applyAuthorStyles dynamically and
verify that the change takes place dynamically as expected.

r- due to lack of adequate tests.

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:27
> +    var sr = new WebKitShadowRoot(div);

Please don't use abbreviations like sr. Also, does this constructor work on ports that don't enable shadow root support yet?

> LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:31
> +    var span = document.createElement('span');
> +    sr.appendChild(span);

Since you're not using span elsewhere, you can just do:
sr.appendChild(document.createElement('span'));

Comment 12 Takashi Sakamoto 2012-04-23 11:09:15 PDT

Created attachment 138386 [details]
Patch

Comment 13 Takashi Sakamoto 2012-04-23 11:17:34 PDT

(In reply to comment #11)
> (From update of attachment 137719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137719&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        seems to work in the same way as applyAuthorStyles, just renamed all applyAuthorSheets and
> > +        added applyAuthorStyles to ShadowRoot.idl.
> 
> Nit: renamed all applyAuthorSheets to applyAuthorStyles, and added applyAuthorStyles to ShadowRoot.idl.

Done.

> 
> > LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles-expected.html:4
> > +<head>
> > +</head>
> 
> We don't need head for this, do we?

I see. Done.

> > LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles-expected.html:7
> > +<div id="apply-author-style"><span style="margin:2px; border:solid"></span></div>
> > +<div id="no-apply-author-style"><span></span></div>
> 
> Can we add more test cases like font-size, color, background-color, etc...?
> Also, you should definitely test cases where the author style is overridden by inline style declaration or
> cases where it overrides UA rules or when it's overridden by important UA rules.

I see. I added these test cases.

> Furthermore, you should test a case where you change the value of applyAuthorStyles dynamically and
> verify that the change takes place dynamically as expected.

I created a bug, 84215 for fixing "dynamically changing applyAuthorStyles doesn't work" and will fix after finishing this bug.

> 
> r- due to lack of adequate tests.
> 
> > LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:27
> > +    var sr = new WebKitShadowRoot(div);
> 
> Please don't use abbreviations like sr.

I renamed all sr to shadowRoot.

> Also, does this constructor work on ports that don't enable shadow root support yet?

I added resources/polyfill.js for the ports.

> 
> > LayoutTests/fast/dom/shadow/shadow-root-applyAuthorStyles.html:31
> > +    var span = document.createElement('span');
> > +    sr.appendChild(span);
> 
> Since you're not using span elsewhere, you can just do:
> sr.appendChild(document.createElement('span'));

I found that shadow root's innerHTML attirbute works. So I rewrote all test cases to use innerHTML, e.g. shadowRoot.innerHTML = '<span></span>'; and so on.

Comment 14 Takashi Sakamoto 2012-05-01 04:46:43 PDT

Created attachment 139610 [details]
Patch

Comment 15 Takashi Sakamoto 2012-05-01 04:48:19 PDT

(In reply to comment #14)
> Created an attachment (id=139610) [details]
> Patch

Since CSSStyleSelector.cpp was renamed to StyleResolver.cpp (and also class CSSStyleSelector was renamed to class StyleResolver), I created a patch again.

Comment 16 Hajime Morrita 2012-05-09 04:37:56 PDT

Comment on attachment 139610 [details]
Patch

Sorry for really late response. Let's move forward.

Comment 17 WebKit Review Bot 2012-05-09 05:52:57 PDT

Comment on attachment 139610 [details]
Patch

Clearing flags on attachment: 139610

Committed r116521: <http://trac.webkit.org/changeset/116521>

Comment 18 WebKit Review Bot 2012-05-09 05:53:16 PDT

All reviewed patches have been landed.  Closing bug.