Closed Bug 1261673 Opened 8 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
47 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: from_bugzilla3, Assigned: stone)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(1 file, 3 obsolete files)

Handle wheel event when mouse cursor is hovered on a focused number input as increasing/decreasing it's value. r=smaug
18.13 KB, patch
stone
: review+
Details | Diff | Splinter Review 
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.
Component: Untriaged → Event Handling
Product: Firefox → Core
Are these toolkit-specific widgets?
Flags: needinfo?(bugs)
Nope. This is deep in DOM/Layout
Flags: needinfo?(bugs) → needinfo?(jwatt)
See also Bug 1261674.
Whiteboard: [tw-dom]
What info do you need here, Olli? This is simply a case of adding something in PostHandleEvent, or something, right?
Flags: needinfo?(jwatt)
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.
Whiteboard: [tw-dom] → [tw-dom] btpp-backlog
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.
I would like to try on this bug.
Assignee: nobody → sshih
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.
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Whiteboard: [tw-dom] btpp-backlog → [tw-dom] btpp-active
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
Flags: needinfo?(botond)
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);
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.
Adding a system group listener should work indeed, assuming APZC cares about system group listeners and not only default group.
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
Flags: needinfo?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Attachment #8752661 - Flags: review?(bugs)
Blocks: 1261674
Not sure why Chrome doesn't implement the behavior on Mac. Should we implement this behavior on all (Mac, Ubuntu, Windows) platforms?
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?
Attachment #8752661 - Flags: review?(bugs) → review-
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.
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.
(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?
oh, ok, if platforms tend to rely on focus-ness, then fine to rely on that here too.
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
Attachment #8766194 - Flags: review?(bugs)
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.
Attachment #8766194 - Flags: review?(bugs) → review+
(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?
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
Follow reviewer's comments to refine patch and add review in patch summary
Attachment #8766194 - Attachment is obsolete: true
Attachment #8767079 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6a8b565fe625
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1295883
Component: Event Handling → User events and focus handling
Regressions: 1690639
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: