Bug 75903 - matchMedia() MediaQueryList not updating
: matchMedia() MediaQueryList not updating
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Luiz Agostini
: HasReduction, InRadar
Depends on: 84107
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 16:16 PST by Nicholas C. Zakas
Modified: 2012-04-18 11:40 PDT (History)
6 users (show)

See Also:


Attachments
patch (10.40 KB, patch)
2012-02-22 18:35 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch (11.21 KB, patch)
2012-02-22 22:06 PST, Luiz Agostini
koivisto: review-
Details | Formatted Diff | Diff
patch (9.83 KB, patch)
2012-02-23 16:37 PST, Luiz Agostini
koivisto: review-
Details | Formatted Diff | Diff
patch (8.20 KB, patch)
2012-03-29 17:08 PDT, Luiz Agostini
koivisto: review+
koivisto: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas C. Zakas 2012-01-09 16:16:59 PST
The implementation of matchMedia() is incomplete in two ways:

1) The returned MediaQueryList's matches property never changes. The matches property should return the correct value based on whether or not the media query matches. The WebKit version only initializes the matches property upon calling matchMedia().

2) The media query listeners are never fired. I suspect this is related to #1, since you're not tracking changes, the listeners are unable to fire when the query becomes valid for the page.

Simple test case:

var media = window.matchMedia("screen and (max-width:600px)");
console.log("Does it match now? " + media.matches);
media.addListener(function(media){
    console.log("HERE: " + window.innerWidth);
});

When resizing the window to less than 600px, you should see a console message appear. It does so in Firefox but not in Chrome or Safari


My UA string:
Mozilla/5.0 (Windows NT 5.1) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.26 Safari/535.11
Comment 1 Nicholas C. Zakas 2012-01-17 11:45:21 PST
Update: It appears that adding a style block with the same media query causes the JavaScript API to work correctly. So if I add:

@media screen and (max-width:600px) {
    .foo {}
}

Then the JavaScript in my previous comment works as expected.
Comment 2 Luiz Agostini 2012-02-22 18:35:10 PST
Created attachment 128363 [details]
patch
Comment 3 Luiz Agostini 2012-02-22 22:06:57 PST
Created attachment 128393 [details]
patch
Comment 4 Antti Koivisto 2012-02-23 13:20:46 PST
Comment on attachment 128393 [details]
patch

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

> Source/WebCore/page/FrameView.cpp:973
> -        // Viewport-dependent media queries may cause us to need completely different style information.
> -        // Check that here.
> -        if (document->styleSelector()->affectedByViewportChange()) {
> -            document->styleSelectorChanged(RecalcStyleImmediately);
> -            InspectorInstrumentation::mediaQueryResultChanged(document);
> -        }
> +        document->styleSelectorChanged(RecalcStyleImmediately, Document::ViewportChangeReason);

