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

Shinya Kawanaka

Reported 2012-02-13 01:14:52 PST

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

Attachments
Patch (11.08 KB, patch) 2012-04-12 22:43 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.61 KB, patch) 2012-04-17 09:59 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (8.81 KB, patch) 2012-04-18 10:29 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (11.54 KB, patch) 2012-04-23 11:09 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Patch (11.40 KB, patch) 2012-05-01 04:46 PDT, Takashi Sakamoto	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 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

Takashi Sakamoto

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

Created attachment 137040 [details] Patch

Hajime Morrita

Comment 3 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.

Takashi Sakamoto

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

Created attachment 137553 [details] Patch

Takashi Sakamoto

Comment 5 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.

Hajime Morrita

Comment 6 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?

Hajime Morrita

Comment 7 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.

Takashi Sakamoto

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

Created attachment 137719 [details] Patch

Takashi Sakamoto

Comment 9 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.

Hajime Morrita

Comment 10 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.

Ryosuke Niwa

Comment 11 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'));

Takashi Sakamoto

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

Created attachment 138386 [details] Patch

Takashi Sakamoto

Comment 13 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.

Takashi Sakamoto

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

Created attachment 139610 [details] Patch

Takashi Sakamoto

Comment 15 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.

Hajime Morrita

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

Comment on attachment 139610 [details] Patch Sorry for really late response. Let's move forward.

WebKit Review Bot

Comment 17 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>

WebKit Review Bot

Comment 18 2012-05-09 05:53:16 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.