[Webkit-unassigned] [Bug 78472] ShadowRoot needs applyAuthorStyles

Mon Apr 23 11:17:34 PDT 2012

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

--- Comment #13 from Takashi Sakamoto <tasak at google.com>  2012-04-23 11:17:34 PST ---
(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.

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