Replace inline script for critical JS with standalone script

[silverwind]

The new script utilizes requestAnimationFrame to run DOM transformations as soon as possible. In my testing, this method is equivalent of a synchronous inline script in terms of outcome (no content flashes).

On Firefox, the callback sometimes fires too early and if that happens we register for another frame. I had 100% sucess within 2 frames in Firefox.

[wxiaoguang]

Although I approved the previous PRs, that's because of I think there are other valuable changes, but not because I consider using hacky methods and inline scripts to avoid flicker is the proper way.

Using hacky methods and inline scripts to avoid every flicker is the wrong way IMO. In the end, not every flicker can be avoided, because there could always be something rendered again after the page gets loaded. The hacky methods only make code harder to understand.

Take GitHub as an example, it also flicks on many pages. I can not see the value of using hacky methods to avoid every flicker. See GitHub pages:

And one more thing, instead of using these inline scripts, I recalled that we have discussed about that moving the Clone URLs into a dropdown menu, then there will be no flicker problem anymore.

[silverwind]

requestAnimationFrame is the proper way to mutate DOM without flicker. It can and should be avoided. It's just sloppiness on GitHub's side that they still have these flickers.

I will also move all stuff in formatting.js into this script as well. These are all critical replacements.

[wxiaoguang]

requestAnimationFrame is the proper way to mutate DOM without flicker.

Hmm, I am still not convinced that it's the proper way. Why attempts>100, not 50 or 1000?

It's just sloppiness on GitHub's side that they still have these flickers.

I wouldn't consider as they are sloppiness. Sometimes when the backend (tmpl) and frontend (like vue) rendered HTML are mixed together, it's very difficult (nearly impossible) to have a clear way to avoid all flickers.

[wxiaoguang]

After reading the code of formatting.js, I have another question about "I will also move all stuff in formatting.js into this script as well. These are all critical replacements.":

Even if you move the formatting.js into the init.js, it still have problems. Imagine a case like this:

<script init.js>

<time id=time1>

<!-- 
very long content, which need a few seconds to transfer, 
then browser will partially render the content above,
while the remaining content will be rendered a few seconds later.
 -->

<time id=time2>

<script index.js>

How could you handle such case?

[silverwind]

That will work as-is. It's going by the assumption that if <script index.js> is in the DOM, all preceding elements are in the DOM as well, which think is a rather safe choice. Could also change it to match on other stuff like <footer>, but it needs to be something that is present on every page.

[silverwind]

Hmm, I am still not convinced that it's the proper way. Why attempts>100, not 50 or 1000?

That's just a last-resort way to run the init function in case we never find the script tag. Fast browsers usually complete in 1 attempt, but depending on connection speed, it might take a few couple of attempts to find the element in the DOM.

[wxiaoguang]

That will work as-is. It's going by the assumption that if <script index.js> is in the DOM, all preceding elements are in the DOM as well, which think is a rather safe choice. Could also change it to match on other stuff like <footer>, but it needs to be something that is present on every page.

Hmm, I am still not convinced that it's the proper way. Why attempts>100, not 50 or 1000?

That's just a last-resort way to run the init function in case we never find the script tag. Fast browsers usually complete in 1 attempt, but depending on connection speed, it might take a few couple of attempts to find the element in the DOM.

How could it work properly? For example: if (document.querySelector('script[src*="index.js"]') || ++attempts > 100) return init();, if the page is partially render, does the requestAnimationFrame work? If requestAnimationFrame works, then the init() will run on a partially rendered page, then <time id=time2> won't be updated?

[silverwind]

I think behaviour of requestAnimationFrame during page-load ist mostly undefined behaviour and browser-specific, but we do not need to concern ourselves with it. If the <script> tag is not found, the function will recurse into itself and request another frame (which IIRC, at earliest shoud happen a 60th of a second later), again looking for the script tag, and only if it's found, it will call init().

[silverwind]

So assuming requestAnimationFrame callbacks happen at a 60th of a second, it might be wise to increase the maximum number of attempts. I initially had it based on the elapsed time (first callback argument), might be wise to use that again, and use something like 30s for the last-resort init() call.

