Closed Bug 1437921 Opened 6 years ago Closed 6 years ago

Run document_idle after both DOMContentLoaded _and_ layout has had a chance to start

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox60 --- verified

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(4 files)

This is pretty easy to accomplish from the WebExtension side. From the DOM side, though, I think we'd need a new internal event that fires when stylesheet loads have finished, and possibly a new internal ready state property that we can check.

For reference, the relevant extension framework code is here:

https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/components/extensions/ExtensionContent.jsm#360-370
https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/toolkit/components/extensions/ExtensionUtils.jsm#295-308
Yep, thank you.  I found it.  I'll have proposed patches up soon.
MozReview-Commit-ID: 9cJ1cGuMzBJ
Attachment #8950739 - Flags: review?(kmaglione+bmo)
Comment on attachment 8950739 [details] [diff] [review]
part 2.  Switch document_idle to use the documentReadyForIdle promise

Review of attachment 8950739 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +354,5 @@
>      try {
>        if (this.runAt === "document_end") {
>          await promiseDocumentReady(window.document);
>        } else if (this.runAt === "document_idle") {
> +        let readyThenIdle = promiseDocumentIdle(window);

Nit: Probably no need for the `readyThenIdle` variable at this point.

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +317,5 @@
> +  * idle time.
> +  *
> +  * @param {Window} window The window whose document we will await
> +                           the readiness of.
> +  * @returns {Promise<Document>}

This returns a Promise<IdleDeadline> now.
Attachment #8950739 - Flags: review?(kmaglione+bmo) → review+
> Nit: Probably no need for the `readyThenIdle` variable at this point.
> This returns a Promise<IdleDeadline> now.

Both fixed.
Comment on attachment 8950738 [details] [diff] [review]
part 1.  Add an attribute on document that is a Promise that resolves when the document has fired DOMContentLoaded _and_ maybe started layout

Review of attachment 8950738 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +6782,5 @@
> +{
> +  mMayStartLayout = aMayStartLayout;
> +  if (MayStartLayout()) {
> +    ReadyState state = GetReadyStateEnum();
> +    if (state == READYSTATE_INTERACTIVE || state == READYSTATE_COMPLETE) {

nit: I think it might be clearer if you write it as something like state >= READYSTATE_INTERACTIVE.
Attachment #8950738 - Flags: review?(nika) → review+
Priority: -- → P1
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6f309f8dd8
part 1.  Add an attribute on document that is a Promise that resolves when the document has fired DOMContentLoaded _and_ maybe started layout.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfd2a4b70cc
part 2.  Switch document_idle to use the documentReadyForIdle promise.  r=kmag
https://hg.mozilla.org/mozilla-central/rev/ce6f309f8dd8
https://hg.mozilla.org/mozilla-central/rev/5cfd2a4b70cc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(bzbarsky)
I actually have no idea how one would go about testing this manually.  One might be able to test this with some sort of custom extension, maybe, which checks that all <head> sheets are loaded by document_idle, but I'm not sure how to go about writing that.
Flags: needinfo?(bzbarsky)
Flags: qe-verify-
Based on the bug this was trying to fix, I'd suggest creating an extension with a content script that runs on <all_urls> and touches `document.body.clientWidth`, and then checking that it does not trigger a flash of unstyled content.

This may require flushing the cache and visiting the site multiple times. I've heard that Github tends to experience this problem, so that might be a good testcase if you can reproduce it there without the patch.
Flags: qe-verify-
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1437921#c12 if you think this scenario is valid, can you please provide an extension to verify this issue?
Flags: needinfo?(bzbarsky)
I cannot, sorry.  I have way too many other things on my plate right now.
Flags: needinfo?(bzbarsky)
I tried testing the FOUC scenario described in https://bugzilla.mozilla.org/show_bug.cgi?id=1437644#c0 but was unable to reproduce the issue with (https://addons.mozilla.org/en-US/firefox/addon/imagus/versions/?page=1#version-0.9.8.59) before they added the hotfix on Firefox 60.0a1 (20180213220104) but it does not reproduce so I think it is not what was fixed by this bug.
I also looked in the .js files of the extension and they did not seem to contain `document.body.clientWidth`, does this need to be a specific scenario or can I look for any flash that might be caused by let's say an add blocker or any page? in order to try and reproduce the issue.
Flags: needinfo?(kmaglione+bmo)
> and they did not seem to contain `document.body.clientWidth`

Sure.  The actual code in Imagus that does the layout flush is described in bug 1437644 comment 1.  It's doing:

        d = d.compatMode === "BackCompat" && d.body || d.documentElement;
        var w = d.clientWidth;
        var h = d.clientHeight;

in client.js.

Also, the Imagus addon's problem would not be fixed by this bug, because it uses document_end, not document_idle.

> does this need to be a specific scenario

The simplest way to reproduce is:

1)  Have an addon that flushes layout on document_idle (e.g. by doing document.body.clientWidth).  Have this addon run the script on all URLs.

2)  Create a test page like so:

  <!DOCTYPE html>
  <head>
    <style>body { font-size: 100px; color: red; }</style>
    <link rel="stylesheet" href="http://software.hixie.ch/utilities/cgi/test-tools/delayed-file?pause=5&mime=text%2Fcss&text=body+%7B+color%3A+green+!important+%3B+%7D">
  </head>
  <body>There should be no red.</body>

and see whether you see red before it goes green after 5 seconds.

> or can I look for any flash that might be caused by let's say an add blocker or any page

Only if that's caused by a flush on document_idle.
Flags: needinfo?(kmaglione+bmo)
Attached image flush working.gif
I was able to reproduce the issue in Firefox 60.0a1 (20180213220104) on windows 10 64Bit.
Retested and verified in Firefox 60.0a1 (20180312100129) on Windows 10 64Bit, Mac OS 10.13.2.
Status: RESOLVED → VERIFIED
(In reply to marius.santa from comment #18)
> I was able to reproduce the issue in Firefox 60.0a1 (20180213220104) on
> windows 10 64Bit.
> Retested and verified in Firefox 60.0a1 (20180312100129) on Windows 10
> 64Bit, Mac OS 10.13.2.

Thanks.
Product: Toolkit → WebExtensions
Regressions: 1746783
You need to log in before you can comment on or make changes to this bug.