Closed Bug 1295719 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jaws, Assigned: stone)

References

Details

Attachments

(3 files, 8 obsolete files)

4.24 KB, patch
Details | Diff | Splinter Review
22.17 KB, patch
stone
: review+
Details | Diff | Splinter Review
22.12 KB, patch
Details | Diff | Splinter Review
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.
Attached patch TestcaseSplinter Review
Attachment #8781671 - Attachment is obsolete: true
stone, could we just change HTMLInputElement::MayFireChangeOnKeyUp a bit?
Flags: needinfo?(sshih)
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.
My test case forgot to include Shift+Up and Shift+Down, which also move by 10 percent.
Need to test whether other browsers fire change right before or after keypress or right before or after keyup
Assignee: nobody → sshih
Flags: needinfo?(sshih)
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.
BTW, we fire change event 'before' keyup event.
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.
Summary: input[type=range] does not fire 'change' event for 10 percent changes (PAGE_UP, PAGE_DOWN, Shift+Left, Shift+Right) → input[type=range,number] does not fire 'change' event for some key combinations
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
(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
When input's value changed by wheel events, we should also fire change events
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
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.
Flags: needinfo?(jaws)
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.
Attachment #8782741 - Flags: feedback+
Flags: needinfo?(jaws)
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()
Flags: needinfo?(bugs)
Updated test case since input (type=number) doesn't increase/decrease its value when shift+up/down
Attachment #8782741 - Attachment is obsolete: true
Attachment #8783376 - Attachment is obsolete: true
Attachment #8783377 - Flags: review?(bugs)
Updated test to ignore Up/Down/Left/Right/PageUp/PageDown tests on Android since these are not supported on Android.
Attachment #8783377 - Attachment is obsolete: true
Attachment #8783377 - Flags: review?(bugs)
Attachment #8783405 - Flags: review?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8783405 - Flags: review?(bugs) → review-
(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.
(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 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)
Attachment #8783405 - Flags: review- → review+
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
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)
Add test to ensure the event sequence is keydown, keypress, input, change, keyup
Attachment #8783405 - Attachment is obsolete: true
Attachment #8784264 - Flags: review+
Blocks: 1297635
(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).
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.
Attachment #8784264 - Attachment is obsolete: true
Attachment #8784654 - Flags: review?(bugs)
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>
Attachment #8784654 - Flags: review?(bugs) → review+
Followed review comments to refine the patch and changed the patch summary
Attachment #8784654 - Attachment is obsolete: true
Attachment #8786249 - Flags: review+
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.
Flags: needinfo?(sshih)
Depends on: 1286509
Flags: needinfo?(sshih)
(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?
Flags: needinfo?(jaws)
Skip the test case with arrow keys on android/b2g since they are not supported on android/b2g
Attachment #8786249 - Attachment is obsolete: true
Attachment #8786570 - Flags: review+
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.
Flags: needinfo?(jaws)
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.
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
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c195197831
input[type=range,number] does not fire 'change' event for some key combinations. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/66c195197831
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi Jared, should we uplift this fix and the one from bug 1291839 to Beta50?
Flags: needinfo?(jaws)
Yes, it looks like this can get uplifted to Beta50.
Flags: needinfo?(jaws)
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
Flags: needinfo?(sshih)
Attachment #8799517 - Flags: approval-mozilla-beta?
[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.
Flags: needinfo?(sshih)
Comment on attachment 8799517 [details] [diff] [review]
Patch for beta50 uplift

Reader mode shipped in Fx49, we should be incrementally improving its quality, Beta50+
Attachment #8799517 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.