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

Detect unsupported browser #7448

Closed
garethbowen opened this issue Dec 15, 2021 · 44 comments
Closed

Detect unsupported browser #7448

garethbowen opened this issue Dec 15, 2021 · 44 comments
Assignees
Labels
Type: Feature Add something new
Milestone

Comments

@garethbowen
Copy link
Member

Is your feature request related to a problem? Please describe.
With the switch to using webviews we no longer control the version of the browser, however we still have a minimum version the app will work with. If the app is used with an unsupported browser it will fail in unpredictable and hard to detect ways.

Describe the solution you'd like
We need to work out which version we want to set as the minimum and provide a clear message to the user about how to fix it. I envisage this as a splash screen on load with a brief message on how to upgrade, or "contact IT support". A significant challenge is how to detect the unsupported browser. Ideally we would use feature detection for just the features we need, but between cht-core and all the libraries we use that's simply too many features. Browser version detection is difficult to do reliably, and the consequence of getting it wrong is significant. I think the right solution is to find a singular feature which was introduced around the right version of the browsers we support and test for that.

Describe alternatives you've considered
I've raised this in cht-core but it could be implemented in cht-android, but it's better to be tied to the cht-core version I think.

@garethbowen garethbowen added the Type: Feature Add something new label Dec 15, 2021
@garethbowen garethbowen added this to the 4.0.0 milestone Dec 15, 2021
@garethbowen garethbowen changed the title Attempt to detect unsupported browser Detect unsupported browser Dec 15, 2021
@dianabarsan
Copy link
Member

dianabarsan commented Dec 17, 2021

We've had reports of discrete features not working on desktop browsers - like Safari - for example: https://forum.communityhealthtoolkit.org/t/using-date-type-leads-to-unexpected-behavior/1623 where the constraint for a date field type if a form was never validating.

I think we should also think about how to detect detect browser type.

@m5r
Copy link
Member

m5r commented Mar 18, 2022

What do we want to do when the app is used with an unsupported browser? We could potentially:

  • warn the user before they even log in
  • display an upgrade card that takes them to the Play store to upgrade WebView/Chrome

This recently updated MDN article covers browser detection using the user agent. It also advises against doing browser detection in general and provides a few alternatives such as feature detection.

Feature detection is verbose but better targets the needed features, if you know exactly which ones you need that is. Modernizr is a widely known feature detection library but is not very granular. For example it can detect ES6 Promise support but not ES2022 Promise.allSettled.
Also, its verbosity makes it add a noticeable weight to the bundle. Detecting 10 features adds 46.69kB, or 3.31 kB gzipped, of Javascript to the bundle.

If we want to go with parsing the user agent, we should rely on robust and proven libraries like bowser. It adds ~4.8kB gzipped to the bundle but allows us to detect the browser, its version, its engine and the OS.
There is an open PR to support detecting whether the user is browsing within a WebView. This combined with the browser version would allow us to know if the CHT is being used with a supported browser. This PR has been open for ~1.5 years now but I've reached out to the maintainer to find out if there was anything we could do to unblock it and getting it merged.
We should note that the user agent can easily be spoofed but this is not a common use of browsers and WebView-based applications to the best of my knowledge.

@garethbowen
Copy link
Member Author

It also advises against doing browser detection in general...

I agree with the advice in that article, however I think we can answer "no" to the questions in the "consideration" section so potentially the CHT is one of the rare cases where an exception can be made. Feature detection is better when there is a specific component of the app/website that you can work around but the rest is usable. We can (and do) polyfill to resolve many of these in JS and CSS but part of this is defining how many versions to polyfill back to which doesn't resolve the question of what to do for browsers earlier than those polyfilled.

We use too many features to feature detect for them all, and as you say, it's not granular enough to test for specific support. We could use it as a proxy for a version, for example if we know a specific feature was introduced in a specific version of a specific browser then checking for that feature will indicate the version. However this has similar fundamental issues as user agent parsing because it's an abstraction away from true feature detection.

