CSS Style is not recalculated when media attribute of style element is changed

[Alexander Shalamov]

When "media" attribute of style element is changed, style should be recalculated.

[Alexander Shalamov]

Created attachment 164099 [details]
Patch 1

For initial review.
I would like to get opinion about new layout test.
Should I write new layout test or we could import w3c media query tests?

[WebKit Review Bot]

Comment on attachment 164099 [details]
Patch 1

Attachment 164099 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13851476

[Alexander Shalamov]

Created attachment 164105 [details]
Patch 2

+#include "MediaList.h"

[WebKit Review Bot]

Attachment 164105 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/HTMLStyleElement.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Shalamov]

Created attachment 164120 [details]
Patch 3

fixed include order

[Ryosuke Niwa]

Comment on attachment 164120 [details]
Patch 3

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

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

We clearly need tests.

[Andreas Kling]

Comment on attachment 164120 [details]
Patch 3

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

> Source/WebCore/html/HTMLStyleElement.cpp:83
> +    HTMLElement::attributeChanged(attribute);
> +    if (attribute.name() == mediaAttr && inDocument()
> +        && !document()->parsing() && document()->renderer() && m_sheet) {
> +        RefPtr<MediaQuerySet> mediaQueries = MediaQuerySet::createAllowingDescriptionSyntax(attribute.value());
> +        m_sheet->setMediaQueries(mediaQueries.release());
> +        document()->styleResolverChanged(DeferRecalcStyle);
> +    }

This logic should be in parseAttribute().

[Alexander Shalamov]

Created attachment 164538 [details]
Patch 4

- Added new layout test
- Moved code from HTMLStyleElement::attributeChanged to HTMLStyleElement::parseAttribute

[Kenneth Rohde Christiansen]

Comment on attachment 164538 [details]
Patch 4

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

> LayoutTests/fast/media/mq-js-update-media-expected.txt:6
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x216
> +  RenderBlock {HTML} at (0,0) size 800x216
> +    RenderBody {BODY} at (8,8) size 784x200
> +      RenderBlock {DIV} at (0,0) size 200x200

Wasnt it better making this a text output? like query and print the color of the div?

> Source/WebCore/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).
> +
> +        When "media" attribute of style element is changed, style is recalculated.
> +
> +        Test: fast/media/mq-js-update-media.html
> +
> +        * css/CSSStyleSheet.cpp:
> +        (WebCore::CSSStyleSheet::setMediaQueries):

Please write HOW you are fixing this

> Source/WebCore/css/CSSStyleSheet.cpp:210
> +    if (m_mediaCSSOMWrapper && m_mediaQueries)
> +        m_mediaCSSOMWrapper->reattach(m_mediaQueries.get());

Is this efficient?

[WebKit Review Bot]

Comment on attachment 164538 [details]
Patch 4

Attachment 164538 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13900214

New failing tests:
fast/media/mq-js-update-media.html

[Alexander Shalamov]

