Bug 75903 - matchMedia() MediaQueryList not updating
: matchMedia() MediaQueryList not updating
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: HasReduction, InRadar
: 84107
:
  Show dependency treegraph
 
Reported: 2012-01-09 16:16 PST by
Modified: 2012-04-18 11:40 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2012-02-22 18:35:10 PST -------
Created an attachment (id=128363) [details]
patch
------- Comment #3 From 2012-02-22 22:06:57 PST -------
Created an attachment (id=128393) [details]
patch
------- Comment #4 From 2012-02-23 13:20:46 PST -------
(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.
------- Comment #5 From 2012-02-23 16:37:28 PST -------
Created an attachment (id=128599) [details]
patch
------- Comment #6 From 2012-02-23 16:54:23 PST -------
(In reply to comment #4)
> (From update of attachment 128393 [details] [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 From 2012-02-27 11:22:23 PST -------
(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.

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 From 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] [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 From 2012-03-29 17:08:55 PST -------
Created an attachment (id=134698) [details]
patch
------- Comment #10 From 2012-04-09 09:24:06 PST -------
(From update of attachment 134698 [details])
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 From 2012-04-10 12:48:19 PST -------
<rdar://problem/11220887>
------- Comment #12 From 2012-04-16 11:46:28 PST -------
Landed this manually, http://trac.webkit.org/changeset/114285 (with the naming change)
------- Comment #13 From 2012-04-16 17:54:39 PST -------
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 From 2012-04-17 19:42:06 PST -------
Mac DRT just doesn't handle window resizing correctly. Chromium DRT seems to be the only one that gets this right.
------- Comment #15 From 2012-04-18 11:40:27 PST -------
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)