1741469 - Consider disabling mousewheel-triggered value changes for <input type="number"> (and "range")

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Daniel Holbert [:dholbert]]

STR:

1. View attached testcase. (Click to focus the input field if it's not already focused.)
2. Hover your mouse cursor above the input field.
3. Scroll your scrollwheel.

ACTUAL RESULTS:
The number value increases/decreases with your scrolling.

EXPECTED RESULTS:
Value should not change; scrolling shouldn't impact the value.

Notes:
This behavior is a feature that we added in bug 1261673, apparently in part for feature-parity with Chrome at the time. But I think we should reconsider & perhaps un-ship this behavior, because:
(1) I think we're the only browser that implements this behavior at this point (I'm actually not finding any old version of Chrome where I can see this behavior, either).
(2) This behavior creates a usability issue and is a somewhat unexpected/surprising footgun.

e.g. I just tripped over this when filling out a form at a website, which had an <input type="number"> field for my birth-year. I happened to mouse across that field when scrolling the page, and I inadvertently changed the contents of that field by a few years.

See also bug 1327816 comment 3 which has some examples of web developers needing/trying to work around this feature in the wild.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

(1) I think we're the only browser that implements this behavior at this point (I'm actually not finding any old version of Chrome where I can see this behavior, either).

Note: I tested Chrome 95 on Windows, and Chrome 97 dev on Linux, and also Chrome 55 on Windows 10 (from 2016, approximately when we added this feature), along with a few other random old Chrome versions; and I'm not seeing focused number-inputs responding to mousewheel scrolls in any of those versions.

Comment 3

[Daniel Holbert [:dholbert]]

This is all true of <input type="range"> as well -- we adjust the form-control's value when it's focused & hovered, if the user does a mousewheel-scroll action, but Chrome does not (nor does Safari). So the same removal argument applies there, too -- this is a footgun if the user happens to try to scroll a page while hovering this type of form-field.

Also notable: it appears Firefox does not change these form-fields' values for mousewheel-scroll events on MacOS -- only on Windows and Linux. I'm guessing that's because we observed (in bug 1261673 comment 19) that it was a Windows/Linux-specific feature in Chrome. (Though, as noted in my previous comment, I'm not actually seeing Chrome reproducing this behavior on any platform at this point.)

Comment 4

[Daniel Holbert [:dholbert]]

smaug, I'm curious for your thoughts here, since you were involved with the process to add this feature:
(1) do you know why I wouldn't be seeing this in current & old version of Chrome? (I'm wondering if there's some variable I'm missing; or perhaps if Chrome is just a bit more judicious about when they expose this behavior; or whether they did remove it entirely). And can you reproduce scroll-induced-number-changes in any Chrome versions at this point?

