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

Inline color mode switcher code to reduce flashing #88

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

ColdHeat
Copy link
Member

@ColdHeat ColdHeat commented Aug 28, 2024

Inline the color mode switcher code to reduce flashing. In my testing even with speed throttling there is no flashing of the incorrect color.

cc @erdnaxe

@ColdHeat ColdHeat merged commit 81c8774 into main Aug 28, 2024
1 check passed
@ColdHeat ColdHeat deleted the inline-color-mode-switcher branch August 28, 2024 02:36
@ColdHeat
Copy link
Member Author

I think inlining is the right move here, I only caught this issue when putting this code up on demo.ctfd.io as locally the flash doesn't happen at all.

@ColdHeat
Copy link
Member Author

I found that the flashing happened on demo.ctfd.io even after this because of Cloudflare's Rocket Loader. However I did notice the flashing locally which this PR did fix so it was still useful.

@erdnaxe
Copy link
Contributor

erdnaxe commented Aug 28, 2024

From my testing on Firefox, the color theme shouldn't flash as the browser waits for the JS (as it is in <head>, without defer, so blocking the HTML renderer).

What browser are you using and how are you refreshing the webpage? Are you forcing the browser to flush its cache when testing? Do you experience the same flashes on https://hackropole.fr/ (I have implemented a similar color theme switcher there) ?

I'm not a fan of inlining JS in HTML as I am used to blocking inline JS in CSP, but if that reduces flashing, then why not.

@ColdHeat
Copy link
Member Author

ColdHeat commented Aug 28, 2024

@erdnaxe I was testing in a recent version of Chrome. I tried hard refresh and a regular refresh. It would even show up when just clicking to other pages. I'm not a big fan of inlining it either but the flash was getting a little annoying.

I think the main cause was Cloudflare's Rocket Loader but I will take a look and try to put it back in a dedicated file.

One possible cause that I didn't investigate too much was whether the type being module caused Chrome to ignore the defer=False.

@ColdHeat
Copy link
Member Author

ColdHeat commented Aug 28, 2024

@erdnaxe I reverted this back to the color_mode_switcher.js file on https://demo.ctfd.io/ and I am seeing a quick flash of white on Chrome and Firefox.

EDIT: Will leave this up for a little bit

@erdnaxe
Copy link
Contributor

erdnaxe commented Aug 28, 2024

@ColdHeat arg, I found the culprit! type=module implies defer.

I confirm that I observe the flashing on demo.ctfd.io, which confirms the JS is deferred.

So the fix should be as simple as patching CTFd to remove module type on this JS file:

-<script type="module" src="/themes/core-beta/static/assets/color_mode_switcher.52334129.js"></script>
+<script src="/themes/core-beta/static/assets/color_mode_switcher.52334129.js"></script>

@erdnaxe
Copy link
Contributor

erdnaxe commented Aug 28, 2024

I am going to make a pull request to remove type=module when defer is False in https://github.com/CTFd/CTFd/blob/master/CTFd/constants/assets.py#L28

@ColdHeat
Copy link
Member Author

Yeah I do see that removing type="module" makes the original code work. I will go through it that new PR and probably revert the inlining commit.

ColdHeat added a commit that referenced this pull request Sep 1, 2024
ColdHeat added a commit that referenced this pull request Sep 2, 2024
* Revert "Inline color mode switcher code to reduce flashing (#88)"

This reverts commit 81c8774.

* Specify type=None for a non-module JS file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants