1286509 - Range input does not fire ‘change’ event when the range is changed using the keyboard.

Status: RESOLVED FIXED

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

Description

[swood]

Range input does not fire ‘change’ event when the range is changed using the keyboard.  Keyboard arrow keys cause the slider to visibly shift along, however the ‘change’ event does not fire.

Can be replicated by creating an input of type "range" and listening for the change event with JavaScript.  Adjusting the input with the keyboard does not fire the 'change' event.

Comment 1

[swood]

Attachment: JS Fiddle Example

Comment 2

[swood]

JS Fiddle Example: https://jsfiddle.net/f1Lgsv7w/

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: Range input does not fire ‘change’ event when the range is changed using the keyboard.

Comment 4

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

Comment on attachment 8776852 [details] [diff] [review]
Range input does not fire ‘change’ event when the range is changed using the keyboard.

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

feedback+ after the following issue is addressed.

::: dom/html/HTMLInputElement.cpp
@@ +3762,5 @@
>          frame->InvalidateFrame();
>        }
> +    }
> +  }
> +  if (aVisitor.mEvent->mMessage == eKeyUp && aVisitor.mEvent->IsTrusted()) {

nit: please add new line above this line.

::: dom/html/HTMLInputElement.h
@@ +1504,5 @@
> +  bool MayFireChangeOnKeyUp(uint32_t aKeyCode) const {
> +    return MayFireChangeOnKeyUp(mType, aKeyCode);
> +  }
> +
> +  static bool MayFireChangeOnKeyUp(uint8_t aType, uint32_t aKeyCode);

This static one looks unnecessary unless we need to handle something similar in HTMLInputElement::HandleTypeChange().

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Range input does not fire ‘change’ event when the range is changed using the keyboard.

Comment 6

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

So change does fire, but only after focus moves elsewhere.

Comment 7

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

Comment on attachment 8777764 [details] [diff] [review]
Range input does not fire ‘change’ event when the range is changed using the keyboard.

Nice and simple. thanks.

Could you perhaps still add a test ensuring we get the right order for
keydown/change/keyup

(I think that is wrong, but that is what other browsers have too.
 IMO it should be keydown/keyup/change, but risky to change that.)

Comment 8

[Ming-Chou Shih [:stone]]

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

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Range input does not fire ‘change’ event when the range is changed using the keyboard.

Comment 10

[Ming-Chou Shih [:stone]]

Comment on attachment 8778226 [details] [diff] [review]
Range input does not fire ‘change’ event when the range is changed using the keyboard.

Added test to check the event sequence and updated patch summary

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85105a2f62d2
Range input does not fire ‘change’ event when the range is changed using the keyboard. r=smaug

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/85105a2f62d2

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for beta50 uplift

Approval Request Comment
[Feature/regressing bug #]: regression fix for bug 1291839
[User impact if declined]: using the keyboard to change narration speed in Reader Mode has no effect
[Describe test coverage new/current, TreeHerder]: automated tests and baked on central and aurora
[Risks and why]: n/i for Stone for risk assessment
[String/UUID change made/needed]: none

Comment 14

[Ming-Chou Shih [:stone]]

[Risks and why]: The risk of this patch should be low. But there are some problems of this patch so that we have to uplift it with the patch of 1295719 at the same time.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8799519 [details] [diff] [review]
Patch for beta50 uplift

Reader mode shipped in Fx49, we should be incrementally improving its quality, Beta50+

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da1c556f903c