search

jquery-form / form

jQuery Form Plugin
https://jquery-form.github.io/form/
GNU Lesser General Public License v2.1
5.19k stars 2.14k forks source link

Update default iframeSrc to be 'about:blank' for browsers other than IE #572

Closed KorvinSzanto closed 4 years ago

KorvinSzanto commented 4 years ago

Resolves #571

As stated in #571 chrome is expecting "about:blank"

yet we're passing javascript:false if the page is HTTPS.

This was originally added with this default value in ce4324159ecd44cab4779bef6f6d484aef958c5c without any real explanation for why javascript:false was used.

Given that "about:blank" is required in the spec and it fixes the current issue with Chrome, let's use it as the default even for HTTPS.

Edit: javacript:false is required by IE browsers, this PR applies the javascript:false default only when the browser is detected to be IE.

CrossTheStreams commented 4 years ago

Hey, thanks so much for submitting this fix! FWIW, we found this broke in IE 11, which apparently expects javascript:false in HTTPS.

KorvinSzanto commented 4 years ago

Hey, thanks so much for submitting this fix! FWIW, we found this broke in IE 11, which apparently expects javascript:false in HTTPS.

I've updated the PR to only apply the javascript:false if the browser is IE :+1:

mcdruid commented 4 years ago

The UA for IE11 is something like:

Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; rv:11.0) like Gecko

...so I think the test either needs a capital T, or should be case-insensitive, e.g.:

/(MSIE|trident)/i.test(navigator.userAgent)

IDDesigns commented 4 years ago

Have just come across this PR whilst trying to solve this same issue...

The fix we have just implemented sets the iFrameSrc to null rather than about:blank for https - this appears to work on Chrome 83, IE9-11 and Edge without issues.

KorvinSzanto commented 4 years ago

@IDDesigns The spec doesn't allow for null from what I see: https://html.spec.whatwg.org/multipage/iframe-embed-object.html so it might work today but it's likely that setting it to null will cause issues later.

davidchin commented 4 years ago

Do you know which version of IE requires javascript:false? So far I've tested IE11 and about:blank seems to work fine.

effulgentsia commented 4 years ago

Hey, thanks so much for submitting this fix! FWIW, we found this broke in IE 11, which apparently expects javascript:false in HTTPS.

Do you know which version of IE requires javascript:false? So far I've tested IE11 and about:blank seems to work fine.

@mcdruid tested this for Drupal 7. He found that IE6 displays a mixed content warning when using "about:blank", but that IE7 and up work just fine over https with "about:blank". Based on that, we decided in that Drupal patch to only target MSIE (and not Trident) for retaining the "javascript:false".

It's entirely possible that there's something specific about Drupal that's causing whatever bug @CrossTheStreams found on IE 11 to not be affecting us. Or maybe the bug only hits IE 11 with certain browser configuration (legacy/compatibility/quirks mode)? If someone figures out how to reproduce a bug with "about:blank" on IE 11 (or any version above 6), please share.

effulgentsia commented 4 years ago

Also, Chromium released a fix to what made "javascript:false" break for them to Chrome 85 (Canary) and is asking for people to test it and report back prior to them releasing the fix to Chrome 84 and 83.

KorvinSzanto commented 4 years ago

Even if chrome resolves this issue I'd argue this pull request should be merged. This will undoubtedly be an issue again at some point in the future.

kevindb commented 4 years ago

LGTM

kevindb commented 4 years ago

Thank you @KorvinSzanto for this PR