1912371 - general.smoothScroll value is clobbered by changing the corresponding the system setting

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

While reading nsLayoutUtils::RecomputeSmoothScrollDefault code, I realized there's a scenario that general.smoothScroll pref value the user did set is clobbered by the system setting.

The scenario can be observed by the following step;

1. Make the general.smoothScroll value be not default, i.e. making the preference entry bold in about:config
2. Flip the corresponding system setting
3. Restart firefox
4. Check the preference value in about:config, the value hasn't yet changed, but it's no longer bold
5. Flip the corresponding system setting again
6. Restart firefox

Comment 1

[fireattack [:fireattack]]

Is there any update on this? It has been more than half an year and I had to manually set this setting everyday (because I use remote desktop daily), which is annoying, to say at at least.

Comment 2

[Botond Ballo [:botond]]

(In reply to fireattack [:fireattack] from comment #1)

Is there any update on this? It has been more than half an year and I had to manually set this setting everyday (because I use remote desktop daily), which is annoying, to say at at least.

Apologies for this annoying regression. I've raised this bug / bug 1928161 with our team to get it onto our agenda (tracked internally in FFXP-2743) and I'm hoping this is something we can get to early in the year.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1912371 - Use nsLayoutUtils::IsSmoothScrollingEnabled rather than directly using general_smoothScroll(). r?stransky,emilio

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1912371 - Make `general.smoothScroll` sticky. r?#settings-reviewers!,#geckoview-reviewers,botond,dlrobertson

With this new pref, even if the system's prefers-reduced-mottion setting
is changed for whatever reason, the new pref persists, thus smooth
scrolling is never clobbered by the system setting.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Though I've posted a patch (D253106) to fix this clobbering bug by introducing a new pref, I am open to any other ideas to solve this bug.

The other idea I can think of is to decouple "general.smoothScroll" from the preferes-reduced-motion setting, which means backing out of bug 1753565.

Comment 6

[fireattack [:fireattack]]

I think the root issue of this bug is that changing system setting would cause user-modified general.smoothScroll value to no longer be seen as "modified" (#4 in your step), and I assume this is because this setting's default value is dynamically decided based on the system reduce motion setting, so toggling system setting back and forth will cause its "(non-)default-ness" state to be lost eventually.

I personally think a better solution is to have the default value of general.smoothScroll to be something like "Not set", instead of a dynamic value of True/False that is linked to system setting.

The behavior of scrolling itself, NOT the setting value, should dynamically follow the system setting if the value is "not set". And once actually set by user, it stays that way.

Pseudo code:

If general.smoothScroll == "Not set":
  smoothScrollBehavior = !system.reduce_motion_setting
else:
   smoothScrollBehavior = general.smoothScroll

I don't know if a pref setting allows it, though.

Comment 7

[fireattack [:fireattack]]

/The behavior of scrolling itself, NOT the setting value/The behavior of scrolling itself, NOT the setting value or setting default/s

Comment 8

[Hiroyuki Ikezoe (:hiro)]

The new pref named general.smoothScroll.user is what you mean by "Not set" initially.

Comment 9

[fireattack [:fireattack]]

Oh yeah I get it, I just think it could be simpler without introducing another pref. But again, as long as it works, I’ve got no objections.

Comment 10

[Masatoshi Kimura [:emk]]

Usually we use three state pref for that. For example, 0=false, 1=true, -1=auto (Not Set). Or 0=false, 1=true, 2=auto.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1912371 - Set `mIsSticky` outside of `if (!ValueMatches())` block in SetDefaultValue. r?nika

So that sticky bit works for static prefs.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/abb255f37964
https://hg.mozilla.org/integration/autoland/rev/2077c9ac7d87
Use nsLayoutUtils::IsSmoothScrollingEnabled rather than directly using general_smoothScroll(). r=emilio
https://github.com/mozilla-firefox/firefox/commit/c48a79d387fb
https://hg.mozilla.org/integration/autoland/rev/08ded652dbec
Set `mIsSticky` outside of `if (!ValueMatches())` block in SetDefaultValue. r=glandium
https://github.com/mozilla-firefox/firefox/commit/b13ef9518ee2
https://hg.mozilla.org/integration/autoland/rev/0a0f2712cb7b
Make `general.smoothScroll` sticky. r=dlrobertson,settings-reviewers,emilio,Gijs

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/2077c9ac7d87
https://hg.mozilla.org/mozilla-central/rev/08ded652dbec
https://hg.mozilla.org/mozilla-central/rev/0a0f2712cb7b