Bug 75903 – matchMedia() MediaQueryList not updating

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

[Nicholas C. Zakas]

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

[Nicholas C. Zakas]

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.

[Luiz Agostini]

Created attachment 128363 [details]
patch

[Luiz Agostini]

Created attachment 128393 [details]
patch

[Antti Koivisto]

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.

[Luiz Agostini]

Created attachment 128599 [details]
patch

[Luiz Agostini]

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

[Antti Koivisto]

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.

[Luiz Agostini]

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

[Luiz Agostini]

Created attachment 134698 [details]
patch

[Antti Koivisto]

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)

[Radar WebKit Bug Importer]

<rdar://problem/11220887>

[Antti Koivisto]

Landed this manually, http://trac.webkit.org/changeset/114285 (with the naming change)

[Anders Carlsson]

I rolled out this, the test added always times out on the Mac tester. It looks like the test was also skipped on Qt.

[Antti Koivisto]

Mac DRT just doesn't handle window resizing correctly. Chromium DRT seems to be the only one that gets this right.

[Antti Koivisto]

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)