1295719 - input[type=range,number] does not fire 'change' event for some key combinations

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

See attached test case.

The "change" event is fired for up/down and left/right but not for page_up, page_down, shift+left, shift+right.

Comment 1

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

Attachment: Testcase

Comment 2

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

Attachment: Testcase

Comment 3

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

stone, could we just change HTMLInputElement::MayFireChangeOnKeyUp a bit?

Comment 4

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

oh, this is a bit messy old code. we handle some key events during PreHandleEvent and some during PostHandleEvent.
This needs some testing, what do other browsers do here.

Comment 5

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

My test case forgot to include Shift+Up and Shift+Down, which also move by 10 percent.

Comment 6

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

Need to test whether other browsers fire change right before or after keypress or right before or after keyup

Comment 7

[Ming-Chou Shih [:stone]]

Tested on Windows 10
Chrome Canary(54.0.2829.0) increases/decreases input's value and fire change event after keydown event of
Up, Ctrl+Up, Shift+Up, Alt+Up
Down, Ctrl+Down, Shift+Down, Alt+Down
Left, Ctrl+Left, Shift+Left, Alt+Left
Right, Ctrl+Right, Shift+Right, Alt+Right
PageUp, Shift+PageUp, Alt+PageUp
PageDown, Shift+PageDown, Alt+PageDown

Edge(25.10586) increases input's value increases input's value and fire change event after keydown event
Up, Shift+Up
Down, Shift+Down
Left, Shift+Left
Right, Shift+Right
PageUp
PageDown

Tested on Linux with Chrome(52.0.2743.116)
Works same as it on Windows 10.

Comment 8

[Ming-Chou Shih [:stone]]

BTW, we fire change event 'before' keyup event.

Comment 9

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

yeah. And some are handled during PostHandleEvent of keypress.

I see no good reason to not follow what blink and edge do here. So, move change handling to PostHandleEvent of keydown.

Comment 10

[Ming-Chou Shih [:stone]]

for input type=range, we need to fire change event for Shift+Up, Shift+Down
for input type=range, we need to fire change event for PAGE_UP, PAGE_DOWN, Shift+PAGE_UP, Shift+PAGE_DOWN, Shift+Left, Shift+Right, Shift+Up, Shift+Down

Comment 11

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #10)
> for input type=range, we need to fire change event for Shift+Up, Shift+Down
input=number, we need to fire change event for Shift+Up, Shift+Down

Comment 12

[Ming-Chou Shih [:stone]]

When input's value changed by wheel events, we should also fire change events

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: input[type=range,number] does not fire 'change' event for some key combinations (V1)

Comment 14

[Ming-Chou Shih [:stone]]

This patch includes
1. changed the timing of firing change event from PreHandleEvent of keyup to PostHandleEvent of keydown
2. fired change event when change input's value for wheel event
3. updated related test cases

Comment 15

[Ming-Chou Shih [:stone]]

I made some modifications of test_change_event.html in my patch. Would you mind to confirm if it's make sense to you? Thanks.

Comment 16

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

Comment on attachment 8782741 [details] [diff] [review]
input[type=range,number] does not fire 'change' event for some key combinations (V1)

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

Only looked at test_change_event.html, looks good just found a bug there.

::: dom/html/test/forms/test_change_event.html
@@ +203,5 @@
>        synthesizeKey("VK_UP", {});
>        synthesizeKey("VK_UP", {});
>        synthesizeKey("VK_DOWN", {});
>        is(numberChange, 4, "Change event should be dispatched on number input element for up/down arrow keys (a special case)");
> +      synthesizeKey("VK_UP", {});

This is not shift+VK_UP. You need to call synthesizeKey with the shiftKey:true property on the object:
synthesizeKey("VK_UP", {shiftKey: true});

same for the following synthesizeKey call.

Comment 17

[Ming-Chou Shih [:stone]]

I found the input (type=number) doesn't increase/decrease its value when handling Shift+Up/Down. Actually the condition to handling keys of input(type=number) and (type=range) are quite different. Should we align them? If the answer is yes, I can create another bug and handle it.

In number input, we check
aVisitor.mEvent->IsTrusted() && !(keyEvent->IsShift() || keyEvent->IsControl() || keyEvent->IsAlt() || keyEvent->IsMeta() || keyEvent->IsAltGraph() || keyEvent->IsFn() || keyEvent->IsOS())

In range input, we check
!keyEvent->IsAlt() && !keyEvent->IsControl() && !keyEvent->IsMeta()

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Change the timing to fire change'

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keydown

Updated test case since input (type=number) doesn't increase/decrease its value when shift+up/down

Comment 20

[Ming-Chou Shih [:stone]]

Attachment: Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keydown

Updated test to ignore Up/Down/Left/Right/PageUp/PageDown tests on Android since these are not supported on Android.

