1261674 - Scroll wheel does not increment/decrement value of type="range" 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="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.

Comment 1

[Andrew Overholt [:overholt]]

Are these toolkit-specific widgets?

Comment 2

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

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.

Comment 3

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

See also Bug 1261673

Comment 4

[Ming-Chou Shih [:stone]]

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

Depends on the patch of bug1261673.

Handle wheel events in PostHandleEvent as increasing/decreasing the input's value when the input type is 'range'.

Comment 5

[Ming-Chou Shih [:stone]]

The expected behavior is not implemented in Chrome (Mac, Ubuntu, Win10), Edge (Win10) and Safari (Mac).

Comment 6

[Ming-Chou Shih [:stone]]

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

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.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 8

[Ming-Chou Shih [:stone]]

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.

Comment 9

[Ming-Chou Shih [:stone]]

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

Follow btseng's feedbacks to refine the patch.

Comment 10

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

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+

Comment 11

[Ming-Chou Shih [:stone]]

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

Comment 12

[Ming-Chou Shih [:stone]]

(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 13

[Ming-Chou Shih [:stone]]

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

Comment 14

[Ming-Chou Shih [:stone]]

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.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7035e4f43546

Comment 17

[Arun Das]

It looks like the fixing of this bug introduced a new problem. Please see bug 1690639.

I'd be surprised if I'm the first to report it in 5 years :)