Closed Bug 1376931 Opened 7 years ago Closed 7 years ago

media queries with resolution: dppx values don't match window.devicePixelRatio, as they should

Categories

(Core :: DOM: CSS Object Model, defect, P3)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: zer0, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

MediaQueryList test
713 bytes, text/html
Details
Bug 1376931 Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1376931 Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1376931 Part 5: Add test to verify that media queries with a resolution set to window.devicePixelRatio dppx match for all zoom values exposed by the UI.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1376931 Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context.
59 bytes, text/x-review-board-request
heycam
: review+
bzbarsky
: review+
Details
Attached file MediaQueryList test — 
Open the attachment, and zoom in / zoom out.

Expected Result:
 - At each zoom, the previous and current devicePixelRatio is logged in the page.

Actual Result:
 - Only the first time the devicePixelRatio is logged (sometimes it was logged twice, but I'm not able to reproduce it all the times).

I'm not sure when this code stop working, at least from 53.

At least Responsive Design Mode is using the MediaQueryList in this way to detected when the device pixel ratio is changing, and we also have tests that do so.
Assignee: nobody → bwerth
Component: Layout → DOM: CSS Object Model
This doesn't seem to be a regression... I can reproduce this issue as described with the STR back to at least Firefox 30 (with a modified version to desugar ES6 features).

It isn't clear to me which in version were resolution and devicePixelRatio implemented, but I would guess this doesn't work from the very beginning...
Haven't had a close look, but this *might* be the restyle bug we're hitting in bug 1358688
Priority: -- → P3
This problem is happening because the notification loop for the MediaQueryLists can't handle adding another MediaQueryList during iteration. That happens when another matchMedia() call happens in the event listener. I'll change the notfication loop to copy the lists before iterating, and that should take care of it.
The issue in comment 3 is part of the problem, but not all of it. Another part of the problem is that we have a float precision comparison problem. In this case, when the MediaQueryList is generated for the 120% size, near 2.4 dppx, we end up doing a comparison that is close but not exact. The nearly 2.4 number is multiplied by 96 pixels per inch, and so we end comparing....

230.399994 from the MQL

vs

230.400009 from the PresContext.

Those aren't the same numbers, but the current code requires them to be the same for sensible things to happen. As it is, this comparison first happens whenthe MQL is given its first listener, and the code concludes (incorrectly) that the MQL does not match the current DPR, though it was created from the window.devicePixelRatio and should match. Later on, when the DPR does change, the MQL sees that it already didn't match, and doesn't match the current, so no need to fire a DPR change event.

So the second part of the solution will be either enforcing some kind of float precision in the calculation, or relaxing the epsilon of the comparison. My preference is to relax the comparison. Within 1e-5 would do it. I'll submit a patch that does that.
Attachment #8910986 - Flags: review?(cam)
Attachment #8910967 - Flags: review?(cam)
Attachment #8910966 - Flags: review?(cam)
I'm a little uncomfortable adding fuzziness to the comparisons to work around this.

It looks like window.devicePixelRatio does its computation with floats, even though the IDL defines the attribute to be a double.  (The IDL did use float at the time it was implemented in bug 564815.)  If we change nsGlobalWindow::GetDevicePixelRatioOuter to work with doubles instead, we should be able to get it to return 2.4 rather than 2.4000000953674316.  WDYT?  Can you check that this would work, i.e. that

  window.matchMedia(`(resolution: ${window.devicePixelRatio}dppx)`).matches

is true, for all the page zoom levels we support?

(I wonder if we should consider using dppx as the canonical unit for resolutions, too, instead of converting everything into dpi, since using dppx in media queries is probably a lot more common than dpi.  CSS Values & Units does say that dppx (well, "x") is the canonical unit for serialization.)
Flags: needinfo?(bwerth)
Depends on: 1402942
I opened bug 1402942 to address this. The test may move over to that bug.
Flags: needinfo?(bwerth)
(In reply to Cameron McCormack (:heycam) from comment #15)
> If we change nsGlobalWindow::GetDevicePixelRatioOuter to work with doubles instead, we
> should be able to get it to return 2.4 rather than 2.4000000953674316. 
> WDYT?  Can you check that this would work, i.e. that
> 
>   window.matchMedia(`(resolution: ${window.devicePixelRatio}dppx)`).matches
> 
> is true, for all the page zoom levels we support?

This doesn't help the MediaQueryList actually match. It does make the condition string become "(resolution: 2.4dppx)", but the internal math at http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/layout/style/nsMediaList.cpp#148 is still off by an epsilon.
Attachment #8912043 - Flags: review?(cam)
Attachment #8911428 - Flags: review?(cam)
Summary: [regression] MediaQueryList: event is dispatched only once → media queries with resolution: dppx values don't match window.devicePixelRatio, as they should
OK, one final question before going ahead with the epsilon approach.  If we changed resolution values to be stored as dppx rather than dpi, would that solve the problem?  Then we shouldn't be doing any floating point math with values that were converted from float to doubles, for dppx values.  For other units, we will divide by a constant (96 or 2.54) to get the dppx value, so we won't be amplifying any precision errors.  WDYT?
Flags: needinfo?(bwerth)
(In reply to Cameron McCormack (:heycam) (away Sep 28) from comment #29)
> OK, one final question before going ahead with the epsilon approach.  If we
> changed resolution values to be stored as dppx rather than dpi, would that
> solve the problem?  Then we shouldn't be doing any floating point math with
> values that were converted from float to doubles, for dppx values.  For
> other units, we will divide by a constant (96 or 2.54) to get the dppx
> value, so we won't be amplifying any precision errors.  WDYT?

The current set of patches implement that approach, built on top of the patches for Bug 1402942 which is landing soon.
Flags: needinfo?(bwerth)
(In reply to Brad Werth [:bradwerth] from comment #41)
> The current set of patches implement that approach, built on top of the
> patches for Bug 1402942 which is landing soon.

We also have a nasty Gecko -> Servo -> Gecko dependency in the patches. The changes in Part 1 will break Stylo until Parts 2 and 3 land. I have one idea on how to land this. I could create a "Part 0" patch that relaxes the Servo assert and adds inch-to-pixel conversion logic at http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/servo/components/style/gecko/media_queries.rs#377, then shift the Servo changes currently in Part 1 into a final Servo fixup patch that restores the assertion and removes the inch conversion logic. I'll restructure around that and open a dependent bug to do the final Servo fixup.
Blocks: 1404097
Attachment #8912043 - Flags: review?(bzbarsky)
Comment on attachment 8912043 [details]
Bug 1376931 Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context.

https://reviewboard.mozilla.org/r/183416/#review189916

r=me

::: layout/base/nsDocumentViewer.cpp:3233
(Diff revision 5)
>  
>  NS_IMETHODIMP
> +nsDocumentViewer::GetEffectiveFullZoom(float* aEffectiveFullZoom)
> +{
> +  NS_ENSURE_ARG_POINTER(aEffectiveFullZoom);
> +  #ifdef NS_PRINT_PREVIEW

Please outdent so the '#' is flush-left

::: layout/base/nsDocumentViewer.cpp:3238
(Diff revision 5)
> +  #ifdef NS_PRINT_PREVIEW
> +    if (GetIsPrintPreview()) {
> +      *aEffectiveFullZoom = mPrintPreviewZoom;
> +      return NS_OK;
> +    }
> +  #endif

And here.
Attachment #8912043 - Flags: review?(bzbarsky) → review+
Comment on attachment 8910986 [details]
Bug 1376931 Part 1: Change Servo media queries of resolution to compare in dppx units without unit conversion.

https://reviewboard.mozilla.org/r/182446/#review190256

::: servo/components/style/gecko/media_queries.rs:383
(Diff revision 7)
> +                  nsCSSUnit::eCSSUnit_Pixel => Resolution::Dppx(css_value.float_unchecked()),
> +                  nsCSSUnit::eCSSUnit_Inch  => Resolution::Dpi(css_value.float_unchecked()),
> +                  _                         => unreachable!(),

Nit: four space indent in Rust code.  (Although if it's just going to be removed in a later patch...)
Attachment #8910986 - Flags: review?(cam) → review+
Comment on attachment 8912043 [details]
Bug 1376931 Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context.

https://reviewboard.mozilla.org/r/183416/#review190258

::: docshell/base/nsIContentViewer.idl:215
(Diff revision 6)
> +  /** The actual full zoom in effect, as modified by the device context. */
> +  readonly attribute float effectiveFullZoom;

Can you clarify the difference between this and fullZoom in the comment?  (It also includes the device pixel scaling, yes?)

::: layout/base/nsDocumentViewer.cpp:3235
(Diff revision 6)
> +nsDocumentViewer::GetEffectiveFullZoom(float* aEffectiveFullZoom)
> +{
> +  NS_ENSURE_ARG_POINTER(aEffectiveFullZoom);
> +#ifdef NS_PRINT_PREVIEW
> +  if (GetIsPrintPreview()) {
> +    *aEffectiveFullZoom = mPrintPreviewZoom;

This means we use mPrintPreviewZoom for both .fullZoom and .effectiveFullZoom, when print preview is active.  Is this the right thing to do?  Is it just that when printing, we never consider the actual resolution, and so would effectively just always match resolution:1dppx (unless some full page zoom applied)?

I think it's worth a comment in here pointing out that we don't have a separate full zoom and effective full zoom in print preview.

::: layout/base/nsPresContext.h:607
(Diff revision 6)
>                                  NS_STYLE_HINT_REFLOW);
>      }
>    }
>  
>    float GetFullZoom() { return mFullZoom; }
> +  float GetEffectiveFullZoom();

Can you add a comment here too, to distinguish the two?
Attachment #8912043 - Flags: review?(cam) → review+
Comment on attachment 8910967 [details]
Bug 1376931 Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion.

https://reviewboard.mozilla.org/r/182428/#review190264
Attachment #8910967 - Flags: review?(cam) → review+
Comment on attachment 8910966 [details]
Bug 1376931 Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events.

https://reviewboard.mozilla.org/r/182426/#review190266

::: layout/base/nsPresContext.cpp:2123
(Diff revision 9)
> +    // XXX: refcounting?
> +    nsTArray<mozilla::dom::MediaQueryList *> localMediaQueryLists;

This would need to store strong references, since the event dispatch could remove an existing item from the list (by dropping its reference to it, then somehow causing a GC/CC).

Although https://drafts.csswg.org/cssom-view/#evaluate-media-queries-and-report-changes doesn't mention anything about taking a snapshot of the current list of MQLs, I think your new behavior here is correct, if we interpret "If target’s matches state has changed since the last time these steps were run" to evaluate to false if the MQL didn't exist the last time the steps were run.

I think this would be a good WPT to add.

::: layout/base/nsPresContext.cpp:2130
(Diff revision 9)
>      for (auto mql : mDocument->MediaQueryLists()) {
> +      localMediaQueryLists.AppendElement(mql);
> +    }
> +
> +    // Now iterate our local array of the lists.
> +    for (auto mql : localMediaQueryLists) {

After changing the above array of nsTArray<RefPtr<MediaQueryList>>, make sure this loop uses either "auto*" or "MediaQueryList*" so that we don't wastefully make a bunch of RefPtr<MediaQueryList> copies.
Attachment #8910966 - Flags: review?(cam) → review+
Comment on attachment 8911428 [details]
Bug 1376931 Part 5: Add test to verify that media queries with a resolution set to window.devicePixelRatio dppx match for all zoom values exposed by the UI.

https://reviewboard.mozilla.org/r/182886/#review190268

Can you please also just run test_bug418986-2.html locally (on your HiDPI machine) to ensure the fingerprinting resistance continues to effectively return 96dpi, since we don't have any CI in HiDPI configurations.
Attachment #8911428 - Flags: review?(cam) → review+
Comment on attachment 8910986 [details]
Bug 1376931 Part 1: Change Servo media queries of resolution to compare in dppx units without unit conversion.

https://reviewboard.mozilla.org/r/182424/#review190734
I see that I've created some confusion by using "effective" full zoom to mean "more precise zoom returned from the device context". Let me know if you'd prefer "GetPreciseFullZoom" instead.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) (away 4 Oct) from comment #58)
> Can you please also just run test_bug418986-2.html locally (on your HiDPI
> machine) to ensure the fingerprinting resistance continues to effectively
> return 96dpi, since we don't have any CI in HiDPI configurations.

I confirmed this test still passes.
(We discussed the naming on IRC.)
Flags: needinfo?(cam)
Attachment #8910986 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3adba9da5967
Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context. r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/61387758d1c4
Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d17d793f9850
Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events. r=heycam
https://hg.mozilla.org/integration/autoland/rev/6021cb8fcc53
Part 5: Add test to verify that media queries with a resolution set to window.devicePixelRatio dppx match for all zoom values exposed by the UI. r=heycam
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b04ebc4bcd3d
Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context. r=bz,heycam
https://hg.mozilla.org/integration/autoland/rev/06c8706c06c9
Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion. r=heycam
https://hg.mozilla.org/integration/autoland/rev/914b4cd64a4a
Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events. r=heycam
https://hg.mozilla.org/integration/autoland/rev/abdc7d29ddf8
Part 5: Add test to verify that media queries with a resolution set to window.devicePixelRatio dppx match for all zoom values exposed by the UI. r=heycam
Comment on attachment 8910966 [details]
Bug 1376931 Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events.

https://reviewboard.mozilla.org/r/182426/#review191644


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/base/nsPresContext.cpp:2129
(Diff revision 13)
> +    for (auto* mql : mDocument->MediaQueryLists()) {
> +      localMediaQueryLists.AppendElement(mql);
> +    }
> +
> +    // Now iterate our local array of the lists.
> +    for (auto mql : localMediaQueryLists) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

    for (auto mql : localMediaQueryLists) {
              ^
         const  &
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 59e5112caa73 -d 1e8554fbafad: rebasing 424254:59e5112caa73 "Bug 1376931 Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context. r=bz,heycam"
merging layout/base/nsPresContext.cpp
note: rebase of 424254:59e5112caa73 created no changes to commit
rebasing 424255:529e1414a901 "Bug 1376931 Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion. r=heycam"
note: rebase of 424255:529e1414a901 created no changes to commit
rebasing 424256:c9e89c16748c "Bug 1376931 Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events. r=heycam"
merging layout/base/nsPresContext.cpp
warning: conflicts while merging layout/base/nsPresContext.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 93eea13901d9 -d edb7fa9379c7: rebasing 424287:93eea13901d9 "Bug 1376931 Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context. r=bz,heycam"
merging layout/base/nsPresContext.cpp
note: rebase of 424287:93eea13901d9 created no changes to commit
rebasing 424288:03177d969434 "Bug 1376931 Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion. r=heycam"
note: rebase of 424288:03177d969434 created no changes to commit
rebasing 424289:f08d52724b17 "Bug 1376931 Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events. r=heycam"
merging layout/base/nsPresContext.cpp
warning: conflicts while merging layout/base/nsPresContext.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 93eea13901d9 -d edb7fa9379c7: rebasing 424287:93eea13901d9 "Bug 1376931 Part 2: Extend ContentViewer to allow reporting of effective full zoom level as determined by the device context. r=bz,heycam"
merging layout/base/nsPresContext.cpp
note: rebase of 424287:93eea13901d9 created no changes to commit
rebasing 424288:03177d969434 "Bug 1376931 Part 3: Change Gecko media queries of resolution to compare in dppx units without unit conversion. r=heycam"
note: rebase of 424288:03177d969434 created no changes to commit
rebasing 424289:f08d52724b17 "Bug 1376931 Part 4: Before MediaQueryList iteration for event listeners, copy the array before sending any events. r=heycam"
merging layout/base/nsPresContext.cpp
warning: conflicts while merging layout/base/nsPresContext.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Blocks: 1406108
Finally got this landed.
Flags: needinfo?(bwerth)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: