Closed Bug 1832598 Opened 2 years ago Closed 2 years ago

Smooth scrolls are disabled if the user prefers-reduced-motion regardless of fingerprint resistance

Categories

(Core :: Layout: Scrolling and Overflow, defect)

defect

Tracking

()

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: dlrobertson, Assigned: dlrobertson)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Summary

We do not check if we should apply fingerprint resistance before disabling smooth scrolls. Currently if the users system settings indicate prefers-reduced-motion, we will not respect smooth scrolls by default and instead perform instant scrolls.

Steps to reproduce

  1. Set prefers reduced motion or set general.smoothScroll to false
  2. Set privacy.resistFingerprinting to true
  3. Navigate to https://dlrobertson.com/examples/scrollinto-view-scrollend.html

Actual Results:

Both scrolls run in the script are instant

Expected Results:

Regardless of the value of prefers-reduced-motion or general.smoothScroll, we should smooth scroll for the second programmatic scroll.

Flags: needinfo?(tom)
Assignee: nobody → drobertson
Severity: -- → S3

There will likely be two parts to this fix:

  1. The easier part: nsLayoutUtils::IsSmoothScrollingEnabled() should be updated to check Document::ShouldResistFingerprinting on the PresContext()->Document()
  2. The harder part: APZ does not have a reference to the Document, so we'll need to pass through some extra data through to APZ. I think an extra bit added to ScrollMetadata that indicates "Should resist fingerprinting" would suffice. Then all checks for general_smoothScroll in APZ could be updated to check the value of the preference and the value of this new bit.

I would propose a more targeted fix for this.

Firstly: it seems odd to me that we would have two different scroll behaviors depending on what page you are on. Consider someone who has enabled PrefersReducedMotion on the OS, which AFAIK is not the default. On about:newtab they would get instant scrolling, and on example.com they'd be smooth scrolling. That's weird. Instead, I think the scroll behavior should be uniform for the entire browser. This means we don't need to check the PresContext's RFP behavior, or plumb anything through in APZ.

Secondly: It's not RFP's job to override every pref a user can set. We do override a few of them, like user agent overrides, but it's infeasible to try to override every single one a user could change that would alter their fingerprint. So we don't need to override general.smoothScroll - we just need to make sure that the OS preference doesn't leak through in it.

Therefore my proposal would be to change this code here to something like this:

if(nsContentUtils::ShouldResistFingerprinting(
    RFPTarget::SmoothScroll, "We use the global RFP pref to maintain consistent scroll behavior in the browser.")) {
  Preferences::SetBool(
      "general.smoothScroll", true, PrefValueKind::Default);
} else {
  // We want prefers-reduced-motion to determine the default
  // value of the general.smoothScroll pref. If the user
  // changed the pref we want to respect the change.
  Preferences::SetBool(
      "general.smoothScroll",
      !LookAndFeel::GetInt(LookAndFeel::IntID::PrefersReducedMotion, 0),
      PrefValueKind::Default);
}

Flags: needinfo?(tom)

Thanks for the info! That makes sense, and should be much more maintainable

When resist fingerprinting is set, ensure that general.smoothScroll has
a resonable default set.

(In reply to Tom Ritter [:tjr] from comment #2)

Secondly: It's not RFP's job to override every pref a user can set. We do override a few of them, like user agent overrides, but it's infeasible to try to override every single one a user could change that would alter their fingerprint. So we don't need to override general.smoothScroll - we just need to make sure that the OS preference doesn't leak through in it.

+1 on this -- it should be possible for a user who prefers instant scrolling but otherwise wants to reduce the rest of their fingerprint, to have both prefs customized.

Pushed by drobertson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e65029dd25d0 Default enable smooth scrolls when resisting fingerprinting. r=tjr,botond

Backed out for causing modules/libpref failures on e.g. test_stickyprefs.js

[task 2023-05-13T13:04:14.203Z] 13:04:14     INFO -  TEST-PASS | js/xpconnect/tests/unit/test_isModuleLoaded.js | took 855ms
[task 2023-05-13T13:04:14.282Z] 13:04:14     INFO -  Cleaning up profile for /builds/worker/workspace/build/tests/xpcshell/tests/js/xpconnect/tests/unit/test_isModuleLoaded.js folder: /data/local/tmp/test_root/xpc/p/aba2bd5c-7788-4db8-a143-129af21b59d6
[task 2023-05-13T13:04:14.899Z] 13:04:14     INFO -  TEST-START | modules/libpref/test/unit/test_stickyprefs.js
[task 2023-05-13T13:04:15.189Z] 13:04:15     INFO -  remotexpcshelltests.py | modules/libpref/test/unit/test_stickyprefs.js | 3566 | Launched Test App
[task 2023-05-13T13:04:16.799Z] 13:04:16     INFO -  remotexpcshelltests.py | modules/libpref/test/unit/test_stickyprefs.js | 3566 | Application ran for: 0:00:01.899795
[task 2023-05-13T13:04:16.869Z] 13:04:16  WARNING -  TEST-UNEXPECTED-FAIL | modules/libpref/test/unit/test_stickyprefs.js | xpcshell return code: 0
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  TEST-INFO took 1969ms
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  >>>>>>>
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  running event loop
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  modules/libpref/test/unit/test_stickyprefs.js | Starting notWrittenWhenUnchanged
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  (xpcshell/head.js) | test notWrittenWhenUnchanged pending (2)
[task 2023-05-13T13:04:16.869Z] 13:04:16     INFO -  TEST-PASS | modules/libpref/test/unit/test_stickyprefs.js | notWrittenWhenUnchanged - [notWrittenWhenUnchanged : 41] true === true
Flags: needinfo?(drobertson)

Having trouble with the bit that changes the smooth scrolling default as soon as privacy.reducedFingerprinting is set (or unset). I think I'm going to remove that portion from the patch and file a follow-up bug for that component

Flags: needinfo?(drobertson)
Blocks: 1834307
Pushed by drobertson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/39fcdc77000c Default enable smooth scrolls when resisting fingerprinting. r=tjr,botond
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
QA Whiteboard: [qa-115b-p2]

I can still reproduce this issue on Firefox 115.0 and latest Nightly on Ubuntu and Windows, but not on macOS. Am I right to assume that bug 1834307 will address the fix on Windows/Ubuntu ?

Flags: needinfo?(drobertson)

(In reply to Ardelean Oana, Desktop QA from comment #11)

I can still reproduce this issue on Firefox 115.0 and latest Nightly on Ubuntu and Windows, but not on macOS. Am I right to assume that bug 1834307 will address the fix on Windows/Ubuntu ?

You should need to restart before the change should go into effect. What are the STR you're following and what are you seeing?

Flags: needinfo?(drobertson)
Flags: needinfo?(oardelean)
Attached file win

I followed the STR from Comment 0 and restarted the browser after applying the preferences.

Flags: needinfo?(oardelean)

The previous attachment might be obsolete therefore I am uploading another screen capture. Please let me know if there's anything else I can do to help, thank you.

(In reply to Ardelean Oana, Desktop QA from comment #13)

I followed the STR from Comment 0 and restarted the browser after applying the preferences.

Is this done with a fresh profile with no prior preferences set? privacy.resistFingerprinting really only impacts the default. It shouldn't override a users configuration (see bug 1832598 comment 2).

See Also: → 1928570
See Also: → 1937881
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: