Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onChange event doesn't trigger when direclty clicking on the maximum input[type=range] value #12926

Closed
k0mpreni opened this issue May 29, 2018 · 15 comments

Comments

@k0mpreni
Copy link

k0mpreni commented May 29, 2018

Do you want to request a feature or report a bug?
bug

What is the current behavior?
When you directly after the loading choose the maximum value on the input type range, the event isn't fire with an onChange. The event is fire when it's not the maximum value of the input.

Sandbox: https://codesandbox.io/s/7kv59yj360
Click on the maximum (right) of the input of type range.
There is no message on the console and the displayed value doesn't change as expected
Click on a other value on the input, a message is displayed in the console and the value change
When you click on the last value it's now displayed.

What is the expected behavior?
An event should trigger when you directly click on the maximum value of the input of type range.
Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Testing on macOS High Sierra, Firefox 60.0.1, Safari 11.1, Chrome 66.0.3359.181, react 16.2.0

To get around this behaviour, I had to change onChange by onInput but add a blanck onChange to still get the drag behaviour on Safari on Iphone (see this CodeSandBox )

@k0mpreni k0mpreni changed the title Event doesn't trigger when direclty clicking on the maximum value on the Event doesn't trigger when direclty clicking on the maximum value on the input type range May 29, 2018
@k0mpreni k0mpreni changed the title Event doesn't trigger when direclty clicking on the maximum value on the input type range onChange event doesn't trigger when direclty clicking on the maximum input[type=range] value May 29, 2018
@fbarrailla
Copy link

fbarrailla commented May 29, 2018

The workaround of using onInput instead of onChange works well but React thinks that the field is uncontrolled and shows a warning.

Without React, there is no issue with the onChange listener on a range input.
https://jsfiddle.net/L0L07119/

@whs-dot-hk
Copy link

whs-dot-hk commented May 30, 2018

@brainlulz I found one interesting thing, the range work after I remove your max attribute, see

https://codesandbox.io/s/318jw3jjkq

It may be related to how react set the attributes when rendering the dom

@Illu
Copy link
Contributor

Illu commented May 30, 2018

It looks like this only happens when max is equal or less than 50.

@whs-dot-hk That's why by removing the max attribute, the range works because max defaults to 100

@whs-dot-hk
Copy link

whs-dot-hk commented May 30, 2018

The value of range is not a integer as everyone expected, it is a string

https://codesandbox.io/s/ykkon3qp5j (Edited)

same here

https://jsfiddle.net/zf8x0s6k/

@fbarrailla
Copy link

fbarrailla commented May 30, 2018

It doesn't change anything to the issue described here.

https://codesandbox.io/s/r883k20pn

When you use html native properties it is string anyway

<input type="range" max="10" />

@whs-dot-hk
Copy link

whs-dot-hk commented May 30, 2018

Compare

https://codesandbox.io/s/81m5w3q4ql

with

https://codesandbox.io/s/llx9714o7m

both of them fire the event input when click on the max value, the max=60 one fire input then change event, but max=10

@whs-dot-hk
Copy link

Is changing the code binding the default of onInput to onChange possible here? Is there any drawbacks?

@gaearon
Copy link
Collaborator

gaearon commented May 30, 2018

This sounds like something that should be easy to fix. Anyone wants to look into why this is happening?

@Illu
Copy link
Contributor

Illu commented May 30, 2018

@gaearon I'm currently working on it

@aweary
Copy link
Contributor

aweary commented May 30, 2018

Sorry, I should have explained what's happening here when I marked it as a bug.

The reason this is happening is because range inputs will have a default value.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range

The default value is halfway between the specified minimum and maximum—unless the maximum is actually less than the minimum, in which case the default is set to the value of the min attribute.

When we track the input's value we read that value property, initializing the tracked value to to the default. In this case, it defaults to 10 because of the order in which the properties are assigned.

  1. Input element is created, value defaults to 50
  2. min attribute is set, value stays at 50
  3. max attribute is set, value is changed to 10 since 50 is greater than the new max

When we track the input's value we define a getter/setter for the value attribute, which should correct the tracked value the next time it's updated. But since the current value isn't empty, we skip setting value and only set defaultValue, which doesn't update the tracked value. So when you go to update the input by setting it to the max, it doesn't actually update because it thinks the tracked value is already the max.

A couple solutions would be:

  1. Add a special case to postMountWrapper that checks if the element is a range input.
  2. Update inputValueTracking so it updates the tracked value when defaultValue is set as well

The first option is likely the easiest solution. I'm not sure what effects updating inputValueTracking might have, we could be relying on the tracked value and the defaultValue to be different in some cases.

@aweary
Copy link
Contributor

aweary commented May 30, 2018

@Illu I have a fix locally I can submit, but if you want to give it a try feel free to and I'll defer to you 🙂

@Illu
Copy link
Contributor

Illu commented May 30, 2018

@aweary thanks man ! I'll give it a try 😃

@whs-dot-hk
Copy link

whs-dot-hk commented May 30, 2018

@aweary May I present you some test case

I have wrote mine, but this is already there

It said the order

it('sets type, step, min, max before value always', () => {

Some test case I wrote

whs-dot-hk@5dc4073

It's the value always correct? Or is it 50?

@gaearon
Copy link
Collaborator

gaearon commented Jun 13, 2018

Fixed in React 16.4.1.

@nikhilroy2
Copy link

oninput working perfect instant of onchange

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants