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 FOUC on Firefox #1728

Merged
merged 1 commit into from
May 31, 2017
Merged

Fix FOUC on Firefox #1728

merged 1 commit into from
May 31, 2017

Conversation

sondr3
Copy link
Contributor

@sondr3 sondr3 commented May 14, 2017

As mentioned in #1698 Firefox users experience a brief FOUC (flash of unstyled content) while loading various pages in Gitea, this commit should fix this by using Filament Groups loadCSS library to asynchronously load the CSS that makes the pages flash.

This commit will add ~1.5kB of JS to the <head> of the webpages. The scripts need to be in the head for the FOUC to not occur, if you do it with the scripts in the bottom of the page it will load the CSS too late and the FOUC will occur regardless. You could potentially also inline them but I went with linking to them.

You will notice in the before and after from the developer panel that the Font Awesome CSS is loaded last because it will not load until the DOM content is ready.
screen shot 2017-05-14 at 19 56 08
screen shot 2017-05-14 at 19 56 23

NOTE: In case users don't have JS enabled it will load the CSS as normal. However I'm not sure if this is required and that I could just remove the <noscript> line.

This is my first pull request to Gitea, so let me know if I need to change anything. 😃

@lunny lunny added this to the 1.2.0 milestone May 15, 2017
@lunny lunny added the type/bug label May 15, 2017
@MCF
Copy link
Contributor

MCF commented May 16, 2017

I've tested this on MacOS as well. Looks good in Firefox and Chrome. Did not see any small flashes in Firefox at all, which occasionally show up without this patch applied.

I'll try to find some time to test on Windows 7 in the next few days.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 16, 2017
@MCF
Copy link
Contributor

MCF commented May 16, 2017

Built and tested on Windows 7. I tested Firefox 53, Chrome 58 and IE 11. No FOUC on any of them, not even on initial (uncached) page load. Better than github in that regard.

I did not notice any obvious problems with page load speeds on any of the browsers. But I just browsed around the site and compared it to try.gitea.io. No real timing tests.

Nice job with the fix.

<link rel="stylesheet" href="{{AppSubUrl}}/assets/octicons-4.3.0/octicons.min.css">
<link rel="preload" href="{{AppSubUrl}}/assets/font-awesome-4.6.3/css/font-awesome.min.css" as="style" onload="this.rel='stylesheet'">
<noscript><link rel="stylesheet" href="{{AppSubUrl}}/assets/font-awesome-4.6.3/css/font-awesome.min.css"></noscript>
<link rel="stylesheet" href="{{AppSubUrl}}/assets/octicons-4.3.0/octicons.min.css">
Copy link
Member

Choose a reason for hiding this comment

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

lines 24 and 25 are indented with spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you use tabs, I didn't notice, I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And done 😄

@lunny
Copy link
Member

lunny commented May 18, 2017

So this will fix #1271?

@MCF
Copy link
Contributor

MCF commented May 18, 2017

Not sure about that one, but it does fix #1698

@sondr3
Copy link
Contributor Author

sondr3 commented May 18, 2017

@lunny, this specifically fixes #1698 and if it fixes anything else that's purely coincidental 😃

@lunny
Copy link
Member

lunny commented May 20, 2017

@sondr3 I have tested that this will fix #1271, so LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 20, 2017
@strk
Copy link
Member

strk commented May 28, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 28, 2017
@strk
Copy link
Member

strk commented May 28, 2017

Actually @sondr3 please add the license of the new .js files to https://github.com/go-gitea/gitea/blob/master/public/assets/librejs/librejs.html

@strk
Copy link
Member

strk commented May 28, 2017

@sondr3 see #1484 (there are also more librejs things missing, in case you want to deal with this in a separate PR)

@lunny
Copy link
Member

lunny commented May 28, 2017

@ethantkoenig please confirm your codereview

@sondr3
Copy link
Contributor Author

sondr3 commented May 28, 2017

@strk I'll add them to this PR when I get to my computer in a few hours and push a new change 😃

Firefox users will experience a flash of unstyled content on loading
various pages, this patch will fix this issue using Filament Groups
loadCSS library to asynchronously load the CSS responsible for the FOUC.

Will fix #1698.

Signed-off-by: Sondre Nilsen <nilsen.sondre@gmail.com>
@sondr3
Copy link
Contributor Author

sondr3 commented May 28, 2017

Alright, added the JS files to the librejs.html file, I wasn't sure where to link to the license since there's not a canonical homepage for it but OSI seemed like a good fit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants