1261673 - Scroll wheel does not increment/decrement value of type="number" fields

Status: RESOLVED FIXED

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

Description

[Stephan Sokolow]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160329042710

Steps to reproduce:

1. Visit a page with a type="number" field like http://robertnyman.com/html5/forms/input-types.html
2. Hold the cursor over the field and move the scroll wheel
3. Click the field to focus it and then hold it over the field and move the scroll wheel


Actual results:

In both cases, the scroll wheel scrolls the page


Expected results:

GtkSpinButton and QSpinBox controls increment/decrement in response to scroll wheel events whether or not they are focused.

(basically, the same behaviour as Firefox uses for scrolling within iframes).

Firefox should aim to match the native widget behaviour.

Comment 1

[Andrew Overholt [:overholt]]

Are these toolkit-specific widgets?

Comment 2

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

Nope. This is deep in DOM/Layout

Comment 3

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

See also Bug 1261674.

Comment 4

[Jonathan Watt [:jwatt]]

What info do you need here, Olli? This is simply a case of adding something in PostHandleEvent, or something, right?

Comment 5

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

Well, I was wondering if you want to take this bug ;)
But yes, something could be added to PostHandleEvent or relevant nsIFrame::HandleEvent. Probably PostHandleEvent.

Comment 6

[Jonathan Watt [:jwatt]]

I've got a bunch of other stuff I'm prioritizing right now. Just finished basic shape clipping and will be back on secure contexts now. I don't think I'll get to this any time soon.

Comment 7

[Ming-Chou Shih [:stone]]

I would like to try on this bug.

Comment 8

[Ming-Chou Shih [:stone]]

Experiments on different platforms and browsers
          Edge   Chrome    Firefox   Safari
Windows10 X      #3        X         NA
Ubuntu    NA     #3        X         NA
Mac       NA     X         X         X

X means the browser doesn't support test step #2 and #3
#3 means the browser supports test step #3

Looks like only Chrome supports the test step #3 on Ubuntu and Windows.

Comment 9

[Ming-Chou Shih [:stone]]

I tried the GtkSpinButton and a Qt5 example app, the values of GUI controls are increased/decreased when the mouse cursor is over on them and scrolling the mouse wheel. Looks like the behavior is unrelated to whether the control was focused. So I think this bug is about handling the mouse wheel events in the element which is found by hit test. Is my understanding correct?

Comment 10

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

Sounds right. wheel scrolling in general doesn't depend on focus, but hit testing.
So, one way to fix this would be to handle (trusted) wheel event in HTMLInputElement::PostHandleEvent if prevent default hasn't been called, and then call PreventDefault(true) on the widget event to ensure other default handling won't happen.

Comment 11

[Ming-Chou Shih [:stone]]

I wrote a patch to handle wheel events in HTMLInputElement::PostHandleEvent but it only worked in non-e10s. In e10s, wheel events are handled by APZ before dispatching to DOM elements. Then the page is scrolled no matter wheel events are prevented by DOM elements or not.

I tried to pass wheel events as unconfirmed target (=false) in [1] to let DOM elements have a chance to prevent wheel events when necessary. But it breaks a gtest [2]. Could you kindly give me some suggestions about how to correctly let APZ not to handle wheel events when they are prevented by DOM elements? Thanks.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?from=APZCTreeManager.cpp#767
[2] APZHitTestingTester.TestRepaintFlushOnWheelEvents

Comment 12

[Ming-Chou Shih [:stone]]

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?from=APZCTreeManager.cpp#767
>        result = mInputQueue->ReceiveInputEvent(
>          apzc,
>          /* aTargetConfirmed = */ hitResult == HitLayer,
           ^ Always set aTargetConfirmed as false.
>          wheelInput, aOutInputBlockId);

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Most likely you need to add the DOM element to the dispatch-to-content region, so that APZC knows that it *might* do a preventDefault. If you explicitly register a wheel listener in the HTMLInputElement this should happen automatically. The slider frame does a similar thing with touch events, see http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsSliderFrame.cpp#1133 for example.

Comment 14

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

Adding a system group listener should work indeed, assuming APZC cares about system group listeners and not only default group.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Just to describe the machinery in a bit more detail, adding a listener should get picked up by the code at [1]. That will make the area occupied by the element be added to mDispatchToContentHitRegion, and that gets propagated by the layers over to APZ. Then, during APZ hit test [2] that should return HitDispatchToContentRegion as the hit result, which will result in "false" being passed because hitResult != HitLayer at [3]. That's what you had hard-coded and found to work, but this would be the proper way of doing it.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=2ccaac8616e4#3554
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=2ccaac8616e4#1755
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=2ccaac8616e4#769

Comment 16

[Ming-Chou Shih [:stone]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Just to describe the machinery in a bit more detail, adding a listener
> should get picked up by the code at [1]. That will make the area occupied by
> the element be added to mDispatchToContentHitRegion, and that gets
> propagated by the layers over to APZ. Then, during APZ hit test [2] that
> should return HitDispatchToContentRegion as the hit result, which will
> result in "false" being passed because hitResult != HitLayer at [3]. That's
> what you had hard-coded and found to work, but this would be the proper way
> of doing it.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp?rev=2ccaac8616e4#3554
> [2]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> APZCTreeManager.cpp?rev=2ccaac8616e4#1755
> [3]
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/
> APZCTreeManager.cpp?rev=2ccaac8616e4#769

Thanks for the detailed information.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value.

Comment 18

[Ming-Chou Shih [:stone]]

This patch includes following modifications
1. Add a function to add/remove a system event listener to wheel events. The system event listener is added when the input type is 'number' and removed when the type is changed to other types. This is for automatically adding the element to the dispatch-to-content region so that APZC knows that the element might do a preventDefault to wheel event.

2. Update the system event listener when the input element is constructed, destructed, and type changed.

3. Handle wheel events in PostHandleEvent as increasing / decreasing the value of input element then do a preventDefault. This behavior is only applied when the input element is focused and mutable, and the event is trusted.

I'm not sure if it's necessary to remove/add the system listener when the element is blurred/focused. Please let me know if anything should be taken into considerations. Thanks.

Comment 19

[Ming-Chou Shih [:stone]]

Not sure why Chrome doesn't implement the behavior on Mac. Should we implement this behavior on all (Mac, Ubuntu, Windows) platforms?

Comment 20

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

Comment on attachment 8752661 [details] [diff] [review]
Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value.


>+class HTMLInputElement::WheelEventListener final : public nsIDOMEventListener
>+{
>+  ~WheelEventListener() {}
>+public:
>+  explicit WheelEventListener() {}
>+  NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent) override { return NS_OK; }
Ah, you use this listener just to trigger PostHandleEvent. Fine. Interesting case. I think we may want to add some helper for this.

>+void HTMLInputElement::UpdateWheelEventListener(bool aForceRemove)
>+{
>+  bool listenWheelEvents = (mType == NS_FORM_INPUT_NUMBER);
>+  if (!aForceRemove && listenWheelEvents && !mWheelListener) {
>+    mWheelListener = new WheelEventListener();
>+    AddSystemEventListener(NS_LITERAL_STRING("wheel"), mWheelListener, false,
>+                           false);
>+  }
>+  else if ((aForceRemove || !listenWheelEvents) && mWheelListener) {
nit, 
} else if (...) {


>+        case eWheel: {
>+          // Handle wheel events as increasing / decreasing the input element's
>+          // value when it's focused and it's type is number.
>+          WidgetWheelEvent* wheelEvent = aVisitor.mEvent->AsWheelEvent();
>+          if (!aVisitor.mEvent->DefaultPrevented() &&
>+              aVisitor.mEvent->IsTrusted() && IsMutable() && wheelEvent &&
>+              wheelEvent->mDeltaY != 0) {
>+            if (mType == NS_FORM_INPUT_NUMBER) {
I would probably move mType == NS_FORM_INPUT_NUMBER check to the 'if' above

>+              nsNumberControlFrame* numberControlFrame =
>+                do_QueryFrame(GetPrimaryFrame());
>+              if (numberControlFrame && numberControlFrame->IsFocused()) {
Hmm, do we need the control to be focused? I don't think so. Just getting event should be enough, no?

>+                StepNumberControlForUserEvent(wheelEvent->mDeltaY > 0 ? -1 : 1);
I think if deltaMode is pixel, very small changes shouldn't cause any step up/down.

>+  class WheelEventListener;
>+  RefPtr<WheelEventListener> mWheelListener;
I'd prefer if we stored the listener as a property. That way we don't increase the size of the HTMLInputElement for all the <input> elements, but just in case type is number.
nsINode::SetProperty
As an example, nsXBLService::AttachGlobalKeyHandler for example sets a property. You probably want to add a new nsGkAtom to nsGkAtomsList.h and use that as the name of the property.


Looking good, but some tweaks, and then some tests too, please?

Comment 21

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

I wonder, could we add a virtual method to EventTarget, say "IsApzAware", and then
EventTarget::HasApzAwareListeners() could call that.
Then HTMLInputElement would override that IsApzAware().
That way there wasn't need for dummy event listeners.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Wouldn't that virtual function get hit a lot? If the perf of such a function isn't a concern then I don't have a problem with it.

Comment 23

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8752661 [details] [diff] [review]
> >+              nsNumberControlFrame* numberControlFrame =
> >+                do_QueryFrame(GetPrimaryFrame());
> >+              if (numberControlFrame && numberControlFrame->IsFocused()) {
> Hmm, do we need the control to be focused? I don't think so. Just getting
> event should be enough, no?

Chrome only changes input element's value when the element is focused.
Qt (on Ubuntu) changes input element's value no matter it's focused or not.
GTK (on Ubuntu) changes input element's value and let the element be focused.
I also try the windows controls (on Windows 10) and input element's values are only changed when they are focused.
It's ok to me to follow the behavior of Chrome, Qt, or GTK.

And I guess this function is only enabled on Windows and Linux/Unix, right?

Comment 24

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

oh, ok, if platforms tend to rely on focus-ness, then fine to rely on that here too.

Comment 25

[Ming-Chou Shih [:stone]]

Attachment: Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value.

Comment 26

[Ming-Chou Shih [:stone]]

Refine the patch as
1 rename MayHaveApzAwareListeners to MayBeApzAware to represent a node is apz aware.
- It is set when apz aware listeners are added or a HTMLInputElement with number type is created
- Check this flag to know if the node may be apz aware

2 add non-virtual member nsINode::IsApzAware { return NodeMayBeApzAware()?IsApzAwareInternal():false; }
- Let nsFrame call non-virtual function 'IsApzAware'
- This will check flag first and invoke virtual function 'IsApzAwareInternal' when necessary

3 let IsApzAwareInternal() check if there are apz aware listeners as before
- let HTMLInputElement override it and return true to tell APZC it's apz aware node
- Otherwise, check if there are apz aware listeners

4 Handle wheel event in HTMLInputElement as increase/decease its value

Comment 27

[Ming-Chou Shih [:stone]]

Attachment: Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value

Comment 28

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

Comment on attachment 8766194 [details] [diff] [review]
Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value

> nsINode::HasApzAwareListeners() const
> {
>-  if (NodeMayHaveApzAwareListeners()) {
>-    return EventTarget::HasApzAwareListeners();
>-  }
>-  return false;
>+  return IsApzAware();
> }
ah, right we need to do it this case to keep layoututils happy.
Hmm, could we perhaps rename HasApzAwareListeners() everywhere (including EventTarget.h/cpp) to IsApzAware()
and then rename the IsApzAware() which this patch adds IsNodeApzAware().
IsApzAware() would then be virtual, just like HasApzAwareListeners() is now, and
IsNodeApzAware() would be non-virtual.
Also rename IsApzAwareInternal to IsNodeApzAwareInternal()

>+#if defined(XP_WIN) || defined(XP_LINUX)
>+        case eWheel: {
>+          // Handle wheel events as increasing / decreasing the input element's
>+          // value when it's focused and it's type is number.
>+          WidgetWheelEvent* wheelEvent = aVisitor.mEvent->AsWheelEvent();
>+          if (!aVisitor.mEvent->DefaultPrevented() &&
>+              aVisitor.mEvent->IsTrusted() && IsMutable() && wheelEvent &&
>+              wheelEvent->mDeltaY != 0 &&
>+              wheelEvent->mDeltaMode != nsIDOMWheelEvent::DOM_DELTA_PIXEL &&
>+              mType == NS_FORM_INPUT_NUMBER) {
why wheelEvent->mDeltaMode != nsIDOMWheelEvent::DOM_DELTA_PIXEL ?
Shouldn't we handle also large delta scrolling. But I guess we can tweak in a separate patch.

Comment 29

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #28)
> ah, right we need to do it this case to keep layoututils happy.
> Hmm, could we perhaps rename HasApzAwareListeners() everywhere (including
> EventTarget.h/cpp) to IsApzAware()
> and then rename the IsApzAware() which this patch adds IsNodeApzAware().
> IsApzAware() would then be virtual, just like HasApzAwareListeners() is now,
> and
> IsNodeApzAware() would be non-virtual.
> Also rename IsApzAwareInternal to IsNodeApzAwareInternal()
OK

> >+#if defined(XP_WIN) || defined(XP_LINUX)
> >+        case eWheel: {
> >+          // Handle wheel events as increasing / decreasing the input element's
> >+          // value when it's focused and it's type is number.
> >+          WidgetWheelEvent* wheelEvent = aVisitor.mEvent->AsWheelEvent();
> >+          if (!aVisitor.mEvent->DefaultPrevented() &&
> >+              aVisitor.mEvent->IsTrusted() && IsMutable() && wheelEvent &&
> >+              wheelEvent->mDeltaY != 0 &&
> >+              wheelEvent->mDeltaMode != nsIDOMWheelEvent::DOM_DELTA_PIXEL &&
> >+              mType == NS_FORM_INPUT_NUMBER) {
> why wheelEvent->mDeltaMode != nsIDOMWheelEvent::DOM_DELTA_PIXEL ?
> Shouldn't we handle also large delta scrolling. But I guess we can tweak in
> a separate patch.
Actually I am not well understand the difference between DOM_DELTA_PIXEL/DOM_DELTA_LINE/DOM_DELTA_PAGE. Is it always true that the changes of delta pixel < delta line < delta page?

Comment 30

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

it is more about the input device type and settins.
 _PAGE is very rare. I think it happens only on Windows if user explicitly sets system level setting to scroll-pages or so.
deltaMode of the event tells what kind of changes the deltaX/Y/Z is about

Comment 31

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

https://w3c.github.io/uievents/#events-wheelevents

Comment 32

[Ming-Chou Shih [:stone]]

Attachment: Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value. r=smaug

Follow reviewer's comments to refine patch and add review in patch summary

Comment 33

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2011336d5b5b1a1b2743a993d2734c6947ed8b7

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8b565fe625
Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value. r=smaug

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a8b565fe625

Comment 37

[Daniel Holbert [:dholbert]]

For what it's worth, I landed a patch in bug 1741469 to turn off this behavior in Nightly (and eventually release when it rides the trains), since we got multiple reports of it introducing unwanted modifications to inadvertently-scrolled-over form fields.