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

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: swood, Assigned: stone)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8770591 [details]
JS Fiddle Example
(Reporter)

Comment 2

2 years ago
JS Fiddle Example: https://jsfiddle.net/f1Lgsv7w/
(Assignee)

Updated

2 years ago
Assignee: nobody → sshih
(Assignee)

Comment 3

2 years ago
Created attachment 8776852 [details] [diff] [review]
Range input does not fire ‘change’ event when the range is changed using the keyboard.
(Assignee)

Updated

2 years ago
Attachment #8776852 - Flags: feedback?(btseng)
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().
Attachment #8776852 - Flags: feedback?(btseng) → feedback+
(Assignee)

Comment 5

2 years ago
Created attachment 8777764 [details] [diff] [review]
Range input does not fire ‘change’ event when the range is changed using the keyboard.
Attachment #8776852 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8777764 - Flags: review?(bugs)

Comment 6

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

Comment 7

2 years ago
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.)
Attachment #8777764 - Flags: review?(bugs) → review+
(Assignee)

Comment 9

2 years ago
Created attachment 8778226 [details] [diff] [review]
Range input does not fire ‘change’ event when the range is changed using the keyboard.
Attachment #8777764 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
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
Attachment #8778226 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85105a2f62d2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

2 years ago
Blocks: 1295719
Created attachment 8799519 [details] [diff] [review]
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
Flags: needinfo?(sshih)
Attachment #8799519 - Flags: approval-mozilla-beta?
(Assignee)

Comment 14

2 years ago
[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.
Flags: needinfo?(sshih)

Updated

2 years ago
status-firefox50: --- → affected
Comment on attachment 8799519 [details] [diff] [review]
Patch for beta50 uplift

Reader mode shipped in Fx49, we should be incrementally improving its quality, Beta50+
Attachment #8799519 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 16

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/da1c556f903c
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.