Comment 21

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

Comment on attachment 8783405 [details] [diff] [review]
Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keydown

>@@ -4445,16 +4433,17 @@ HTMLInputElement::PostHandleEvent(EventC
>                   // requires us to jump more.
>                   newValue = value + std::max(step, (maximum - minimum) / Decimal(10));
>                   break;
>                 case  NS_VK_PAGE_DOWN:
>                   newValue = value - std::max(step, (maximum - minimum) / Decimal(10));
>                   break;
>               }
>               SetValueOfRangeForUserEvent(newValue);
>+              FireChangeEventIfNeeded();
>               aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
So this code is executed for keypress. Could we change to use keydown, so that we'd be a bit more consistent.

And please test how others are firing input event.

Comment 22

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

(In reply to Ming-Chou Shih [:stone] from comment #17)
> I found the input (type=number) doesn't increase/decrease its value when
> handling Shift+Up/Down. Actually the condition to handling keys of
> input(type=number) and (type=range) are quite different. Should we align
> them? If the answer is yes, I can create another bug and handle it.
Yeah, sounds like a good idea to have consistent behavior. Some helper method should be used to check
whether key event should change the value.

Comment 23

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

(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #21)
> 
> And please test how others are firing input event.
I mean other browsers.  And are we inconsistent with input + change events?
Feel free to file followup bugs for those.

Comment 24

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

Comment on attachment 8783405 [details] [diff] [review]
Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keydown

Actually, I guess the keypress->keyup should be simple change, so r+, assuming tests pass. (though, we need some test ensuring change fires after keydown but before possible keypress)

Comment 25

[Ming-Chou Shih [:stone]]

Tested with Chrome Canary 54.0.2835.0 canary (64-bit) and Edge 25.10586.0.0, when input element (type=range) is focused, the event sequence of long pressing arrow keys (UP/DOWN) are:
keydown, input, change (repeated several times)
keyup

Chrome has the same behavior when input type=number. Edge doesn't change input's (type=number) value when pressing arrow keys (UP/DOWN)

In same test scenario, Firefox nightly 51.0a1 fires follow events
keydown, keypress, input (repeated several times)
change, up

Comment 26

[Ming-Chou Shih [:stone]]

Same environment as comment 25, press number (0-9) keys on input (type=number), the event sequences of Firefox nightly, Chrome Canary and Edge are
keydown, keypress, input (repeat several times)
keyup
change (when blurred)

Comment 27

[Ming-Chou Shih [:stone]]

Attachment: Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keypress

Add test to ensure the event sequence is keydown, keypress, input, change, keyup

Comment 28

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

(In reply to Ming-Chou Shih [:stone] from comment #25)
> Tested with Chrome Canary 54.0.2835.0 canary (64-bit) and Edge 25.10586.0.0,
> when input element (type=range) is focused, the event sequence of long
> pressing arrow keys (UP/DOWN) are:
> keydown, input, change (repeated several times)
> keyup
> 
> Chrome has the same behavior when input type=number. Edge doesn't change
> input's (type=number) value when pressing arrow keys (UP/DOWN)
> 
> In same test scenario, Firefox nightly 51.0a1 fires follow events
> keydown, keypress, input (repeated several times)
> change, up

Ok, what about with your patch then? We want change to fire multiple times.

(and no need to care of keypress here).

Comment 29

[Ming-Chou Shih [:stone]]

Attachment: Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keypress (V5)

In this patch, I changed the timing to fire 'change' events for following conditions
1.HTMLInputElement::PostHandleEvent handles key events (keycode=up/down) for number input
2.HTMLInputElement::PostHandleEvent handles key events (keycode=arrows) and page down/up for range input 
3.HTMLInputElement::PostHandleEvent handles wheel events for range input
#1,2 are handled when keypress as original implementation.

I also added two test cases to ensure that the events sequence (keydown, keypress, input, change, keyup) fired to DOM for key events (keycode=number, arrows, page down, page up) are similar to other browsers. (I say similar because we fire 'keypress' when handling key events with keycode=arrows but other browsers don't)

Then the final events sequence fired for key events (keycode=arrows) will be
keydown, keypress, input, change (may repeated several times)
keyup

The events sequence fired for key events (keycode=numbers) are not impacted by this patch (and it's same as other browsers)

I'm not sure if there is anything else I missed, so I change it to r? and ask for review again.

Comment 30

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

Comment on attachment 8784654 [details] [diff] [review]
Change the timing to fire 'change' event from PreHandleEvent of keyup to PostHandleEvent of keypress (V5)

>+  let waiting_event_sequence = ["keydown", "keypress", "input", "change"];
>+
>+  let waiting_event_idx = 0;
>+  waiting_event_sequence.forEach((element) => {
>+    number.addEventListener(element, (event) => {
could you rename 'element' to 'eventType'. I was wondering what this is doing before realizing element is just a value from waiting_event_sequence, not any DOM element.
Same also elsewhere.

>+      let waiting_event = waiting_event_sequence[waiting_event_idx];
>+      is(waiting_event, element, "Waiting " + waiting_event + " get " + element);
>+      waiting_event_idx = waiting_event_idx == waiting_event_sequence.length -1 ? 0 : waiting_event_idx + 1;
>+    }, false);
>+    range.addEventListener(element, (event) => {
>+      let waiting_event = waiting_event_sequence[waiting_event_idx];
>+      is(waiting_event, element, "Waiting " + waiting_event + " get " + element);
>+      waiting_event_idx = waiting_event_idx == waiting_event_sequence.length - 1 ? 0 : waiting_event_idx + 1;
>+    }, false);
>+  });
>+
>+  number.focus();
>+  synthesizeKey("VK_DOWN", {type: "keydown"});
>+  synthesizeKey("VK_DOWN", {type: "keydown"});
>+  synthesizeKey("VK_DOWN", {type: "keyup"});
>+  number.blur();
>+  range.focus();
>+  waiting_event_idx = 0;
>+  synthesizeKey("VK_DOWN", {type: "keydown"});
>+  synthesizeKey("VK_DOWN", {type: "keydown"});
>+  synthesizeKey("VK_DOWN", {type: "keyup"});
So you're dispatching two keydowns to test repetition? and 
waiting_event_idx = waiting_event_idx == waiting_event_sequence.length -1 ? 0 : waiting_event_idx + 1;
is needed because of that.
This needs some comment somewhere.

>+  number.addEventListener("change", (event) => {
>+    is(keyup_event_of_number, 1, "change event should be fired after keyup");
after blur, right? Not after keyup
>+</html>

Comment 31

[Ming-Chou Shih [:stone]]

Attachment: Bug 1295719 - input[type=range,number] does not fire 'change' event for some key combinations. r=smaug. (V6)

Followed review comments to refine the patch and changed the patch summary

Comment 32

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

stone, would this patch be something that we could uplift to aurora50? bug 1291839 just got aurora approval but it depends on the functionality that you are fixing here.

Comment 33

[Ming-Chou Shih [:stone]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> stone, would this patch be something that we could uplift to aurora50? bug
> 1291839 just got aurora approval but it depends on the functionality that
> you are fixing here.

This patch change the timing to fire 'change' event and follow what other browsers do. So I think the answer is Yes. But this bug depends on 1286509. The patch of this bug overrides the patch of bug1286509. Only the test case added for bug1286509 should be merged. Should I merge the test case added for bug1286509 into the patch of this bug for aurora?

Comment 34

[Ming-Chou Shih [:stone]]

Attachment: Bug 1295719 - input[type=range,number] does not fire 'change' event for some key combinations. r=smaug. (V7)

Skip the test case with arrow keys on android/b2g since they are not supported on android/b2g

Comment 35

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

So bug 1286509 is obsolete with the changes in this bug? If that is true, then yeah, it makes sense to me to merge the test case added for bug 1286509 into the patch of this bug for aurora.

Alternatively, you could request uplift for bug 1286509 since it has three weeks of testing now and then this patch should rebase cleanly on top of it.

Comment 36

[Ming-Chou Shih [:stone]]

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

Comment 37

[Ming-Chou Shih [:stone]]

I tried the patch for aurora but it looks like NG because some tests are always failed on try server. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac4c66eecd505c633f63a22a9e847a2a0abdd4c8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e21c0a8ae159118ef78ed4bf6988bdfef5d6c8f4
The same tests are passed in the tries of my patch for mozilla-central. Still trying to figure out whether the failures are related to my patch or not.

Comment 38

[Ming-Chou Shih [:stone]]

Looks like those failures addressed in comment37 are not related to my patch because they also happened in my try [1] of a empty patch. I would like to commit this patch to central first. Tried results are in [2]. Thanks.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b62bcec74773a44ddffccac519b2f1255ffeaddc
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcfae505a76ab0269fb4afc94bc1eb594549ddff

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c195197831
input[type=range,number] does not fire 'change' event for some key combinations. r=smaug

Comment 40

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66c195197831

Comment 41

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

Hi Jared, should we uplift this fix and the one from bug 1291839 to Beta50?

Comment 42

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

Yes, it looks like this can get uplifted to Beta50.

Comment 43

[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 44

[Ming-Chou Shih [:stone]]

[Risks and why]: It changes the timing of firing 'change' event from handling keyup to handling keypress. The new timing to fire 'change' event is same as Edge and Chrome. I think the risk of this patch should be low.

Comment 45

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

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

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

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/97b73c8ba503