-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Workaround for #179 - use datalink for Chrome iOS #240
Conversation
@@ -31,6 +31,7 @@ var saveAs = saveAs || (function(view) { | |||
node.dispatchEvent(event); | |||
} | |||
, is_safari = /constructor/i.test(view.HTMLElement) | |||
, is_chrome_ios =/CriOS\/[\d]+/.test(navigator.userAgent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not long ago i have switched the is_safari to test a webkit feature instead of doing browser sniffing on user agent
Safari's /constructor/i.test(view.HTMLElement)
is uniq for Safari for Safari's webkit
And since Apple don't allow 3rd party browser engine chrome is basically stuck with running uiwebview or wkwebview
which is the same as running Safari basically.
So the conclusion here is: We can as well use is_safari instead of is_chrome_ios
Since chrome for ios also reports true for /constructor/i.test(view.HTMLElement)
What those this really solve? see my comment. Chrome on ios should fall in to the same category as is_safari since it's the same rendering engine. |
With this condition datalink is used in case of Chrome for iOS. Whats your opinion on:
|
And I forgot, difference from is_safari is that for CriOS |
Also a big difference for Safari & CriOS - |
My opinion on opening a new page vs current page is: open a new page if possible otherwise use current page. There is some scenarios where opening a new page won't work. So my preference would be popup = window.open(x)
if(!popup) window.location.href = x |
So what your commit really comes down to is this? force = type === force_saveable_type || /CriOS\/[\d]+/.test(navigator.userAgent) |
force = type === force_saveable_type || /CriOS\/[\d]+/.test(navigator.userAgent) Almost, but not exactly I think. If
|
Just a observation, what happens if the blob don't have a type? |
Nice one! It is empty. I've just tested the empty datatype also works in CriOS. Or we can then use: blob.type || "application/octet-stream" Both display plaintext "hello world" in a new tab. |
} | ||
var dataType = is_chrome_ios ? blob.type : 'attachment/file'; | ||
var popup = window.open("data:" + dataType + base64Data.slice(base64Data.search(/[,;]/)), '_blank'); | ||
if(!popup) window.location.href = "data:" + dataType + base64Data.slice(base64Data.search(/[,;]/)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we could do something smarter... we are basically reconstructing the same result we get from the reader if it's CriOS? So something like this would be better?
var url = is_chrome_ios ? reader.result : reader.result.replace(/mimetype/, 'attachment/file')
var popup = view.open(url, '_blank')
if(!popup) view.location.href = url
(regex is wrong, but a quick google search may find something)
Something along that line... Probably should use view
instead of window
also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Regarding
view
instead ofwindow
- sure! it was a bug. my fault. - Regarding url - will let you know after I test it
It works quite nice! thanks for suggestion |
Once we have a solution, I can squash it to a single commit so it does not mess the commit history if you want. |
lgtm |
12f5b77
to
3843680
Compare
- Blob links / url objects are not supported in Chrome iOS for now - thx @jimmywarting for suggestions & help with improving this PR
squashed, minified version updated |
closes #179