There is an bowser-js/bowser#452 to support detecting whether the user is browsing within a WebView.

We have some code to detect whether the webapp is running in cht-android so I don't think this is necessary. If it's running in some other webview then it should be treated in the same way as a browser.

If we want to go with parsing the user agent, we should rely on robust and proven libraries like bowser.

I'm leaning towards this approach. The versions we check here can be aligned with our polyfilling minimums, as well as manual and automatic testing suites. However I don't think we should block users from logging in if they don't meet the requirement so as to reduce the harm from false negatives.

My inclination is to show a notification on the login page and the same notification on the About page as well as a dialog which shows on page load post upgrade (not quite an upgrade card but similar). Instead of linking to the Play Store lets link to a new docs site page so we can update it without needing a cht-core release.

@m5r m5r self-assigned this Mar 22, 2022
@m5r
Copy link
Member

m5r commented Mar 22, 2022

@garethbowen I've opened the PR #7568 and I just want to make sure this is going in the right direction.

For this first rough iteration, this is what it looks like
image
image
image

What is it supposed to look like ultimately? What needs to be shipped to consider this issue done?

@garethbowen
Copy link
Member Author

I think you've got the warnings in the right places. One issue is that users who are already logged in hardly ever go to the About page. It's great to have it there for debugging purposes, but I liked your idea of having an upgrade modal as well.

Please work with @n-orlowski for the appropriate UX and wording here.

@n-orlowski
Copy link

Hi @m5r !

On the login screen we should use something like:

We recommend you use another browser for a better experience

For more information visit our docs site

Although are we actually linking users to the docs site from the login page? As per @garethbowen 's earlier comment, maybe something like "Contact support" might be friendlier here

About page LGTM!

@kennsippell
Copy link
Member

kennsippell commented Apr 27, 2022

Quick feedback/questions

  • What percentage of our CHW users know what the word "browser" means? (I suspect it is low)
  • Would this same message show if you loaded CHT 4.x using CHT Andorid 0.x? When a CHW is inside an CHT-Android-App (eg. YendaNafe), do they know they are in a browser? (I would suspect not). How should they know to resolve this by upgrading?
  • If a CHW user is logging into the App with a connection via Telco Whitelist (eg. 10,000 users in Uganda), then can they be expected to click on the docs link? (It won't load)

Does the Android 1.x upgrade require users to sync all documents fresh?

@garethbowen
Copy link
Member Author

I agree with a lot of what @kennsippell said here. This needs some wordsmithing and testing to make sure the description makes sense to our users.

What would help is as well as detecting the browser version, also detect whether or not the webapp is running inside cht-android and if so what version it's on (we have code to base this on). The advice could then be tailored to the specific case. As an example...

  1. If running cht-android < 1.0.0, user should contact program admins to update cht-android
  2. If running cht-android 1.0.0+ and old webview, user should contact program admins to update webview apk
  3. If running in an old browser but not in the wrapper, user should contact program admins to update their browser

The initial message should basically always be to contact program admins, but it's likely we need to provide users with the additional info so they can provide this to the administrator and fix the issue remotely if desired. In particular the admin will likely need to know which the cht-android, webview, and browser versions, so giving the user an easy way to find that would be good.

@n-orlowski
Copy link

Makes sense! Something like For a better app experience, please contact [your administrator]. Let them know [xyz]. feels easier to understand.

Do we know if CHWs ever contact admins directly? If not, would the appropriate route be getting Supervisors to assist?

cc @katanu @leah-ngaari for any insights here

@leah-ngaari
Copy link

@n-orlowski , it varies across projects, in some projects CHWs contact admins as their primary line of support while in other projects they contact their supervisors. We have also received feedback about CHWs taking too long to be supported on technical issues if they raise them with the supervisors. So I think having a message that instructs CHWs to reach their program admin might be worthwhile. On the other hand, do CHWs know who the program admins are? (maybe an appreciable proportion don't), we could solve this during training by ensuring it is captured in the training materials. @n-orlowski any thoughts?

@m5r
Copy link
Member

m5r commented May 16, 2022

Quick update: the browser detection part is ready for review in #7568, now adding cht-android detection

@m5r
Copy link
Member

m5r commented May 25, 2022

@n-orlowski @leah-ngaari any updates about the wording and how we should present this within the CHT?

@n-orlowski
Copy link

@m5r, had a quick chat with @garethbowen and agreed that having the most human language and instructions here will be the most effective. There might be be variations of the message a user might see, based on the detection, where users are instructed to either contact their administrator or supervisor (depending on project).

  1. For a better app experience, please contact your [administrator or supervisor]. Let them know to update cht-android.
  2. For a better app experience, please contact your [administrator or supervisor]. Let them know to update webview apk.
  3. For a better app experience, please contact your [administrator or supervisor]. Let them know to update your browser.

My thoughts are that this language will be easier to remember and communicate than whatever version users might be on 🙂

@garethbowen
Copy link
Member Author

In addition I think the order of detection is important, for example, if their android app is out of date then don't bother checking the browser. In other words...

if (isOnAndroid()) {
  if (hasOldAppVersion()) {
    // show warning about cht-android
  } else if (hasOldBrowserVersion()) {
    // show warning about webview
  }
} else if (hasOldBrowserVersion()) {
  // show warning about browser
}

@kennsippell
Copy link
Member

kennsippell commented Jun 1, 2022

update cht-android
update webview apk

Can this use the branding of the Android App (eg. "update the YendaNafe Android App"?) or will it actually say "webview apk"? I suspect many users administrator/supervisors won't know the meaning of - "CHT" or "Webview APK"

@garethbowen
Copy link
Member Author

garethbowen commented Jun 1, 2022

Can this use the branding of the Android App (eg. "update the YendaNafe Android App"?) or will it actually say "webview apk"?

The "webview apk" bit there is not about the "YendaNafe Android App", it's about the browser provided by Google (usually) that actually does the rendering. We felt the need to provide some specific information to distinguish between these two cases because the steps to fix the issue are different, so if the admin does know what a "webview apk" is then they can provide the right instructions.

Where we could use the actual app name is with the previous message that mentions "cht-android", because they for sure won't know what that is. I don't think we have access to the app name in the webapp right now but we could add an API to the app for fetching this.

The workaround for now would be for each project to update the message to include the app name that their users will recognise and other useful info, for example, specifying clearly who to contact to help fix this.

@garethbowen
Copy link
Member Author

@m5r How are you getting on with this issue?

Secondly, I came across this issue requesting a warning for Safari users. Could you check if the solution you've chosen also resolves that?

@m5r
Copy link
Member

m5r commented Aug 1, 2022

@garethbowen I've shifted my focus on more pressing issues (bulk user upload for 3.16 and more recently offline user replace) but I've planned to get around to this issue and get it ready for QA by thursday

Thanks for bringing up this Safari issue, I'm not sure if my solution also resolves it but I will check later today and report back here

@m5r
Copy link
Member

m5r commented Aug 3, 2022

00556AF8-8F4D-4339-9564-3E42C2FD29E4

@garethbowen this is the current behavior with my solution
Reading through the description in #6784 it sounds like we want a more specific message for iOS. Is the current generic message good enough?

@garethbowen
Copy link
Member Author

Yes I think we want a stronger message, because unlike a browser being a little old, we know Safari will not work.

I don't want to add too much scope creep to this issue - if it's really easy to add then let's add the Safari check, but if it's going to be difficult, then we'll keep Safari as a separate issue.

@m5r
Copy link
Member

m5r commented Aug 11, 2022

@garethbowen I'd prefer to keep it as a separate issue but I will take some time to implement this in the coming weeks.
This PR lays the groundwork for browser detection so it will make implementing the Safari message easier. In the meantime we can refine how we want the message to be displayed in #6784

@m5r
Copy link
Member

m5r commented Aug 11, 2022

@medic/quality-assurance This is ready for AT on branch 7448-detect-unsupported-browser

@tatilepizs
Copy link
Contributor

Hi @m5r, could you please help me with some steps/comments/ideas about what will be the correct way to test it?

@mrjones-plip
Copy link
Contributor

@tatilepizs - FYI @m5r is out of the office 'til Monday

@mrjones-plip
Copy link
Contributor

mrjones-plip commented Aug 25, 2022

@m5r - @tatilepizs and I did some testing based on your slack input:

the "unsupported browser" message is displayed when using an unsupported browser according to the docs
Chrome <53 and Firefox <98 (latest version at the time of the commit) and any version of Safari should get the message
Chrome >=53 and Firefox >=98 should work as usual

I tested on FF Mobile v57. I did not see a warning on login and did see a note on the about page:

FF Mobile v57 Screenshots

firefox mobile 57 about

firefox mobile 57

Probably not worth mentioning, but on a Windows VM I found an old version of Chrome 43 to test with. It was unable to load the login page and I was unable to login. Again, I do not think this a regression given how very wonky the test setup was:

Chrome 43 on Windows VM Screenshots

chrome 43

@garethbowen
Copy link
Member Author

@mrjones-plip If you still have the VM can you grab the error from the console? Ideally the login page would show instructions even on wonky setups.

@mrjones-plip @tatilepizs The thing to keep in mind when testing this is whether a non-technical user has the right information to act on which has been covered in a lot of discussion above. We can assume they won't know how to update their browser, webview version, or android app (or even know what these are). However, if they take the device to their tech support person, then they need the details to know how to fix the problem.

@tatilepizs
Copy link
Contributor

We also tested using Safari, I think that from this comment nothing was changed for Safari and it will be managed in a different Ticket, But I am not sure if I should see a message on the login page.

This is what we saw:

Images

image

image

@m5r Is this the expected behavior? or should we see some message on the login page?

@mrjones-plip
Copy link
Contributor

If you still have the VM can you grab the error from the console? Ideally the login page would show instructions even on wonky setups.

Yup I still have the VM up! I do have two errors in the console:

Refused to execute script from 'https://mrjones-plip-ssl.cht-tunnel.plip.com/login/lib-bowser.js' because its MIME type ('text/html') is not executable, and strict MIME type checking is enabled.
Uncaught SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode

When I try to login I see another full screen error on https://mrjones-plip-ssl.cht-tunnel.plip.com/medic/login which returns a 400:

https://mrjones-plip-ssl.cht-tunnel.plip.com/medic/login
Images

image

The thing to keep in mind when testing this is whether a non-technical user has the right information to act on which has been covered in a lot of discussion above.

I don't think we have achieved this goal then. We do not show anything on the login page (though I thought we were supposed to? A bug?). Further, as non-technical user, if I happen to look in the "about" section and click on the "see requirements" I'm totally lost as to what to now that I'm on the requirements page.

Earlier m5r suggested we might:

  • warn the user before they even log in
  • display an upgrade card that takes them to the Play store to upgrade WebView/Chrome

The second option seems most important above all others, it will always be seen by new and old users alike!

@m5r
Copy link
Member

m5r commented Sep 1, 2022

Thanks for the amazing work you've done @tatilepizs @mrjones-plip, sorry for letting this bug slip.

The lib-bowser.js file that contains the library we're using to detect a browser was not being served properly and it resulted in the login page never displaying the "unsupported browser" message. Aside from this, I have noticed a translation bug with the error message where it wasn't getting properly translated when changing languages.

I fixed these two bugs and tested them with latest webview on android, latest chrome and firefox on linux and latest safari on iOS. I still can't find a non-hacky way to install older versions of chrome and firefox so I simulated older versions by spoofing the user agent.

I agree with you @mrjones-plip, we can improve on this feature now that we have a better idea of who we're targeting and how we want to nudge them into using a supported browser. We could merge the current work from #7568 but keep this issue open while we iterate on the solution, how does everyone feel about it?

@mrjones-plip
Copy link
Contributor

Thanks @m5r !

I'll give the 7448-detect-unsupported-browser branch another go today and report back here with any feedback on the latest code and my two cents on how to best proceed.

@mrjones-plip
Copy link
Contributor

Oh no! I thought I would have time to test this to this today, but ended up not getting to it. Sorry!

If I understand correctly: we have the warning at login working but no prompt for users who just got a CHT upgrade. I think we should keep working this PR as there's a bigger need to get this feature right than there is to get half the feature merged to main.

4.0 is coming soon, but not tomorrow!

@garethbowen
Copy link
Member Author

@mrjones-plip @m5r The goal is to get all code merged for the 4.0.0 release this week so we can build a beta for Release Testing next week. Can we resolve all known issues with this branch before then?

@tatilepizs
Copy link
Contributor

Thanks @m5r!

Using branch 7448-detect-unsupported-browser I was able to see successfully the message on the login page using Safari.

Images attached - Phone image image

@m5r
Copy link
Member

m5r commented Sep 5, 2022

@garethbowen Thanks for sharing a timeline for the 4.0.0 release preparations. The last outstanding issue is to give a non-technical user the right information to act on, something more actionable than simply being redirected to the requirements page from the CHT docs. Given the short time-frame and the ongoing efforts for the allies team objectives, I won't be able to wrap up this last issue by the end of this week.

@mrjones-plip what's your feeling about this?

@tatilepizs thanks for testing the fix in such a short amount of time 🙌

@garethbowen
Copy link
Member Author

I think we should at least show the same message on the About page as we do on the Login page.

@mrjones-plip Is that sufficient, or do we need to improve the warning message as well?

@mrjones-plip
Copy link
Contributor

I think we should at least show the same message on the About page as we do on the Login page.

Given the pressing need to stabilize master in prep for 4.0, if we add this same wording as above on the about page too, along with the link, that is good enough for me in the short term.

In the long term I think we need something for the already logged in user to see in the UI that isn't on the login page or the about page. If y'all agree, I'll open another ticket!

@garethbowen
Copy link
Member Author

In the long term I think we need something for the already logged in user to see in the UI that isn't on the login page or the about page. If y'all agree, I'll open another ticket!

I agree that'd be ideal as very few people ever see the login page or about page once they've logged in successfully once. Please raise the issue.

@m5r When you have a chance, could you please look at adding the message shown on the login page to the About page above the requirements link?

@mrjones-plip
Copy link
Contributor

Please raise the issue.

Tracking in #7770

@m5r
Copy link
Member

m5r commented Sep 13, 2022

I've implemented the changes and this is how it looks like with the same message in the About page. If everyone is okay with it I'll put it up for review and then AT again.
cc @mrjones-plip @garethbowen @tatilepizs

localhost_5988_

@mrjones-plip
Copy link
Contributor

thanks @m5r -

lgtm!

image
image

@tatilepizs
Copy link
Contributor

Same for Safari, looks good 😄
Thanks @m5r !

Images attached - Phone image image

@m5r
Copy link
Member

m5r commented Sep 14, 2022

@medic/quality-assurance Could you please re-run AT on this issue? Code is on the same branch 7448-detect-unsupported-browser
Many thanks!

@tatilepizs
Copy link
Contributor

I ran it yesterday and tested the Safari behavior and it looks good 😄 - Comment here.
I don't have a device to test it using an old version of Firefox or Chrome, @mrjones-plip was helping us in that part, if he also approves it we can move the ticket to Ready to Merge

@mrjones-plip
Copy link
Contributor

yeah - approved! ship it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Add something new
Projects
Archived in project
Development

No branches or pull requests

8 participants