Closed Bug 1954493 Opened 5 months ago Closed 4 months ago

Device pixel ratio is different from the media query resolution when using RFP

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
139 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr128 --- unaffected
firefox137 --- wontfix
firefox138 --- wontfix
firefox139 --- fixed

People

(Reporter: pierov, Assigned: pierov)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1954493 - Make DPR use the correct zoom in RFP. r?#anti-tracking
48 bytes, text/x-phabricator-request
Details | Review

window.devicePixelRatio is wrong when using RFP, because it uses nsPresContext::GetFullZoom rather than nsPresContext::GetDeviceFullZoom().

STR: notice the difference in values with and without RFP:

<body>
<pre></pre>
<script>
const target = document.querySelector("pre");
const resizeObserver = new ResizeObserver(() => {
  target.textContent = window.devicePixelRatio;
});
resizeObserver.observe(document.body);
</script>
</body>

In particular, both 210% and 220% show 4.285714285714286, rather than 22.0689655172413794 and 22.2222222222222223, respectively.

Regressed by: 1873382

Also, refactor nsRFPService::GetDevicePixelRatioAtZoom to make it
easier to understand what the intermediate results are.

Attachment #9472671 - Attachment description: WIP: Bug 1954493 - Make DPR use the correct zoom in RFP. → Bug 1954493 - Make DPR use the correct zoom in RFP. r?#anti-tracking
Severity: -- → S3
Priority: -- → P3

Hello :emilio, would you be able to help us verify our hypothesis?

We have a function called nsRFPService::GetDevicePixelRatioAtZoom which is a replica of the logic in nsDeviceContext for computing devicePixelRatio at different zoom levels. We noticed that RFP'ed window.devicePixelRatio is generally off by small amounts. We think that's because we call nsRFPService::GetDevicePixelRatioAtZoom(nsPresContext::mFullZoom) which uses the readjusted zoom level. However, when nsDeviceContext computes mAppUnitsPerDevPixel, it uses the zoom level from nsPresContext which is not the readjusted value.

Our question is, would you be able to confirm our idea that nsPresContext::mFullZoom is the correct value (instead of nsDeviceContext::mFullZoom, or something else) for computing devicePixelRatio? Our testing shows that it is correct, but we wanted to ask to be extra sure.

 
 
 
 
 

Additional context should you need it:

We have two issues. First one is, there's a discrepancy between window.devicePixelRatio and media query device pixel ratio in resist fingerprinting mode, and the second one is non-RFP window.devicePixelRatio doesn't match with RFP'ed window.devicePixelRatio at different zooms. This patch was intended to fix the first issue and it fixes it. Turns out the discrepancy was caused because we were using nsDeviceContext::mFullZoom in one place and nsPresContext::mFullZoom in another. During discussion, we realized that's probably also what causes the discrepancy between non-rfp and rfp window.devicePixelRatio. Our testing shows that using nsPresContext::mFullZoom instead of nsDeviceContext::mFullZoom when computing the spoofed devicePixelRatio is correct, but we wanted to verify regardless.

Flags: needinfo?(emilio)

(In reply to Fatih Kilic [:fkilic] from comment #2)

Our question is, would you be able to confirm our idea that nsPresContext::mFullZoom is the correct value (instead of nsDeviceContext::mFullZoom, or something else) for computing devicePixelRatio? Our testing shows that it is correct, but we wanted to ask to be extra sure.

Using nsPresContext::mFullZoom seems closer to what nsDeviceContext / nsPresContext does, but not sure it's perfect. It seems you'd want to call ApplyFullZoom(), with 2 * AppUnitsPerDevPixel() and the pres context's full zoom as an argument, then do that rather than presContext->AppUnitsPerDevPixel in the devicePixelRatio getter.

But I'd also note that nsPresContext::mFullZoom still contains values that it seems you're trying to hide, like the OS text scale factor? So both approaches seem wrong, really.

Flags: needinfo?(emilio)

Oh I assume you mean this line here https://searchfox.org/mozilla-central/rev/5bea6ede57be43d450ecc24af7a535288c9a9f7d/layout/base/nsPresContext.cpp#994. Hmm, that's bad. I'm on mac, I don't think there's such setting so i hard-coded it 1.5, and on my normally 2 dpr device, it reports 3 which makes sense.

I think we can ignore it though by dividing LookAndFeel::SystemZoomSettings().mFullZoom, because we just spoof getters and not affect actual rendering.

Anyway, thank you!

so i hard-coded it 1.5

I do not follow this

Pushed by fkilic@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ee99a17f250 Make DPR use the correct zoom in RFP. r=fkilic

https://hg.mozilla.org/mozilla-central/rev/3ee99a17f250

Status: NEW → RESOLVED
Closed: 4 months ago
status-firefox139: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 139 Branch

The patch landed in nightly and beta is affected.
:pierov, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox138 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(pierov)

so far seems fixed, but needs more testing. As originally noted by myself after both Bug 1873382 and Bug 1929955, on my machine which has an actual devicePixelRatio of 1 and system scaling at 100%, I had some failures notably at 90% zoom and 110% zoom.

As you can see, devicePixelRatio hasn't changed, but everything else has (ignore the -webkit value because that's rounded down to a limited set in gecko). This is promising, because the only tests in gecko code is for dppx == devicePixelRatio. I actually added a max-resolution test for dppx as well, it passes. I also added some other tests for -webkit and dpi_css (and dpi_css failed, but now passes). Still trying to work out how to add tests for the other three (-moz, dpi, dpcm), it's not as straightforward as it seems