(2) Do you have any thoughts/concerns about potentially removing this behavior at this point? (assuming we're the only browser that does this)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

It is not that different to having a scrollable iframe. If mouse is over that, one can't scroll the top level page (assuming one can scroll the iframe).
I guess we should detect whether in middle of existing scroll action and not change the value of input elements in that case.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #5)

It is not that different to having a scrollable iframe. If mouse is over that, one can't scroll the top level page (assuming one can scroll the iframe).

page-scroll-failure is not my primary concern here -- my concern is more that user-entered data is accidentally modified by the inadvertent scroll operation, perhaps without the user noticing. I filed this because this actually happened to me (though fortunately I did notice before submitting the form).

I guess we should detect whether in middle of existing scroll action and not change the value of input elements in that case.

That doesn't fully address the use-case, unfortunately. My specific STR where I triggered this in the wild:
(1) Click an <input type="number"> near the bottom of the viewport (note, this leaves the cursor hovering over this field)
(2) Move your hands to the keyboard, and type in some numeric value.
(3) Move your hand back to the mouse, and mousewheel-scroll several times to proceed down the page to complete more fields.

In step (3) here, if you do several mousewheel-spins while also subtly moving the mouse (just from placing your hand on it), it's quite easy/possible for the first spin to inadvertently change the value that you just entered (since step 1 had left the cursor hovering the input field, so that's where it starts), and then for the subsequent mousewheel-spins to successfully scroll the page. This can mean you change the number you just entered without noticing (and also without it being "in the middle of an existing scroll action", hence my note that this suggested mitigation wouldn't necessarily help).

Given the fact that this is (I think?) a Firefox-only behavior at this point, and the potential for user harm via accidental corruption of entered data, is there a strong reason that we want to keep it? (Do we know why/when other browsers unsupported it? Or do they still support it in a way that I haven't yet discovered?)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (minimal representation of the page where I triggered this bug)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: screencast of this causing issues w/ testcase 2 (with scroll operation that happens to start while mouse is still over the number field)

(In reply to Daniel Holbert [:dholbert] from comment #6)

[...] if you do several mousewheel-spins while also subtly moving the mouse (just from placing your hand on it), it's quite easy/possible for the first spin to inadvertently change the value that you just entered [...] and then for the subsequent mousewheel-spins to successfully scroll the page. This can mean you change the number you just entered without noticing.

Here's a screencast to demonstrate this, with testcase 2. At 0:05, I start some repeated-scrolls of my scrollwheel, while inadvertently moving the mouse. Within ~1 second, the page is indeed scrolling, so it's easy to miss the fact that the very start of the scroll operation also modified the value that I entered.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

It is still very similar to iframe case. One could move mouse from iframe to the top level page.

I don't feel too strongly about this, but I do think the current behavior is quite nice and yet I do see how it can lead to problems.

But is this behavior enabled on Linux only? For some reason I can't see the behavior on Windows, only Linux, yet the code should be enabled in both cases. ....ah, I guess it is about delta mode.
On Linux the behavior is quite close to the platform convention. On Gnome settings for example if one scrolls over a range, that gets easily changed too.

We could add initially some pref for this and disable it by default, and if we get lots of complains, revert that change.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug] from comment #9)

It is still very similar to iframe case. One could move mouse from iframe to the top level page.

It's functionally similar, but the user impact is quite different:

• Accidentally scrolling an iframe by a few ticks (without noticing) is unlikely to be problematic.
• Accidentally modifying the contents of a number input frame (without noticing) is much more likely to be problematic.

On Linux the behavior is quite close to the platform convention.

True; though note that we've moved away from having native look and feel for form-fields, so there's less of an intuition that our form-fields should behave precisely like the native ones, and more of an intuition that they should behave the same across platforms.

We could add initially some pref for this and disable it by default, and if we get lots of complains, revert that change.

That sounds like a good way forward to me.

Comment 11

[Šime Vidas]

Relevant article ”Number Inputs Can Be Annoying” https://falkus.co/2023/01/number-input-hassle

Unless you suspect that there exist Firefox users who actually change number input values via mouse wheel and find that useful, I suggest that you remove this functionality immediately at priority 1. Even if such people exist, the number of users for whom this happens unintentionally and by accident is probably higher at a ratio of 100 to 1, so it’s not worth it.

Comment 12

[Theodore]

+1 on removing this "feature". We've been dealing with support tickets caused by users accidentally submitting wrong amounts due to this unexpected behavior.

Comment 13

[Daniel Holbert [:dholbert]]

I'll tentatively take this & see if I can just put this behavior behind an off-by-default pref, per comment 9 - comment 10.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: Bug 1741469 part 3: Add an off-by-default pref to control whether input type="number"/"range" fields are modified when mousewheel-scrolled. r?smaug

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: Bug 1741469 part 1: Make some cleanups and readability edits to test_bug1261673.html. r?smaug

The next patch in this series will extend this test a bit. To make that change
(as well as this test in general) easier to understand, I'm doing a bit of
initial tweaks/cleanup.

Notable changes:

• Switched to "is(a,b)" instead of "ok(a == b, 'expect a get b)".
• Renamed (and added brief documentation for) variales 'result',
  'numberChange', and 'expectChange' to make their meanings/usage clearer.
• Expanded out some one-liner ternary-logic expressions, both for readability
  and because I'm going to be extending them with more cases in the next patch.

Comment 16

[Daniel Holbert [:dholbert]]

Green try run: https://treeherder.mozilla.org/jobs?repo=try&revision=71370fbf1ea42803b99245add468f0516bd36f86

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a75f2a426fa3
part 1: Make some cleanups and readability edits to test_bug1261673.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b007c139e65c
part 2: Add an off-by-default pref to control whether input type="number" fields are modified when mousewheel-scrolled. r=smaug

Comment 18

[Atila Butkovits]

Backed out for causing failures at test_bug1261674-1.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/98f132cebd625e9b0224690010f1218d85573c0b

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=IaOQmK1UQNSpBrF_0ObcKw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitests%2Cwith%2Csoftware%2Cwebrender%2Cwithout%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-plain-swr-nofis%2C2&revision=b007c139e65cbb5c81643aea369931e16ecab9f2

Failure log: https://treeherder.mozilla.org/logviewer?job_id=413175605&repo=autoland&lineNumber=3658

Comment 19

[Daniel Holbert [:dholbert]]

Ah, that test needs the same changes that I made to test_bug1261673.html. (Or at least an explicit pref-flip.)

I'm on PTO for a few days but I'll take care of that when I'm back.

(Note: my "Green try run" in comment 16 was just for that one modified test; I thought/hoped that was the only test that depended on this behavior, but I should've known better. :) )

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:dholbert, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 21

[Bernat]

I'm currently trying to fix this with javascript in my forms so users don't mess up their work. I would really like to be able to patch Firefox but I can't. This is a real footgun with some victims already.

Comment 22

[Bernat]

Here's a fix using Javascript:

    document.querySelectorAll("input[type=number]").forEach(function (element) {
        element.addEventListener("wheel", function(event) {
            if (document.activeElement === event.target) {
                event.preventDefault();
            }
        });
    });

Comment 23

[benroussin]

I can attest TODAY, that this "feature" is very bad. My client yelled at me for an hour saying that the (very important for him) values changes when he saves the form, and it took me ages to understand that the browser he uses i.e. firefox, behaves differently than mine...

Inconsitent behaviour is a killer of usability. I advised him to switch browser because of this. Having to write javascript queries to solve a basic need for consistency is ridiculous.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: Bug 1741469 part 2: Make some cleanups and readability edits to test_bug1261674-1.html. r?smaug

Comment 25

[Daniel Holbert [:dholbert]]

Thanks for the nudge. This has been in the back of my mind to get landed - it was blocked on one test that needed to be updated to account for the behavior-removal (comment 18), but I've done that now and am resubmitting patches here.

Comment 26

[Daniel Holbert [:dholbert]]

Try run, for all mochitest-* tasks:
https://treeherder.mozilla.org/jobs?repo=try&revision=2b97aa2347d4213d6f67e5a99e3b50c8e9889999

Lots of orange, but I think it's all intermittent, at first glance; none of it seems to be associated with the changes here (and I'm not seeing cases of the same task/test failing across many configurations).

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad1ba2561fa2
part 1: Make some cleanups and readability edits to test_bug1261673.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/942ec1986c35
part 2: Make some cleanups and readability edits to test_bug1261674-1.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e596940bc7d8
part 3: Add an off-by-default pref to control whether input type="number"/"range" fields are modified when mousewheel-scrolled. r=smaug

Comment 28

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ad1ba2561fa2
https://hg.mozilla.org/mozilla-central/rev/942ec1986c35
https://hg.mozilla.org/mozilla-central/rev/e596940bc7d8

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: Bug 1741469 part 1: Make some cleanups and readability edits to test_bug1261673.html. r?smaug

The next patch in this series will extend this test a bit. To make that change
easier to understand (and make the test in general easier to follow), I'm doing
a bit of initial tweaks/cleanup.

Notable changes:

• Switched to "is(a,b)" instead of "ok(a == b, 'expect a get b)".
• Renamed (and added brief documentation for) variables 'result',
  'numberChange', and 'expectChange' to make their meanings/usage clearer.
• Expanded out some one-liner ternary-logic expressions, both for readability
  and because I'm going to be extending them with more cases in the next patch.
• Removed unnecessary parseInt() (p.valueChanged is already an integer; no
  parsing needed) but added another (wrapping input.value, so that is() doesn't
  spuriously fail due to integer vs. string comparison).

Original Revision: https://phabricator.services.mozilla.com/D175971

Comment 30

[Daniel Holbert [:dholbert]]

Attachment: Bug 1741469 part 2: Make some cleanups and readability edits to test_bug1261674-1.html. r?smaug

Original Revision: https://phabricator.services.mozilla.com/D216497

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: Bug 1741469 part 3: Add an off-by-default pref to control whether input type="number"/"range" fields are modified when mousewheel-scrolled. r?smaug

The C++ code change here is simply replacing an ifdef with a pref-check (and
associated reindentation for being placed in an "if"-block).

The pref is off-by-default for now, since no other browser seems to modify
these fields through mousewheel scrolling, and users seem to be surprised when
they encounter the behavior in Firefox. See discussion on the bugzilla page.

Note: now that this behavior is controlled by a pref, we don't need to have
platform-specific #ifdefs and "skip-if" test exemptions. If we decide to bring
this behavior back on certain platforms, we can do so by simply changing the
default value of the pref on those platforms.

Original Revision: https://phabricator.services.mozilla.com/D175719

Comment 34

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: inadvertent unwanted edits to form fields (input and ranges) if the user happens to hover over them when wheel-scrolling
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: See https://bugzilla.mozilla.org/show_bug.cgi?id=1741469#c0
• Risk associated with taking this patch: Low
• Explanation of risk level: this patch is just adding a pref-check around some existing platform code (the code that modifies number fields when they're scrolled across). That code was already off-by-default on several platforms (mac/Android), via an #ifdef rather than a pref-check, so that's a good indication that it's safe to skip over. In the unlikely event that we change our mind (or users want to reenable this behavior themselves), then a pref-flip is all that's needed.
• String changes made/needed: None
• Is Android affected?: no

Comment 35

[Daniel Holbert [:dholbert]]

I verified the fix myself in Nightly with a fresh profile, FWIW:
Nightly 2024-07-16 gives ACTUAL RESULTS.
Nightly 2024-07-17 gives EXPECTED RESULTS.

And I'm aware that D217523 doesn't graft cleanly to ESR128 right now; I'm cloning as-we-speak and then will rebase and update the phab-revision.

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #35)

And I'm aware that D217523 doesn't graft cleanly to ESR128 right now; I'm cloning as-we-speak and then will rebase and update the phab-revision.

OK, I've just rebased the ESR stack so that it applies cleanly on ESR128 now. (moz-phab patch D217523 --apply-to 8ab8cee6727c now completes successfully, in a local clone of ESR128.)

Comment 37

[Donal Meehan [:dmeehan]]

This should be uplifted for 128.2esr that ships alongside Fx130

Comment 38

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/133d1557016e
https://hg.mozilla.org/releases/mozilla-esr128/rev/d526c54220a3
https://hg.mozilla.org/releases/mozilla-esr128/rev/7e3d2a20fa87

Comment 39

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue described in comment 0 using an old Nightly build before the fix, verified that using latest Firefox 128.2.0esr build this is fixed across platforms (Windows 11, Ubuntu 22.04 and macOS 13.6).