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

Use change event for Range inputs in IE #4052

Closed
wants to merge 1 commit into from
Closed

Use change event for Range inputs in IE #4052

wants to merge 1 commit into from

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jun 7, 2015

The input event doesn't fire in IE for range types

Half fixes #554 it doesn't address old Chrome, but I wasn't sure if that
matters...there is also no precedent for sniffing chrome, though I'd imagine we should just test directly for that behavior, again tho there is no precedent for doing this in the React code base (as far as I can tell), but wouldn't be too tough if we wanted to add it.

The input event doesn't fire in IE for range types

Half fixes #554 it doesn't address old Chrome, but I wasn't sure if that
matters...
@jquense
Copy link
Contributor Author

jquense commented Aug 8, 2015

anything I need to do here to get this past the finish line?

@sophiebits
Copy link
Collaborator

Sorry, not sure if I missed this or just forgot. Instead of using browser detection, I'd like to listen to both events and deduplicate them. This will require some refactoring.

@jquense
Copy link
Contributor Author

jquense commented Aug 9, 2015

I went with checking the document mode since the plugin was already checking. Perhaps deduping would be more of a robust solution...maybe it would also solve #4051 which is a bit more nuclear about IE input events, but I am not really sure how to do that effectively without a timeout, or a lot of active element tracking. Not to mention that the event propagation stuff in React is really opaque (see #4251). I'd be happy to take a stab at it but i'm not quite sure what you have in mind.

My major concern is just that these inputs are unusable in React in IE at the moment, without manually attaching event handlers.

@sophiebits
Copy link
Collaborator

Thanks for the effort. I realize this is painful but I'd rather fix this in a way that's more likely to work across browsers and is more likely to be future-proof. I think listening to all the events is probably the best way to do that.

In any event, I don't think we want to take this as-is so I'm going to close it.

@sophiebits sophiebits closed this Dec 16, 2015
@jquense
Copy link
Contributor Author

jquense commented Dec 17, 2015

I can appreciate that there are better fixes for this, but currently this input is completely broken in IE.

Given that a basic HTML input is just impossible to use in a major browser, short of using a ref and manually attaching change handlers, wouldn't it make more sense to take the band-aid fix, instead of closing it for a indeterminate future better fix? Especially since this doesn't limit or prevent said future fix in anyway?

I'm sorry if I sound a bit nonplussed, I do appreciate ya'll wanting to address this more correctly and robustly, but to be honest the pace and priority of addressing these event issues (6 months on this PR, 2 years on the original issue) suggests that that is just not going to happen anytime soon.

Ya'll have a ton to do and a small team to do it I get that, its just frustrating that trying pitch in to help with that takes months, and ultimately doesn't help. Thanks for taking the time to look at this at least.

@sophiebits
Copy link
Collaborator

I know. But when we have hacky solutions committed we have to maintain them and it's more likely that people will run into weird edge-case bugs that I won't find in my testing, leading me to believe that everything works but then other people run into bugs. There's no good solution here.

@jimfb
Copy link
Contributor

jimfb commented Dec 17, 2015

@jquense This is one of the few long-standing issues that people do run into, and we would be interested in a complete/correct solution. The problem is that for many of these issues, a partial fix is worse than no fix, for the reasons @spicyj mentioned.

If you'd like to try to create a fix that listens to both events and deduplicates, let me know and we can try to get your PR fast-tracked. React does occasionally do user-agent sniffing, but in this case modern browsers don't even agree on behavior with regards to range inputs, and sniffing might make it harder for browsers to converge/change on behavior (because they need to continue supporting our sniffing), so we'd like to avoid sniffing to the extent possible. Basically, what we would need to do is subscribe to both events, and fire a change event when we see an event that gives us a new value which differs from an old value that we've seen. Does that make sense? Let me know if you have any questions and we can help try to come up with a design that makes sense!

You've been a very active contributor, and we appriciate it! Feel free to ping us on any PR that you think we haven't been responsive enough on, and we'll try to jump in. If you don't get a response, just keep pinging! ;). We do want to be more responsive to pull requests, especially from regular contributors like yourself.

@jquense
Copy link
Contributor Author

jquense commented Dec 17, 2015

thanks for the quick responses, and for all the work y'all do. I'd be happy to put together another PR that takes a stab at deduping the events, thanks for the hints @jimfb.

I'm glad to try and pitch in to fix stuff, but as you've noted that gets frustrating when it's hard to get feedback, or it takes months. permission to keep pinging y'all about it, is appreciated and definitely goes a long way. Thanks for taking the time to help me out.

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

Successfully merging this pull request may close these issues.

input type=range onChange should fire when changing the value using the keyboard arrow keys
5 participants