edit: using TZP /end edit

90% zoom: system scaling 100% with real DPR of 1 | with RFP

// FF138 FAIL
    "metrics": {
      "-moz-device-pixel-ratio": 1.7647058963775635,
      "-webkit-min-device-pixel-ratio": 1,
      "devicePixelRatio": 1.8181818181818181,
      "dpcm": 66.6975440625,
      "dpi": 169.41176601562498,
      "dpi_css": 169,
      "dppx": 1.7647059375,
    }
// FF139 PASS
    "metrics": {
      "-moz-device-pixel-ratio": 1.8181818723678589,
      "-webkit-min-device-pixel-ratio": 1,
      "devicePixelRatio": 1.8181818181818181,
      "dpcm": 68.71868507812502,
      "dpi": 174.54546,
      "dpi_css": 174,
      "dppx": 1.8181818750000003,
    }

110% zoom: system scaling 100% with real DPR of 1 | with RFP

// FF138 FAIL
    "metrics": {
      "-moz-device-pixel-ratio": 2.142857074737549,
      "-webkit-min-device-pixel-ratio": 2,
      "devicePixelRatio": 2.2222222222222223,
      "dpcm": 80.98987500000004,
      "dpi": 205.71428000000003,
      "dpi_css": 205,
      "dppx": 2.1428571875000006,
    }
// FF139 PASS
    "metrics": {
      "-moz-device-pixel-ratio": 2.222222328186035,
      "-webkit-min-device-pixel-ratio": 2,
      "devicePixelRatio": 2.2222222222222223,
      "dpcm": 83.98950570312502,
      "dpi": 213.33334000000008,
      "dpi_css": 213,
      "dppx": 2.22222234375,
    }
status-firefox138: fix-optional → wontfix
Flags: needinfo?(pierov)

(In reply to Thorin [:thorin] from comment #10)

so far seems fixed, but needs more testing. As originally noted by myself after both Bug 1873382 and Bug 1929955, on my machine which has an actual devicePixelRatio of 1 and system scaling at 100%, I had some failures notably at 90% zoom and 110% zoom.

As you can see, devicePixelRatio hasn't changed, but everything else has (ignore the -webkit value because that's rounded down to a limited set in gecko). This is promising, because the only tests in gecko code is for dppx == devicePixelRatio. I actually added a max-resolution test for dppx as well, it passes. I also added some other tests for -webkit and dpi_css (and dpi_css failed, but now passes). Still trying to work out how to add tests for the other three (-moz, dpi, dpcm), it's not as straightforward as it seems

Please open a new bug for this (and set this bug as see_also or depends_on).

As you can see, devicePixelRatio hasn't changed, but everything else has

That makes sense because we only changed the devicePixelRatio getter for CSS/Media Features. So, I would also expect window.devicePixelRatio to stay the same between 138 and 139 (assuming you don't have a systemwide text-scaling set)

the only tests in gecko code is for dppx == devicePixelRatio

We actually also added the following tests:

but yeah no tests for -webkit, -moz, dpcm etc.

That makes sense because

Yeah, I was just pointing out that all the media/matchmedia values changing was a good thing :) the devicePixelRatio side of the equation was never the problem :)

I filed Bug 1959729

QA Whiteboard: [qa-triage-done-c140/b139]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: