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.

Sign up for GitHub

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

Jump to bottom

enhance(noscript): better handling of JS-disabled-or-not-working states #3852

Merged
merged 11 commits into from
Aug 22, 2024
Merged

enhance(noscript): better handling of JS-disabled-or-not-working states #3852

merged 11 commits into from
Aug 22, 2024

Conversation

marcelgerber
Copy link
Member

@marcelgerber marcelgerber commented Aug 6, 2024 •
edited
Loading

This PR resolves most of the cases where we weren't handling a browser with JS not enabled or not working properly (e.g. old browser) nicely.

There are still a few cases that could see improvements, but we have a good foundation now, mostly based on the mutually-exclusive js-enabled/js-disabled classes on the <html> element.

This all is way more involved than you'd think: See https://github.com/owid/owid-grapher/blob/noscript/docs/browser-support.md#detecting-disablednon-functioning-js for all the details of this.

There's also a long companion video on Slack: https://owid.slack.com/archives/CQQUA2C2U/p1722965563740379

One before/after example

(styles can be improved 😁)
CleanShot 2024-08-06 at 18 30 15

@owidbot
Copy link
Contributor

owidbot commented Aug 6, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-noscript

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-08-06 14:13:42 UTC
Execution time: 1.17 seconds

@marcelgerber marcelgerber requested a review from ikesau August 6, 2024 17:30
@marcelgerber marcelgerber marked this pull request as ready for review August 6, 2024 17:31
@marcelgerber
Copy link
Member Author

I just changed KeyIndicatorCollection to use mobile styles in case JS is disabled.
The mobile version of it is very usable without JS; the desktop version is an interactive accordion where all but the initially-open items are unusable without JS.

This is what this looks like now on desktop without JS (pending the fix in #3864):

CleanShot 2024-08-12 at 10 41 13

@rakyi
Copy link
Contributor

rakyi commented Aug 19, 2024

The case when JS is enabled, but the script is blocked, seems very rare. Are we aware of this happening in the wild (and the user expecting it to work)? IMHO, the additional complexity of supporting it is not worth it, but I might be missing something.

@marcelgerber
Copy link
Member Author

@rakyi You mean the code over here, right?

owid-grapher/site/NoJSDetector.tsx

Lines 41 to 56 in 394d7c2

// Attaches `onerror` event listeners to all scripts with a `data-attach-owid-error-handler` attribute.
// If they fail to load, it will set the `js-disabled` class on the HTML element.
export const ScriptLoadErrorDetector = () => (
<script
dangerouslySetInnerHTML={{
__html: `
document.querySelectorAll("script[data-attach-owid-error-handler]").forEach(script => {
script.onerror = () => {
console.log(new Error("Failed to load script: ", script.src));
document.documentElement.classList.add("js-disabled");
document.documentElement.classList.remove("js-enabled");
}
})`,
}}
/>
)

I'm not certain how common it is, but two scenarios where it can happen are:

  • Rogue / misconfigured ad blockers
  • Network errors or timeouts, especially on mobile on cellular data
    • This one is probably more common - I'm pretty sure I've had this happen on my phone a couple of times when I had bad reception. A reload usually fixes that scenario.

I don't think the 15-or-so lines needed to handle that case are too bad.

The other JS-is-enabled-but-not-working case is the one of SyntaxErrors. These are arguably more annoying to handle, but there's also no way around these.
For example, something simple like

try {
	const regex = /\p{L}/ui
} catch {
}

will lead to a SyntaxError in Firefox <78 for using \p{L}, and also in Chrome <66 because of catch {} rather than catch (e) {}.
It means that the whole JS file cannot be parsed, and therefore not executed.

However, catching a SyntaxError is no easy feat, since it can pretty much only be caught using a global window.onerror handler. I don't love it, but I think it is important enough to be caught.

@danyx23
Copy link
Contributor

danyx23 commented Aug 19, 2024

(Just wanted to quickly jump in here and say thanks for working on this :) )

@marcelgerber marcelgerber requested review from rakyi and removed request for ikesau August 19, 2024 16:20
rakyi
rakyi approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@rakyi rakyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain how common it is, but two scenarios where it can happen are:

My hunch is that it's not worth it to support that, but I might be wrong.

will lead to a SyntaxError in Firefox <78 for using \p{L}, and also in Chrome <66 because of catch {} rather than catch (e) {}.

We even declare that we don't support these versions of those browsers, so I'm surprised that we now add code to support them. Do we have analytics on how many users use them?

Otherwise, nice work, LGTM! None of the above is blocking, up to you if you want to change anything.

@marcelgerber marcelgerber merged commit 8255ad8 into master Aug 22, 2024
23 checks passed
@marcelgerber marcelgerber deleted the noscript branch August 22, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rakyi rakyi rakyi approved these changes

Assignees

@marcelgerber marcelgerber

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcelgerber @owidbot @rakyi @danyx23