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

Fix Tinter.setTheme to not fire using Firefox #6831

Merged
merged 2 commits into from Oct 12, 2018

Conversation

@MTRNord
Copy link
Contributor

MTRNord commented May 31, 2018

This if checks if we got a Firefox using a variable that is undefined everywhere except in Firefox. In Firefox because of how it renders the DOM ensure that css is always loaded before it loads/runs the js code. Therefor onload 1. never triggers and 2. we can just call setTheme.

This if checks if we got a Firefox using a variable that is undefined everywhere except in Firefox. In Firefox because of how it renders the DOM ensure that css is always loaded before it loads/runs the js code. Therefor onload 1. never triggers and 2. we can just call setTheme.
@Half-Shot

This comment has been minimized.

Copy link
Contributor

Half-Shot commented Jun 13, 2018

The only thing I'd probably say is to use something more substantial than typeof X for platform detection. There is probably a handy library for it somewhere?

@MTRNord

This comment has been minimized.

Copy link
Contributor Author

MTRNord commented Jun 13, 2018

@Half-Shot After searching a few hours and looking for libs most of them did UserAgent detection whioch can break badly. This typeof approach is the one considered most safe according to the Stackoverflow awnser linked in the next comment. Using feature detection instead. Firefox is the only one which uses this Type so it is unique on Firefox.

(Edit: Rephrased to make it more clear)

@MTRNord

This comment has been minimized.

Copy link
Contributor Author

MTRNord commented Jun 13, 2018

Link I got this check from: https://stackoverflow.com/a/9851769/4929236

@@ -313,9 +313,13 @@ async function loadApp() {
// FIXME: we should probably block loading the app or even
// showing a spinner until the theme is loaded, to avoid
// flashes of unstyled content.
a.onload = () => {
if (typeof InstallTrigger !== 'undefined') {

This comment has been minimized.

Copy link
@dbkr

dbkr Jun 15, 2018

Member

Hmm. please comment this kind of thing (imagine how confused you'd be if you were reading this file and found it looking at something called InstallTrigger...)

Also, is there not a way to do this based on whether the CSS is loaded or not? I'm thinking something like https://gist.github.com/cvan/8a188df72a95a35888b70e5fda80450d although I tested that and Chrome still thinks the stylesheet has rues before onload is fired. How does Chrome fail if you run the Tinter without the onload? Can we not detect that failure and run the Tinter conditionally on that?

This comment has been minimized.

Copy link
@MTRNord

MTRNord Jun 16, 2018

Author Contributor

@dbkr

I sure can add a comment. Sorry for having missed to do that.

the eventListener you linked is afaik the same as doing onload. I tried it in firefox and it still did not work.

I am not sure if we can catch a error on chrome as I think the Tinter doesnt return a error at all. I only saw some console logs but even these dont always trigger on a error

…ition
@MTRNord

This comment has been minimized.

Copy link
Contributor Author

MTRNord commented Jul 24, 2018

@dbkr Added a comment and moved the old one to its correct Position. Please mention if there is still something unclear.

@turt2live turt2live requested a review from vector-im/riot-web Oct 12, 2018
Copy link
Member

turt2live left a comment

Thanks for addressing this, and major apologies for letting it sit this long.

This fixes one problem, but doesn't fix #6424 anymore - I've opened matrix-org/matrix-react-sdk#2214 to fix the remaining issue. I suspect the problem is just a symptom of time, given a lot has happened between May and now.

@turt2live turt2live merged commit 47ade14 into vector-im:develop Oct 12, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MTRNord MTRNord deleted the MTRNord:patch-8 branch Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.