Bug 22429 – document.styleSheets collection ignores media=presentation

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Martin Hassman]

Collection document.styleSheets should contain all stylesheets from the document, but Webkit ignores stylesheets with media="presentation" from this collection.

[Martin Hassman]

Created attachment 25380 [details]
Testcase

Testcase contains 4 stylesheets, but document.styleSheets.length is 3 (the media="projection" is missing).

[Rob Buis]

Created attachment 26014 [details]
First attempt

I think it is not just media="projection", since with any media attribute I try the sheet ends up in the styleSheets collection. This patch makes an empty sheet entry to make the styleSheets collection correct. Since they are empty there are no rules that contribute to the style collection. Obviously there are speed regressions when encountering media other than all, screen or print, but I assume these are uncommon. If this is the right approach I'll make some testcase(s).
Cheers,

Rob.

[Darin Adler]

Comment on attachment 26014 [details]
First attempt

> +        document.styleSheets collection ognores media=presentation

"ognores"

> +            if (m_sheet)
> +                m_sheet = 0;

The above is a strange way to write "m_sheet = 0".

But also, is there a reason we need to set m_sheet to 0 separately before assigning a new value? Sometimes we have done that intentionally in the past when there was a need to guarantee that the old ref went away before the new object was created. But I think in this case that code should just be omitted.

> +            // create a dummy sheet without url and content

I'd prefer to see new comments be written as full sentences. I'd write it like this:

    // Create a dummy sheet without a URL or content.

> +            m_sheet = CSSStyleSheet::create(this, m_url, chset);
> +            m_sheet->setTitle(title());
> +            m_sheet->setMedia(media.get());

This should be media.release(), not media.get().

Is an empty CSSStyleSheet really what's called for here? Don't we need a style sheet with a DOM that scripts can look at? Why is an empty one OK?

> +            document()->updateStyleSelector();

It's a shame to call updateStyleSelector in this case. There are no new style rules, and there's no need to do work unless there was a non-empty style sheet before that we're replacing. It would be better to treat an empty style sheet the same as lack of a style sheet and call updateStyleSelector only when it is really needed. We don't want to do needless style computation when you add a <link> for a style sheet that fails the media query.

I'm going to say review- because I'd like to see a justification of the value of creating an empty style sheet. I really don't understand why it's correct to not load the style sheet and create an empty style sheet object. Is this what other browsers do? Or do they load these style sheets?

I'd like to hear Hyatt's thoughts on this too.

[Rob Buis]

(In reply to comment #3)
> (From update of attachment 26014 [details] [review])
> > +        document.styleSheets collection ognores media=presentation
> 
> "ognores"

Right, I didn't want to touch the original title, but I guess it is better to fix it.

> > +            if (m_sheet)
> > +                m_sheet = 0;
> 
> The above is a strange way to write "m_sheet = 0".

Right. I first had some logic in the if to set a bool to decide whether updateStyleSelector should be called.

> But also, is there a reason we need to set m_sheet to 0 separately before
> assigning a new value? Sometimes we have done that intentionally in the past
> when there was a need to guarantee that the old ref went away before the new
> object was created. But I think in this case that code should just be omitted.

Possible, I am not sure.

> > +            // create a dummy sheet without url and content
> 
> I'd prefer to see new comments be written as full sentences. I'd write it like
> this:
> 
>     // Create a dummy sheet without a URL or content.

I'll remember for future comments.

> > +            m_sheet = CSSStyleSheet::create(this, m_url, chset);
> > +            m_sheet->setTitle(title());
> > +            m_sheet->setMedia(media.get());
> 
> This should be media.release(), not media.get().

Ok.
 
> Is an empty CSSStyleSheet really what's called for here? Don't we need a style
> sheet with a DOM that scripts can look at? Why is an empty one OK?

I sort of did that because of efficiency. But it turns out FF and Opera still load stylesheets
for links with invalid/uncommon media attributes.

> > +            document()->updateStyleSelector();
> 
> It's a shame to call updateStyleSelector in this case. There are no new style
> rules, and there's no need to do work unless there was a non-empty style sheet
> before that we're replacing. It would be better to treat an empty style sheet
> the same as lack of a style sheet and call updateStyleSelector only when it is
> really needed. We don't want to do needless style computation when you add a
> <link> for a style sheet that fails the media query.

Agreed.

> I'm going to say review- because I'd like to see a justification of the value
> of creating an empty style sheet. I really don't understand why it's correct to
> not load the style sheet and create an empty style sheet object. Is this what
> other browsers do? Or do they load these style sheets?

Right, FF and Opera do load them.
I did a new patch that does load and a start of a testcase, I'll upload what I have right now.
Cheers,

Rob.

[Rob Buis]

Created attachment 26031 [details]
Different approach

This patch simply loads all sheets regardless of the media attribute. However addRulesFromSheet still blocks the rules in this sheet. The testcase is a start but should also verify that the sheet gets loaded but does not apply for media attributes other than all, screen and print. I am not sure yet whether those should be multipe testcases. It is a bit hard for me to judge efficiency, obviously it is slower when there are sheets with media other than all, screen or print but I think that is uncommon. A more efficient solution would be to make ::styleSheets get all sheets on demand but I guess that would involve it waiting for all sheets to load before returning.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 26031 [details]
Different approach

Ideally your test should also make sure that these loaded sheets also have some expected content.  I.e. you could iterate over the CSSStyleSheet dom object looking for a specific known rule.

[Rob Buis]

Created attachment 26236 [details]
Improved testcase

This patch now reuses an existing style sheet in resources and tests that it gets loaded, as suggested by Eric.
Cheers,

Rob.

[Darin Adler]

I need Hyatt's take on this. It seems like we need to parse all stylesheets, but it also seems like it's going to slow down browsing.

I think the patch is great, but I'm not going to review it on my own without input from Dave.

[Eric Seidel (no email)]

Comment on attachment 26236 [details]
Improved testcase

I'm confused by your test case.  Why don't you explicitly look for the types of stylesheets in the test?  Or test for a specific number of exposed stylesheets?

Your patch should ideally contain the expected results as well.  Also, it would be good to know what other browsers do on this test.  Knowing what other browsers do makes it easier for the reviewer to agree with the patch concept. :)

[Eric Seidel (no email)]

Comment on attachment 26236 [details]
Improved testcase

Marking this as r=hyatt based on darin adler's comments above.

[Dave Hyatt]

This seems ok.  It's unfortunate that we have to load irrelevant sheets, but if other browsers do it, I guess we have to.

[Dave Hyatt]

Comment on attachment 26236 [details]
Improved testcase

r=me

[David Levin]

Assign to levin for landing.

[David Levin]

Added the missing expected results: fast/css/sheet-collection-link-expected.txt

[David Levin]

Committed as http://trac.webkit.org/changeset/44277.

[Antti Koivisto]

Did this actually fix any real problems? It is pretty silly to load stylesheets that are never going to be used.

While braille sheets are too rare to matter, some complex media queries (for example to pick a stylesheet for device screen dimensions) may be more common. We should be able to optimize cases like that and the behavior from this patch goes exactly to wrong direction.

[Antti Koivisto]

I mean cases like this http://broadcast.oreilly.com/2010/04/using-css-media-queries-ipad.html