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

Separate the identification and removal of attributes #4911

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

aredridel
Copy link
Contributor

Because EdgeHTML does not actually remove the value attribute from an HTMLInputElement, the prior implementation would generate an infinite loop. By making this two passes, we are a bit less memory efficient but avoid the nasty edge cases when DOM elements don't fully respect the API.

I believe the existing tests, if they were to run against Edge, would suffer from this case. I was unable to do so as the harness is not configured in a way to make that easy, however, it does fix the case I discovered this with in situ.

Because EdgeHTML does not actually remove the 'value' attribute from an
HTMLInputElement, the prior iteration would generate an infinite loop.
By making this two passes, we are a bit less memory efficient but avoid
the nasty edge cases when DOM elements don't fully respect the API.
@aredridel aredridel changed the title Separate the id and removal of attributes Separate the identification and removal of attributes May 26, 2020
@Conduitry
Copy link
Member

Do you have a repro in which this issue currently manifests itself in old Edge?

@aredridel
Copy link
Contributor Author

I can build one. I ran into this today in QA for a release. I'll see if I can make a nice minimal repro in the morning.

@aredridel
Copy link
Contributor Author

@Conduitry repro at https://github.com/aredridel/svelte-bug-repro; key files are index.svelte and index.html, the rest are build tooling or build product for examination.

@rodoch
Copy link

rodoch commented Jun 6, 2020

I found this issue searching for a solution to exactly this problem. The problem makes forms with bound inputs unusable in most if not all non-Chromium versions of MS Edge and is a blocker to production use. I have reproduced and can confirm that this PR solves the problem - good catch! If it has no negative side-effects I'd love to see it merged to master.

@aredridel
Copy link
Contributor Author

@rodoch Indeed. We've gone to prod with a patch applied via the patch-package package (which is very well built, btw) — but it would be a blocker for us otherwise.

@Conduitry Conduitry merged commit 0dceb2c into sveltejs:master Jun 8, 2020
@Conduitry
Copy link
Member

This has been released in 3.23.1 - thanks!

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.

3 participants