Show error page on firefox v49 and below #339

ericawright · 2017-07-27T15:42:34Z

@abhinadduri can you create an appropriate GA event for this?

Everyone else, any opinions on what should be shown on this page?

/unsupported/outdated now looks like this:

very similar to /unsupported/gcm

link goes to "https://support.mozilla.org/kb/update-firefox-latest-version" which offers advice on how to update firefox

Fixes: #319.
Ref: #127.

pdehaan · 2017-07-27T16:48:12Z

frontend/src/common.js

+gcmCompliant().catch(err => {
+  $('#page-one').attr('hidden', true);
+  $('#download').attr('hidden', true);
+  sendEvent('sender', 'unsupported', {


Below this was "recipient" in /frontend/src/download.js (but "sender" in the upload.js).
Is this fine here as "sender" always?

No, it's not. Good catch, we might have to take this out of common.js.

oh, yeah, didn't see that

pdehaan · 2017-07-27T16:49:31Z

frontend/src/common.js

+if (navigator.userAgent.toLowerCase().indexOf('firefox') > -1 &&
+    parseInt(navigator.userAgent.toLowerCase().match(/firefox\/*([^\n\r]*)\./)[1]) <= 49) {
+    sendEvent('sender', 'unsupported', {
+      cd6: 'Unsupported Firefox'


Is this fine as a string?
In every other instance of cd6 I grepped, the cd6 was an error object.

Disclaimer: I know nothing about metrics collection.

Pushed changes.

abhinadduri · 2017-07-27T16:57:18Z

Do you think we should keep it in common and maybe look through the url for a 'download' to determine if it's the recipient or the sender? Or do you think it would be better to move it out of common and into upload and download?

ericawright · 2017-07-27T16:58:32Z

I like not having to repeat the code twice, but then it does add an extra step to figure out which page.

abhinadduri · 2017-07-27T17:03:47Z

I've added a check in common to address this, let me know what you think.

pdehaan · 2017-07-27T17:28:41Z

Semi related, but do we want to do similar UA sniffing and redirect IE users to /unsupported?
Ref: #127 (comment)

ericawright · 2017-07-27T17:29:35Z

@pdehaan yeah, that's my plan at least

ericawright · 2017-07-27T18:36:23Z

@SoftVision-CiprianMuresan can you test on IE for me to ensure that it is being redirected to the unsupported page?

Also, should we block Edge as well, or just some versions of it. If so, which versions?

pdehaan · 2017-07-27T18:50:35Z

public/locales/en-US/send.ftl

@@ -67,6 +67,8 @@ expiredPageHeader = This link has expired or never existed in the first place!
 notSupportedHeader = Your browser is not supported.
 // Firefox Send is a brand name and should not be localized.
 notSupportedDetail = Unfortunately this browser does not support the web technology that powers Firefox Send. You’ll need to try another browser. We recommend Firefox!
+notSupportedOutdatedDetail = Unfortunately this version of Firefox does not support the web technology that powers Firefox Send. You’ll need to update your browser.
+updateFirefox = Update Firefox


/cc @flodolo since this adds some l10n strings.

Looks good to me.

Feel free to use the testpilot solution to ping automatically ;-)
https://github.com/mozilla/testpilot/blob/master/CODEOWNERS

Cool, I just learnt something new today!
Filed as #343, which I can create a PR for shortly.

pdehaan · 2017-07-27T18:51:58Z

server/server.js

-app.get('/unsupported', (req, res) => {
-  res.render('unsupported');
+app.get('/unsupported/:reason', (req, res) => {
+  const outdated = req.params.reason === 'outdated'? true : false;


I think the ? true : false sorcery is unnecessary here since req.params.reason === 'outdated' should be Boolean-enough(tm).

pdehaan · 2017-07-27T18:54:59Z

views/unsupported.handlebars

-    </div>
-  </a>
+  {{#if outdated}}
+    <div class="description" data-l10n-id="notSupportedOutdatedDetail"></div>


Re: notSupportedOutdatedDetail = Unfortunately this version of Firefox does not support the web technology that powers Firefox Send. You’ll need to update your browser.

Then below you send me to a SUMO page. It may be more useful to send users right to the Firefox download page here. :unsolicted_advice:

but the users already have firefox...they just need to know how to update it

pdehaan · 2017-07-27T18:55:49Z

views/unsupported.handlebars

-  </a>
+  {{#if outdated}}
+    <div class="description" data-l10n-id="notSupportedOutdatedDetail"></div>
+    <a id="update-firefox" href="https://support.mozilla.org/az/kb/update-firefox-latest-version">


If I'm reading this correctly, we're sending everybody to the Azerbaijani (az) locale of the page. We may just want to neuter the /az/ portion of the URL.

This should definitely be https://support.mozilla.org/kb/update-firefox-latest-version

pdehaan · 2017-07-27T19:22:45Z

frontend/src/common.js

+if (navigator.userAgent.toLowerCase().indexOf('msie') > -1 ||
+    navigator.userAgent.toLowerCase().match(/trident\/7\./)) {
+  window.location.replace('/unsupported/ie');
+}


Before we go too far down a UA+regex abyss, not sure if we want to consider something like https://github.com/faisalman/ua-parser-js maybe.
https://www.npmjs.com/package/ua-parser-js#using-jqueryzepto-ua

const UAParser = require('ua-parser-js'); const parser = new UAParser(); const uaBrowser = parser.getResult().browser; console.log(uaBrowser); // { name: "Firefox", version: "55.0", major: "55" }

UPDATE: A bit more verbose than I was hoping for, but it reads OK-ish. Pseudo-code ahoy!

const UAParser = require('ua-parser-js'); const uaBrowser = new UAParser().getResult().browser; const isSender = !location.pathname.includes('/download'); const senderOrRecipient = isSender ? 'sender' : 'recipient'; gcmCompliant().catch(err => { sendEvent(senderOrRecipient, 'unsupported', { cd6: err }) window.location.replace('/unsupported/gcm'); }); checkUserAgent(uaBrowser, senderOrRecipient); function checkUserAgent(browser, type) { const majorVersion = parseInt(browser.major, 10); switch (browser.name.toLowerCase()) { case 'firefox': if (majorVersion <= 49) { sendEvent(type, 'unsupported', { cd6: new Error('Firefox is outdated.') }); location.replace('/unsupported/outdated'); return; } break; case 'explorer': // NOTE: I made this value up. location.replace('/unsupported/ie'); break; } }

I'm not a huge fan of including npm modules for everything that seems like overkill. I'm open to it, but the regex is already written and really not a big problem, I don't see too many more UA checks being added in...

pdehaan · 2017-07-27T22:25:03Z

frontend/src/common.js

+
+if (navigator.userAgent.toLowerCase().indexOf('msie') > -1 ||
+    navigator.userAgent.toLowerCase().match(/trident\/7\./)) {
+  window.location.replace('/unsupported/ie');


Do we need a sendEvent() here (before the location.replace())?

good idea. @abhinadduri would you like a different event, or the same event as gets sent for outdated firefox?

SoftVision-CiprianMuresan · 2017-07-28T13:31:06Z

@ericawright, using a local server I've run branch v49 on a Mac and connected remotely from my Windows machine.
Unfortunately Internet Explorer does not show the "Browser not supported" page:

It does, however, work on Firefox 49 and lower.

And to answer your other question, in my opinion we should only block Microsoft Edge v.25.10586.672.0 . The other two versions of Edge work as expected (see Cosmin's comment in #127).

ericawright · 2017-07-28T14:46:17Z

@SoftVision-CiprianMuresan @dannycoates @pdehaan (and others) we have some options to deal with IE. what's happening is that it doesn't support ES6. And it errors in our scripts then fails to run any part of them. So @dnarcese and I have come up with a few solutions.

We create a separate script included in all html files that checks for IE, then redirects to our error page without any ES6 syntax.
We use babelify/browserify to interpret our code to es5 in the bundles.

1 is easier, but we may end up never supporting IE if we go that route.
2 causes us to lose some of the benefits of new ES6 speed, though it shouldn't be too big a problem.

pdehaan · 2017-07-28T14:53:01Z

I think +1 to using babel to transpile to ES5 for maximum compatibility (although we'd still need GCM support, so we wouldnt get that much compatibility from older browsers anyways).

Also, while trying to hack on UA options yesterday, I found https://github.com/biggora/express-useragent. Maybe we can just detect the IE browsers using Express middleware on the servers and do the redirect there for your option 1. Then it can easily use a custom layout if needed.

pdehaan · 2017-07-28T14:54:32Z

@SoftVision-CiprianMuresan Any chance you can send us your IE user agent string: http://www.whatismyuseragent.net/?

ericawright · 2017-07-28T14:57:01Z

@pdehaan turns out @dnarcese has ie11 set up so we've been using that this morning.
"Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; .NET4.0C; .NET4.0E; rv:11.0) like Gecko”

pdehaan · 2017-07-28T14:58:24Z

And to answer your other question, in my opinion we should only block Microsoft Edge v.25.10586.672.0 .

@SoftVision-CiprianMuresan I guess of you have a UserAgent string for that too, it would be helpful.

SoftVision-CiprianMuresan · 2017-07-28T15:02:36Z

Here's my Edge user-agent:
Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/13.10586

And my IE one is:
Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; rv:11.0) like Gecko

pdehaan · 2017-07-28T15:42:18Z

Well, the IE one would fail, because the RegExp works for /trident\/7\./i, but neither of your IE user agent strings contain the "msie" substring.

+if (navigator.userAgent.toLowerCase().indexOf('msie') > -1 ||
+    !!navigator.userAgent.toLowerCase().match(/trident\/7\./)) {
+  window.location.replace('/unsupported/ie');
+}

UPDATE: I take that back... re-reading your if(){}, it has an || not an &&, so the /trident/ regexp should still have been truthy-enough to make that work.

ericawright · 2017-07-28T15:46:54Z

@pdehaan this is true. I'll find another way to identify IE (outside of IE11)

ericawright · 2017-07-28T18:00:55Z

we're struggling with babelify. Since @dannycoates talked about doing a new release today, maybe we merge this without the IE changes. Then either get babelify working or do a last minute fix using option one from above.
(the problem is that it's not correctly changing the class keyword if anyone wants to look into it)

@pdehaan

I take that back... re-reading your if(){}, it has an || not an &&, so the /trident/ regexp should still have been truthy-enough to make that work.

Yes, the 'msie' check is for IE < 11, and the 'trident' check is for IE11

pdehaan · 2017-07-28T18:05:51Z

we're struggling with babelify

I was starting to look into replacing browserify with webpack the other day, but then got side tracked by other projects... If I have a few minutes today I may see if I can get a basic webpack config+minification working, then try adding in babel into the stack-of-doom.

pdehaan · 2017-07-28T18:08:19Z

But I'm 👍 for merging whatever we have, and filing new bugs for sorting out the older IE issues in a future bug/PR.

I really thought IE would have been caught by the GCM check anyways. But maybe that's an issue w/ the promise being async and it trying to run the UA regexes before the GCM check has had a chance to fail out. Or something else entirely.

ericawright · 2017-07-28T18:10:26Z

It seems that because of the ES6 syntax, IE just gives up and doesn't run anything if there is any ES6 syntax anywhere in the file. So the GCM check might have otherwise caught on it. it's the same with the redirect I'm trying to do. the code is correct. and will run in the console, but it sees the file as being corrupted.

ericawright · 2017-07-28T18:18:14Z

Ok, I took out the IE code. So it's ready for review from whoever would like to give one :)

dannycoates

👍

pdehaan · 2017-07-28T19:09:39Z

Submitted new bug to re-add IE detection code: #351 "Redirect IE users to an /unsupported/ie page".

ericawright requested a review from abhinadduri July 27, 2017 15:42

abhinadduri previously approved these changes Jul 27, 2017

View reviewed changes

pdehaan reviewed Jul 27, 2017

View reviewed changes

ericawright and others added 4 commits July 27, 2017 13:05

show error page on firefox v49 and below

6b291ae

adding metrics event

fae6561

fixed metrics for sender and recipient

8eae1f2

moved back to common

7a53ed9

ericawright force-pushed the v49 branch 2 times, most recently from 7b0d2dc to cfd88a9 Compare July 27, 2017 17:19

remove hiding elements with jquery

318e1a4

ericawright force-pushed the v49 branch from cfd88a9 to 318e1a4 Compare July 27, 2017 17:23

create /unsupported/outdated version of the /unsupported page

8d8c2ef

ericawright requested a review from SoftVision-CiprianMuresan July 27, 2017 18:35

ericawright force-pushed the v49 branch 2 times, most recently from d10a9dc to 78b857c Compare July 27, 2017 18:38

pdehaan reviewed Jul 27, 2017

View reviewed changes

ericawright force-pushed the v49 branch from f12964c to 1077ce6 Compare July 27, 2017 18:57

pdehaan reviewed Jul 27, 2017

View reviewed changes

ericawright force-pushed the v49 branch from 1077ce6 to 3c36f77 Compare July 28, 2017 14:53

edit some syntax

8cb40ef

ericawright force-pushed the v49 branch from 3c36f77 to 8cb40ef Compare July 28, 2017 18:14

pdehaan requested a review from dannycoates July 28, 2017 18:30

dannycoates approved these changes Jul 28, 2017

View reviewed changes

dannycoates merged commit 6be9415 into master Jul 28, 2017

pdehaan mentioned this pull request Jul 28, 2017

Redirect IE users to an /unsupported/ie page #351

Closed

pdehaan deleted the v49 branch July 31, 2017 05:08

Show error page on firefox v49 and below #339

Show error page on firefox v49 and below #339

Conversation

ericawright commented Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinadduri commented Jul 27, 2017 • edited Loading

ericawright commented Jul 27, 2017 • edited Loading

abhinadduri commented Jul 27, 2017 • edited Loading

pdehaan commented Jul 27, 2017

ericawright commented Jul 27, 2017

ericawright commented Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdehaan Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SoftVision-CiprianMuresan commented Jul 28, 2017

ericawright commented Jul 28, 2017

pdehaan commented Jul 28, 2017

pdehaan commented Jul 28, 2017

ericawright commented Jul 28, 2017

pdehaan commented Jul 28, 2017

SoftVision-CiprianMuresan commented Jul 28, 2017 • edited Loading

pdehaan commented Jul 28, 2017 • edited Loading

ericawright commented Jul 28, 2017

ericawright commented Jul 28, 2017

pdehaan commented Jul 28, 2017

pdehaan commented Jul 28, 2017

ericawright commented Jul 28, 2017

ericawright commented Jul 28, 2017

dannycoates left a comment

Choose a reason for hiding this comment

pdehaan commented Jul 28, 2017

ericawright commented Jul 27, 2017 •

edited

Loading

abhinadduri commented Jul 27, 2017 •

edited

Loading

ericawright commented Jul 27, 2017 •

edited

Loading

abhinadduri commented Jul 27, 2017 •

edited

Loading

ericawright commented Jul 27, 2017 •

edited

Loading

pdehaan Jul 27, 2017 •

edited

Loading

SoftVision-CiprianMuresan commented Jul 28, 2017 •

edited

Loading

pdehaan commented Jul 28, 2017 •

edited

Loading