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

Use @ssorallen's blob support test in customizer #12773

Merged
merged 2 commits into from
Mar 17, 2014
Merged

Conversation

cvrebert
Copy link
Collaborator

Fixes #12617.
I've tested in Chrome and Safari v7.0.1.
Need to test older IE, newer IE, Firefox, and Safari v6.1.

@hnrch02
Copy link
Collaborator

hnrch02 commented Feb 18, 2014

Why not use jQuery for the check? Something like this will do just fine and is a little bit smaller. Also, why directly link to Chrome? I personally also prefer Chrome but shouldn't we be as unbiased as possible about the browser choice? I'd refer to http://browsehappy.com or http://whatbrowser.org

@XhmikosR
Copy link
Member

I don't get any warning with IE 9 but download fails with it. I can see in the console that SCRIPT5022: blob is not supported by this browser and no file does actually get downloaded.

Also, in IE 8 mode, SCRIPT438: Object doesn't support property or method 'bind' so JS parsing fails there.

So to summarize, it's the same as it was for older IE; no warning is shown.

Latest FF is fine ofc.

@XhmikosR
Copy link
Member

Also, irrelevant, but with IE I get SEC7118: XMLHttpRequest for https://api.github.com/gists required Cross Origin Resource Sharing (CORS). Not sure how to fix that (if it's at all possible on our side).

@cvrebert
Copy link
Collaborator Author

Okay, revised. If someone could test in IE again, that'd be swell.

@XhmikosR
Copy link
Member

I still don't get any warning in IE 9 mode with IE 10.

@cvrebert
Copy link
Collaborator Author

@XhmikosR But no non-CORS JS console error message?

@XhmikosR
Copy link
Member

CORS was unrelated to this patch, I just noticed it because of this patch I was looking at IE's console :P

@ssorallen
Copy link
Contributor

The test I wrote checks for native support, but it was not written for the faked Blob prototype that "Blob.js" creates as a polyfill for IE9 and IE10. I will modify the code to check for the polyfill.

@XhmikosR
Copy link
Member

@cvrebert: can you update the PR with the latest change of @ssorallen's check?

EDIT: NVM, I thought he had pushed the change already to his repo :)

@ssorallen
Copy link
Contributor

My intention with this check was to force FileSaver.js to fallback to data: URLs and actually enable the customizer for Safari, not permanently disable it for those users.

The problem is FileSaver.js tests for Blob support by looking for the Blob constructor and incorrectly assumes its existence means it can download files with blob: URLs. Safari 6, 6.1, and 7 implement the Blob constructor but do not support opening blob: URLs.

I'd like to work on the issue in FileSaver.js more to make it fall back properly, which would enable Safari users to download customized Bootstrap files again.

@ssorallen
Copy link
Contributor

I updated the feature check to test for polyfilled Blob: https://github.com/ssorallen/blob-feature-check Safari 6, 6.1, and 7 and IE8 and 9 properly fail the test now.

This should at least give error messages to the correct browsers now.

@XhmikosR
Copy link
Member

Awesome, thanks a lot @ssorallen. I'll wait for @cvrebert to update the PR and I'll test again.

@fat
Copy link
Member

fat commented Mar 17, 2014

lgtm guys when you get this working – good stuff 👍

@cvrebert
Copy link
Collaborator Author

Okay, pushed a new version based on the newer blob-feature-check v1.1.0.
On my Mac: Chrome correctly passes; Safari 7.0.2 correctly fails; Firefox correctly succeeds
@XhmikosR Could you test against IE again?

@XhmikosR
Copy link
Member

@cvrebert: I get the warning when I'm in IE9 compatibility mode but not in IE8, or IE7 mode.

@cvrebert
Copy link
Collaborator Author

@XhmikosR The compatibility modes are notorious for being not entirely consistent with the actual full versions of those browsers.

@XhmikosR
Copy link
Member

Yeah but IE9 is in compatibility mode too :P

@cvrebert
Copy link
Collaborator Author

Going to merge this anyway, since it fixes our handling of Safari and theoretically might fix our handling of older IE, but gonna punt proper verification of older IE handling to another issue.

cvrebert added a commit that referenced this pull request Mar 17, 2014
Use @ssorallen's blob support test in customizer
@cvrebert cvrebert merged commit ac1aba4 into master Mar 17, 2014
@cvrebert cvrebert deleted the customizer-compat branch March 17, 2014 06:00
@mdo mdo mentioned this pull request Mar 17, 2014
@cvrebert
Copy link
Collaborator Author

Opened #13090 regarding IE <= 9.

@ssorallen
Copy link
Contributor

@cvrebert I wonder if the team at http://www.browserstack.com/ would give the core Bootstrap devs free access. I used the free demo to test my blob snippet in IE9.

I will try IE7 and IE8 again, but they properly failed when I tried last time. I did not try IE9 in compatibility mode.

@cvrebert
Copy link
Collaborator Author

@ssorallen We have a free Sauce Labs account for this sort of thing. What's currently hindering us is needing to host the dev version of the docs somewhere publicly-accessible, but that's on our to-do list.

@ssorallen
Copy link
Contributor

@cvrebert Sauce Labs Connect lets you tunnel a connection between your machine and Sauce to allow local testing. That should let you test on all versions of IE without hosting the dev docs somewhere public.

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.

Customizer: Show a warning when IE <=9 or Safari is used
5 participants