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 default iframeSrc to be 'about:blank' for browsers other than IE #572

Merged
merged 3 commits into from
Jun 7, 2020

Conversation

KorvinSzanto
Copy link
Contributor

@KorvinSzanto KorvinSzanto commented May 21, 2020

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 ce43241 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
Copy link

CrossTheStreams commented May 22, 2020

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

This seems to fix the reported issue in IE and every browser I tested with.
@KorvinSzanto KorvinSzanto changed the title Update default iframeSrc to be 'about:blank' in all cases Update default iframeSrc to be 'about:blank' for browsers other than IE May 24, 2020
@KorvinSzanto
Copy link
Contributor Author

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 👍

@mcdruid
Copy link

mcdruid commented May 26, 2020

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
Copy link

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
Copy link
Contributor Author

@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
Copy link

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

@effulgentsia
Copy link

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
Copy link

effulgentsia commented May 30, 2020

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 pushed a commit to concretecms/concretecms-core that referenced this pull request Jun 3, 2020
@KorvinSzanto
Copy link
Contributor Author

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.

KorvinSzanto pushed a commit to concretecms/concretecms-core that referenced this pull request Jun 4, 2020
KorvinSzanto pushed a commit to concretecms/concretecms-core that referenced this pull request Jun 4, 2020
KorvinSzanto pushed a commit to concretecms/concretecms-core that referenced this pull request Jun 4, 2020
@kevindb
Copy link
Contributor

kevindb commented Jun 7, 2020

LGTM

@kevindb kevindb merged commit b3c2209 into jquery-form:develop Jun 7, 2020
@kevindb
Copy link
Contributor

kevindb commented Jun 7, 2020

Thank you @KorvinSzanto for this PR

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.

7 participants