Closed Bug 1261674 Opened 4 years ago Closed 3 years ago

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

Categories

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

47 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: from_bugzilla2, Assigned: stone)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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="range" field like http://robertnyman.com/html5/forms/input-types.html
2. Hold the cursor over the slider widget and move the scroll wheel
3. Click the slider to focus it and then move the scroll wheel


Actual results:

In both cases, the scroll wheel scrolls the page


Expected results:

GtkHScale, GtkVScale, and QSlider controls increment/decrement in response to scroll wheel events whether or not they are focused.
Component: Untriaged → Event Handling
Product: Firefox → Core
Are these toolkit-specific widgets?
Flags: needinfo?(bugs)
This one isn't toolkit specific either.

Not sure whether we want to handle this in HTMLInputElement::PostHandleEvent or in relevant nsIFrame's HandleEvent method.
Flags: needinfo?(bugs)
Whiteboard: [tw-dom]
Whiteboard: [tw-dom] → [tw-dom] btpp-backlog
Assignee: nobody → sshih
Whiteboard: [tw-dom] btpp-backlog → [tw-dom] btpp-active
Depends on the patch of bug1261673.

Handle wheel events in PostHandleEvent as increasing/decreasing the input's value when the input type is 'range'.
Depends on: 1261673
The expected behavior is not implemented in Chrome (Mac, Ubuntu, Win10), Edge (Win10) and Safari (Mac).
This patch includes
1. handle mouse wheel event in HTMLInputElement with range type as increase / decrease its value.
2. SetMayBeApzAware when the element's type is range
3. Let IsNodeApzAwareInternal return true if it's type is range
#2 and #3 are used to tell APZC it's APZ aware node.
Attachment #8752664 - Attachment is obsolete: true
Attachment #8768290 - Flags: feedback?(btseng)
Comment on attachment 8768290 [details] [diff] [review]
Handle wheel event when mouse cursor is hovered on a focused range input as increasing/decreasing it's value.

Review of attachment 8768290 [details] [diff] [review]:
-----------------------------------------------------------------

There is still some question to be clarified.
Please see my comments below.

::: dom/html/HTMLInputElement.cpp
@@ +4861,5 @@
> +                aVisitor.mEvent->PreventDefault();
> +              }
> +            }
> +            else if (mType == NS_FORM_INPUT_RANGE &&
> +                     nsContentUtils::IsFocusedContent(this)) {

Is there any reason why the focus check algorithm varies when mType is different?

@@ +4869,5 @@
> +                step = GetDefaultStep();
> +              }
> +              MOZ_ASSERT(value.isFinite() && step.isFinite());
> +              (wheelEvent->mDeltaY < 0) ? (value += step) : (value -= step);
> +              SetValueOfRangeForUserEvent(value);

Please prevent reusing the variable for different purpose.
I'd prefer to do it in this way:
  SetValueOfRangeForUserEvent(wheelEvent->mDeltaY < 0 ? value + step : value - step);

::: dom/html/test/test_bug1261674.html
@@ +29,5 @@
> +  // deltaY: deltaY of WheelEvent
> +  // deltaMode: deltaMode of WheelEvent
> +  // valueChanged: expected value changes after input element handled the wheel event
> +  let params = [
> +    {focus: true, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: -1},

Is it expected that the result lower/larger than the boundary we set at the beginning?

@@ +31,5 @@
> +  // valueChanged: expected value changes after input element handled the wheel event
> +  let params = [
> +    {focus: true, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: -1},
> +    {focus: true, deltaY: -1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: 1},
> +    {focus: true, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_PAGE, valueChanged: -1},

ditto
Attachment #8768290 - Flags: feedback?(btseng)
Comment on attachment 8768290 [details] [diff] [review]
Handle wheel event when mouse cursor is hovered on a focused range input as increasing/decreasing it's value.

Review of attachment 8768290 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLInputElement.cpp
@@ +4861,5 @@
> +                aVisitor.mEvent->PreventDefault();
> +              }
> +            }
> +            else if (mType == NS_FORM_INPUT_RANGE &&
> +                     nsContentUtils::IsFocusedContent(this)) {

It's because that nsContentUtils::IsFocusedContent doesn't work for InputElement with number type.

::: dom/html/test/test_bug1261674.html
@@ +29,5 @@
> +  // deltaY: deltaY of WheelEvent
> +  // deltaMode: deltaMode of WheelEvent
> +  // valueChanged: expected value changes after input element handled the wheel event
> +  let params = [
> +    {focus: true, deltaY: 1.0, deltaMode: WheelEvent.DOM_DELTA_LINE, valueChanged: -1},

valueChanged is the InputElement's value difference before and after it handles wheel events.
Follow btseng's feedbacks to refine the patch.
Attachment #8768290 - Attachment is obsolete: true
Attachment #8768368 - Flags: review?(bugs)
Comment on attachment 8768368 [details] [diff] [review]
Handle wheel event when mouse cursor is hovered on a focused range input as increasing/decreasing it's value.


> HTMLInputElement::IsNodeApzAwareInternal() const
> {
>   // Tell APZC we may handle mouse wheel event and do preventDefault when input
>   // type is number.
>-  return (mType == NS_FORM_INPUT_NUMBER) || nsINode::IsNodeApzAwareInternal();
>+  return (mType == NS_FORM_INPUT_NUMBER) || (mType == NS_FORM_INPUT_RANGE) ||
>+          nsINode::IsNodeApzAwareInternal();
Nit, one extra space before nsINode

>+            }
>+            else if (mType == NS_FORM_INPUT_RANGE &&
Nit, } else if (...) {

>+                     nsContentUtils::IsFocusedContent(this)) {
>+              Decimal value = GetValueAsDecimal();
>+              Decimal step = GetStep();
>+              if (step == kStepAny) {
>+                step = GetDefaultStep();
>+              }
>+              MOZ_ASSERT(value.isFinite() && step.isFinite());
>+              SetValueOfRangeForUserEvent(wheelEvent->mDeltaY < 0 ?
>+                                          value + step : value - step);
>               aVisitor.mEvent->PreventDefault();
>             }
Looks like we have elsewhere if (minimum < maximum) { check. We should probably have that here too.
Otherwise we'll get at least input event and possible get the range to an odd state.


>+    window.postMessage("finished", "http://mochi.test:8888");
>+    testIdx++;
>+  }
>+
>+  window.addEventListener("message", event => {
>+    if (event.data == "finished") {
>+      ok(input.value == result,
>+        "Handle wheel in range input test-" + testIdx + " expect " + result +
>+        " get " + input.value);
>+      (testIdx >= params.length) ? SimpleTest.finish() : runNext();
>+    }
>+  });
>+  runNext();
>+}
No idea why we need postMessage + "message" listener.
Is that to get asynchronousness? Could use a comment.

Could you also add a simple test that we get 'input' events.

those fixed, r+
Attachment #8768368 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> No idea why we need postMessage + "message" listener.
> Is that to get asynchronousness? Could use a comment.
It's to handle asynchronous wheel event then check the input's value. I found there is a better method 'sendWheelAndPaint'. I checked the expected input value in its callback.

> Could you also add a simple test that we get 'input' events.
I added some checks that we get input events in the test case with a normal range input (max > min). And a new test case with invalid range (max < min).
Comment on attachment 8770009 [details] [diff] [review]
Bug 1261674: Handle wheel event when mouse cursor is hovered on a focused range input as increasing/decreasing it's value.

Fixed the patch to meet reviewer's comments.
Attachment #8770009 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7035e4f43546
Handle wheel event when mouse cursor is hovered on a focused range input as increasing/decreasing it's value. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7035e4f43546
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.