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

Update SubmitEvent Polyfill to Workaround Safari 15 bug #405

Merged
merged 1 commit into from Sep 24, 2021

Conversation

terracatta
Copy link
Contributor

@terracatta terracatta commented Sep 24, 2021

Fixes #390

This PR updates the Polyfill to work around the Safari 15 bug where the submitter property is sometimes null when a form is submitted with element of type button vs an element of type submit. More info https://bugs.webkit.org/show_bug.cgi?id=229660

@terracatta terracatta marked this pull request as ready for review September 24, 2021 16:08
@terracatta
Copy link
Contributor Author

@dhh ok I manually tested the broken behavior using Safari 15 with yarn start and I can confirm this fixes the issue with the least amount of required changes.

@terracatta
Copy link
Contributor Author

cc @ParamagicDev since you use a similar polyfill for your mrujs project.

@KonnorRogers
Copy link
Contributor

Thank you @terracatta , making a note on Mrujs so I can hotfix this.

@dhh
Copy link
Member

dhh commented Sep 24, 2021

Do we need any additional verification that this didn’t break other browsers handling it? Will this automatically become irrelevant when safari is fixed or does it require a change then?

@terracatta
Copy link
Contributor Author

terracatta commented Sep 24, 2021

@dhh

Do we need any additional verification that this didn’t break other browsers handling it?

I don't think so.

First, the way it's written I only run a new behavior if it's Safari (via testing the navigator.vendor property) and Safari says it supports SubmitEvent.

Second, I've been running my ES6 polyfill on a high-traffic app that generates exceptions for console errors, and I am not seeing any regression in browsers that were already working fine.

Will this automatically become irrelevant when safari is fixed or does it require a change then?

So I downloaded the latest Safari Technical Preview Release 132 (Safari 15.4, WebKit 16613.1.1.5) which according to the release notes, has the underlying bug fixed (which I verified using the test case attached to the original Safari bug report). On this version, the polyfill still runs (even though it doesn't need to) but it doesn't do anything bad like throw errors/warnings or regress the behavior we want.

I cannot think of a clean way to test if we are on the fixed version of Safari without actually creating a fake form that produces the bug, submitting it via JS, preventing default, and checking the value of submitter. This might be possible, but I'm not sure if the juice is worth the squeeze.

@dhh
Copy link
Member

dhh commented Sep 24, 2021

Great work. Let’s put a note to revisit this in 3 months or whatever and hope that the version in the wild no longer is a meaningful percentage so we can clean this up 👍

@dhh dhh merged commit a4a3033 into hotwired:main Sep 24, 2021
@terracatta terracatta deleted the jem_workaround_safari_15_bug branch September 24, 2021 18:17
// Certain versions of Safari 15 have a bug where they won't
// populate the submitter. This hurts TurboDrive's enable/disable detection.
// See https://bugs.webkit.org/show_bug.cgi?id=229660
if ("SubmitEvent" in window && /Apple Computer/.test(navigator.vendor)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly question, since Safari on iOS is probably similar to desktop Safari, is there a chance this will need to be fixed when iOS 15 ships?

Copy link
Contributor

Choose a reason for hiding this comment

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

Appears navigator.vendor can only be one of:
"Google Inc.", "Apple Computer Inc.", or ""

So I think we should be okay (you also tested it as well, thank you!)

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

Successfully merging this pull request may close these issues.

TurboDrive fires on forms despite being disabled in Safari 15 beta
3 participants