[wxiaoguang]

If the <script> tag is not found, the function will recurse into itself and request another frame (which IIRC, at earliest shoud happen a 60th of a second later), again looking for the script tag, and only if it's found, it will call init().

But the code reads like : if (hasIndexJs || attempts > 100) init()

Then back to the demo case: at the beginning, a page is partially rendered, then after 100th animation, init() is called, then the remaining page is rendered, then the newly rendered page's <time> won't be udpated?

[silverwind]

Right, anything that happens after init will not be mutated. For that, a MutationObserver would be more fitting, and I'd certainly consider it for the <time> mutation, but at least for the clone states, I could not see a easy way to use a MutationObserver (as it affects many elements), so I went with this solution.

[wxiaoguang]

Right, anything that happens after init will not be mutated. , then how could this solution be proper ....

If a user's network is a little slow, they spends more than 1 seconds to fully load the page, then the init() won't take affect on newly rendered elements. They will see broken incorrect (not updated) page elements frequently?

[silverwind]

Yes, the attempt counting is flawed, I will revert it to being time-based (e.g. before 577b5ff), so if a page does not load for example after 30s, we just execute init, even thought it will likely fail.

[wxiaoguang]

However, I am still not convinced that it's the proper way. The methods used are too hacky and do not seem to be easily tested or maintained.

Sometimes when the backend (tmpl) and frontend (like vue) rendered HTML are mixed together, it's very difficult (nearly impossible) to have a clear way to avoid all flickers.

[silverwind]

It seems better than inline scripts which we should eliminate in an case because those are often forbidden via CSP.

[wxiaoguang]

And one more thing, instead of using these inline scripts, I recalled that we have discussed about that moving the Clone URLs into a dropdown menu, then there will be no flicker problem anymore.

[silverwind]

The final solution to clone states will be a backend user setting, than we can just render the right stuff into HTML, no JavaScript involved.

[wxiaoguang]

Yup, there could be better solutions than hacking the JS&DOM loading

[silverwind]

Yeah, but the date formatting unfortunately has no server-side solution, so it will need this mutation mechanism even after clone states are properly fixed, so I consider this effort unavoidable :)

[wxiaoguang]

The only challenge for date/time formatting is timezone.

Timezone-based date formatting could also have a server-side solution, eg:

• let users set their own timezone
• use relative date/time instead of accurate ones
• or even more: automatically save the browsers timezone to session and refresh the current page
  • when a user visits a page, render the session-saved timezone into the page, and use JS to check whether current session already has a timezone setting, if no, show a "preparing ..." shadow dialog (or just hide the whole HTML body) and update the timezone to backend by API/cookie, then reload current page.

Personally, I think it's better to tolerate the slight flicker for timezone-based date formatting by JS instead of making the problem too complex.

[silverwind]

Personally, I can't tolerate flicker/layout shift during load, it's a sign of low-quality to me.

[wxiaoguang]

I can make a flawless (nearly 😁 ) solution for "or even more: automatically save the browsers timezone to session and refresh the current page ", if you do not mind.

Then the backend can get the browser's locale and timezone, and users can get consistent rendered result by backend and by frontend. It could be the best solution for Gitea at the moment because Gitea's frontend mixes a lot of things together.

[wxiaoguang]

I made a simple demo:

• Demo for making backend code know browser's locale and timezone #21443

[silverwind]

Would still like to eventually have a script like this as not all things can be done in webcomponents and I'd like to remove all the inline scripts eventually. Maybe there is a solution without requestAnimationFrame.

As for this PR, it's no longer needed.

[wxiaoguang]

I will try to refactor the number and time JS to Custom Elements.

The "Clone" popup could be resolved by #23231

[silverwind]

See if you can use https://github.com/github/relative-time-element for time.

I was referring to the recent inline JS added in #23553. Those should really be in .js files instead.

[wxiaoguang]

PrettyNumber => LocaleNumber: #23861