(In reply to comment #9)
> (From update of attachment 164538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164538&action=review
> 
> > LayoutTests/fast/media/mq-js-update-media-expected.txt:6
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x216
> > +  RenderBlock {HTML} at (0,0) size 800x216
> > +    RenderBody {BODY} at (8,8) size 784x200
> > +      RenderBlock {DIV} at (0,0) size 200x200
> 
> Wasnt it better making this a text output? like query and print the color of the div?

I was thinking that getting render tree is faster than dumping text from test runner. I will rewrite test to get text output.

> 
> > Source/WebCore/ChangeLog:13
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        When "media" attribute of style element is changed, style is recalculated.
> > +
> > +        Test: fast/media/mq-js-update-media.html
> > +
> > +        * css/CSSStyleSheet.cpp:
> > +        (WebCore::CSSStyleSheet::setMediaQueries):
> 
> Please write HOW you are fixing this

I will. Thanks.

> > Source/WebCore/css/CSSStyleSheet.cpp:210
> > +    if (m_mediaCSSOMWrapper && m_mediaQueries)
> > +        m_mediaCSSOMWrapper->reattach(m_mediaQueries.get());
> 
> Is this efficient?

CSSOM wrapper is lazily initialised and never updated. When media query is updated CSSOM wrapper should be updated as well. Those lines are required to update media query CSSOM wrapper.

[Alexander Shalamov]

Created attachment 164682 [details]
Patch 5

- Added detailed explanation to changelog.
- Modified layout test that outputs text rather than render tree.

[Alexander Shalamov]

Created attachment 167295 [details]
Patch 6

Rebased to master

[WebKit Review Bot]

Attachment 167295 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/html/HTMLStyleElement.cpp:88:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Shalamov]

Created attachment 167298 [details]
Patch 7

Looks like check-webkit-style become stricter. Fix should make bot happy.

[Kenneth Rohde Christiansen]

Comment on attachment 167298 [details]
Patch 7

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

> LayoutTests/fast/media/mq-js-update-media.html:13
> +          if (window.testRunner) {
> +              testRunner.dumpAsText();
> +              testRunner.waitUntilDone();
> +          }

Why not do this outside the function?

> LayoutTests/fast/media/mq-js-update-media.html:19
> +          test(divComputedStyle.backgroundColor, "255", "Div should have rgb(255, 0, 0) background color.");

Maybe there is a shouldBe method for comparing this

> LayoutTests/fast/media/mq-js-update-media.html:33
> +      function test(actual, expected, message) {
> +          var re = /^rgba?\((\d+),\s*(\d+),\s*(\d+)(?:,\s*(\d+(?:\.\d+)?))?\)$/;
> +          var color = re.exec(actual)[1];
> +          if(color === expected)

This test should include the js test frame work used by other tests, and ie. not define log() itself.

[Alexander Shalamov]

Created attachment 167524 [details]
Patch 8

- Use testharness.js and testharnessreport.js
- Use getRGBColorValue instead of regexp to compare color component values

[Kenneth Rohde Christiansen]

Comment on attachment 167524 [details]
Patch 8

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

> Source/WebCore/html/HTMLStyleElement.cpp:87
> +    else if (attribute.name() == mediaAttr && inDocument() && !document()->parsing() && document()->renderer() && m_sheet) {

So what happens if it is parsing? Then this will happen when it is finished? Can it be tested? Why do you need the renderer() check?

[Alexander Shalamov]

(In reply to comment #18)
> (From update of attachment 167524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167524&action=review
> 
> > Source/WebCore/html/HTMLStyleElement.cpp:87
> > +    else if (attribute.name() == mediaAttr && inDocument() && !document()->parsing() && document()->renderer() && m_sheet) {
> 
> So what happens if it is parsing? Then this will happen when it is finished? Can it be tested?

I have to add javascript code to body and check that case. Thanks.

> Why do you need the renderer() check?

If document is being destroyed and there is no renderer, there is no point to recalc style.

[Kenneth Rohde Christiansen]

> If document is being destroyed and there is no renderer, there is no point to recalc style.

If that is the only case that can happen it sounds fine, but maybe a comment makes sense, like // The render will only be null doing destruction of the document, in which case we don't need to recalculate.

But then again, can't people use the media query listeners from javascript and actually need to get notified? ie. media queries doesn't always depend on style

[Alexander Shalamov]

Created attachment 167736 [details]
Patch 9

- Modified test to verify that media query is updated when document is parsed

[Alexander Shalamov]

(In reply to comment #20)
> > If document is being destroyed and there is no renderer, there is no point to recalc style.
> 
> If that is the only case that can happen it sounds fine, but maybe a comment makes sense, like // The render will only be null doing destruction of the document, in which case we don't need to recalculate.
> 
> But then again, can't people use the media query listeners from javascript and actually need to get notified? ie. media queries doesn't always depend on style

Are you talking about the case when style node is detached from document?

> > Source/WebCore/html/HTMLStyleElement.cpp:87
> > +    else if (attribute.name() == mediaAttr && inDocument() && !document()->parsing() && document()->renderer() && m_sheet) {
> 
> So what happens if it is parsing? Then this will happen when it is finished? Can it be tested?

I removed that condition, I was too strict. Checked all implementations of *::parseAttribute and parsing state is never checked.

[Kenneth Rohde Christiansen]

> > But then again, can't people use the media query listeners from javascript and actually need to get notified? ie. media queries doesn't always depend on style

Or say when it is not create yet.

I mean orientation can change at any point and you can write scripts 

mql = window.msMatchMedia("(orientation: landscape)");

function orientationChange(mql) 
{
  if (mql.matches) 
  {
     ... // landscape
  } 
  else 
  {
     ... // portrait
  }
}

mql.addListener(orientationChange);

[Kenneth Rohde Christiansen]

> mql = window.msMatchMedia("(orientation: landscape)");

matchMedia :-) I had to look up the documentation and used the MS one :-)

[Alexander Shalamov]

(In reply to comment #24)
> > mql = window.msMatchMedia("(orientation: landscape)");
> 
> matchMedia :-) I had to look up the documentation and used the MS one :-)

If there are media listeners, they would be notified and their media query re-evaluated.

Document::styleResolverChanged calls evaluateMediaQueryList() that would notify listeners if there are any.

[Kenneth Rohde Christiansen]

ok good, could you test that? just to make sure

[Alexander Shalamov]

(In reply to comment #26)
> ok good, could you test that? just to make sure

Sure

[Kenneth Rohde Christiansen]

Comment on attachment 167736 [details]
Patch 9

OK LGTM, but the added test would be nice!

[Darin Adler]

Comment on attachment 167736 [details]
Patch 9

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

> Source/WebCore/html/HTMLStyleElement.cpp:89
> +        RefPtr<MediaQuerySet> mediaQueries = MediaQuerySet::createAllowingDescriptionSyntax(attribute.value());
> +        m_sheet->setMediaQueries(mediaQueries.release());

Not sure the local variable here makes the code easier to read. I’d just do it on one line.

[Alexander Shalamov]

(In reply to comment #28)
> (From update of attachment 167736 [details])
> OK LGTM, but the added test would be nice!

Only test case I managed to implement didn't test what I wanted. I had an iframe and then I was changing style for iframe that triggered notification for media listeners in sub-document.

Unfortunately I don't have valid test for the use-case you've mentioned. Whenever there is a style change based on media query, document style is recalculated, but properties like width (frame), height, device-width, color, etc are not affected, therefore, notification for media query listeners is not fired.

I also tried to change font-size and were thinking that if media query uses rem units I could trigger notification, but for some reason that didn't work either, maybe there is a bug in media query evaluator for length calculation.

[Alexander Shalamov]

Created attachment 167811 [details]
Patch 10

- Fixed code according to Darin's comments

[Kenneth Rohde Christiansen]

Comment on attachment 167811 [details]
Patch 10

You can add the test in another patch. I am sure more fixes are needed as few features (orientation being one) actually trigger a reevaluated media query

[WebKit Review Bot]

Comment on attachment 167811 [details]
Patch 10

Clearing flags on attachment: 167811

Committed r130816: <http://trac.webkit.org/changeset/130816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.