Instead of adding a flag to an existing function, you should add a new one that handles viewport changes and calls to styleSelectorChanged() if needed. Document::evaluateViewportDependentMediaQueries() or similar.
Comment 5 Luiz Agostini 2012-02-23 16:37:28 PST
Created attachment 128599 [details]
patch
Comment 6 Luiz Agostini 2012-02-23 16:54:23 PST
(In reply to comment #4)
> (From update of attachment 128393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128393&action=review
> 
> > Source/WebCore/page/FrameView.cpp:973
> > -        // Viewport-dependent media queries may cause us to need completely different style information.
> > -        // Check that here.
> > -        if (document->styleSelector()->affectedByViewportChange()) {
> > -            document->styleSelectorChanged(RecalcStyleImmediately);
> > -            InspectorInstrumentation::mediaQueryResultChanged(document);
> > -        }
> > +        document->styleSelectorChanged(RecalcStyleImmediately, Document::ViewportChangeReason);
> 
> Instead of adding a flag to an existing function, you should add a new one that handles viewport changes and calls to styleSelectorChanged() if needed. Document::evaluateViewportDependentMediaQueries() or similar.

ok. I've uploaded a new patch.
Comment 7 Antti Koivisto 2012-02-27 11:22:23 PST
Comment on attachment 128599 [details]
patch

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

> Source/WebCore/dom/Document.cpp:3031
> +void Document::viewportDependentStyleSelectorChange(StyleSelectorUpdateFlag updateFlag)
> +{

Why does this have an argument? You only ever invoke it with RecalcStyleImmediately.

Why didn't you follow the naming suggestion?

> Source/WebCore/dom/Document.cpp:3039
> +    if (m_mediaQueryMatcher)
> +        m_mediaQueryMatcher->styleSelectorChanged();

Why are you calling styleSelectorChanged() even when style selector did not change?

> Source/WebCore/dom/Document.cpp:3050
>  void Document::styleSelectorChanged(StyleSelectorUpdateFlag updateFlag)
>  {
> +    updateStyleSelector(updateFlag);
> +    if (m_mediaQueryMatcher)
> +        m_mediaQueryMatcher->styleSelectorChanged();
> +}
> +
> +void Document::updateStyleSelector(StyleSelectorUpdateFlag updateFlag)
> +{

Why are you adding a new function? It adds no obvious value.
Comment 8 Luiz Agostini 2012-02-27 12:01:17 PST
(In reply to comment #7)

The MediaQueries need to be checked every time styleSelectorChanged is called, unconditionally. I have made a small refactoring, renaming styleSelectorChanged to updateStyleSelector and wrote a new styleSelectorChanged that calls updateStyleSelector and m_mediaQueryMatcher->styleSelectorChanged(). It keeps the code in Documet::styleSelectorChanged unchanged and makes sure that m_mediaQueryMatcher->styleSelectorChanged() will always get called.

There is an optimization in FrameView::layout that avoids calling styleSelectorChanged if the document's style selector is not affected by viewport changes. The problem is that the media queries need to be checked anyway. That is why I moved the call styleSelector()->affectedByViewportChange() from FrameView to Document.

The objective is to make sure that the media query listeners will always get called anytime its media query changes.

> (From update of attachment 128599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128599&action=review
> 
> > Source/WebCore/dom/Document.cpp:3031
> > +void Document::viewportDependentStyleSelectorChange(StyleSelectorUpdateFlag updateFlag)
> > +{
> 
> Why does this have an argument? You only ever invoke it with RecalcStyleImmediately.

This is just a parameter to styleSelectorChanged. It may be removed for sure.

> 
> Why didn't you follow the naming suggestion?

The method should call styleSelectorChanged if needed and call m_mediaQueryMatcher->styleSelectorChanged(). I think that it is not only about evaluating viewport dependent MediaQueries.

> 
> > Source/WebCore/dom/Document.cpp:3039
> > +    if (m_mediaQueryMatcher)
> > +        m_mediaQueryMatcher->styleSelectorChanged();
> 
> Why are you calling styleSelectorChanged() even when style selector did not change?

m_mediaQueryMatcher->styleSelectorChanged checks the media queries and call the related listeners if needed.

> 
> > Source/WebCore/dom/Document.cpp:3050
> >  void Document::styleSelectorChanged(StyleSelectorUpdateFlag updateFlag)
> >  {
> > +    updateStyleSelector(updateFlag);
> > +    if (m_mediaQueryMatcher)
> > +        m_mediaQueryMatcher->styleSelectorChanged();
> > +}
> > +
> > +void Document::updateStyleSelector(StyleSelectorUpdateFlag updateFlag)
> > +{
> 
> Why are you adding a new function? It adds no obvious value.

It just makes sure that m_mediaQueryMatcher->styleSelectorChanged() will always get called without making changes to the code in Document::styleSelectorChanged.
Comment 9 Luiz Agostini 2012-03-29 17:08:55 PDT
Created attachment 134698 [details]
patch
Comment 10 Antti Koivisto 2012-04-09 09:24:06 PDT
Comment on attachment 134698 [details]
patch

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

> Source/WebCore/dom/Document.cpp:3121
> +void Document::evaluateMediaQueries()
> +{
> +    if (m_mediaQueryMatcher)
> +        m_mediaQueryMatcher->styleSelectorChanged();
> +}

evaluateMediaQueries() is not a great name here as it gives impression that all media queries are evaluated. Something like evaluateMediaQueryList would be better. (MediaQueryMatcher:: styleSelectorChanged() makes no sense either)
Comment 11 Radar WebKit Bug Importer 2012-04-10 12:48:19 PDT
<rdar://problem/11220887>
Comment 12 Antti Koivisto 2012-04-16 11:46:28 PDT
Landed this manually, http://trac.webkit.org/changeset/114285 (with the naming change)
Comment 13 Anders Carlsson 2012-04-16 17:54:39 PDT
I rolled out this, the test added always times out on the Mac tester. It looks like the test was also skipped on Qt.
Comment 14 Antti Koivisto 2012-04-17 19:42:06 PDT
Mac DRT just doesn't handle window resizing correctly. Chromium DRT seems to be the only one that gets this right.
Comment 15 Antti Koivisto 2012-04-18 11:40:27 PDT
Re-landed in http://trac.webkit.org/changeset/114538 with the test skipped on Mac (DRT would need some heavy patching, test is fine manually)