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

Ensure preact/compat is not converting onchange to oninput for input type="range" in IE11 #2817

Merged

Conversation

gcraftyg
Copy link
Contributor

@gcraftyg gcraftyg commented Nov 5, 2020

When using preact/compat, an input of type="range" converts an onchange to oninput except in IE11. This mimics react's implementation as oninput will not fire on an <input type="range" /> in IE11.

I came across a situation where I was not seeing the onChange prop firing in IE11. Looking at it closer, preact/compat detects IE11 by checking typeof Symbol != 'undefined'. This works well as long as Symbol is not being polyfilled, for instance in a NextJS app with preact.

This PR is adding an additional check to verify that Symbol is native to the browser and not polyfilled, allowing for <input type="range" /> to behave as expected.

@gcraftyg gcraftyg changed the title Ensure preact/compat is not converting onchange to oninput for input type="range" when Symbol is polyfilled Ensure preact/compat is not converting onchange to oninput for input type="range" in IE11 Nov 5, 2020
@gcraftyg gcraftyg changed the title Ensure preact/compat is not converting onchange to oninput for input type="range" in IE11 Ensure preact/compat is not converting onchange to oninput for input type="range" in IE11 Nov 5, 2020
@gcraftyg gcraftyg changed the title Ensure preact/compat is not converting onchange to oninput for input type="range" in IE11 Ensure preact/compat is not converting onchange to oninput for input type="range" in IE11 Nov 5, 2020
@coveralls
Copy link

coveralls commented Nov 5, 2020

Coverage Status

Coverage increased (+0.8%) to 100.0% when pulling 2a09279 on gcraftyg:compat-input-range-ie11-check into 3898945 on preactjs:master.

@gcraftyg gcraftyg marked this pull request as ready for review November 5, 2020 23:26
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this is great! Let's add a quick unit test to make sure we don't regress on this in the future. Our suite runs tests in all browsers including IE11 automatically, so a test where we test if a value was changed should do the job 👍

@gcraftyg
Copy link
Contributor Author

gcraftyg commented Nov 6, 2020

Thanks for taking a look at this @marvinhagemeister.

To clarify on the test coverage, we could amend the existing test:

it('should normalize onChange for range, except in IE11', () => {
// NOTE: we don't normalize `onchange` for range inputs in IE11.
const eventType = /Trident\//.test(navigator.userAgent)
? 'change'
: 'input';
render(<input type="range" onChange={() => null} />, scratch);
expect(proto.addEventListener).to.have.been.calledOnce;
expect(proto.addEventListener).to.have.been.calledWithExactly(
eventType,
sinon.match.func,
false
);
});
or add a new test, to include a Symbol polyfill when it is ran in IE11. That would replicate the problematic scenario we're guarding against. But I'm not sure if that's the same as what you were suggesting.

I'm happy to add the test, just looking for clarification. Thanks!

@marvinhagemeister
Copy link
Member

@gcraftyg yup, that sounds perfect. Either way, amending or adding a new test is fine, as long as we have something that checks for it in the CI 👍

@gcraftyg
Copy link
Contributor Author

I pushed up a test that guards against the regression. In order to polyfill Symbol for the test, I converted the input type check into a function call. It was originally a variable assignment, but that was being assigned before the tests were run.

My preference would be to keep it as a variable, but I'm not aware of a way to inject a polyfill for a specific test before the module is imported.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, the test is perfect! Thank you so much for your PR 🙌

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you so much!

@marvinhagemeister marvinhagemeister merged commit c6223d4 into preactjs:master Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants