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)
WebExtensions
General
Tracking
(firefox60 verified)
VERIFIED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | verified |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(4 files)
10.62 KB,
patch
|
nika
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
503 bytes,
application/x-xpinstall
|
Details | |
245.68 KB,
image/gif
|
Details |
Comment 1•6 years ago
|
||
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
Assignee | ||
Comment 2•6 years ago
|
||
Yep, thank you. I found it. I'll have proposed patches up soon.
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: LpPFPKlHTXe
Attachment #8950738 -
Flags: review?(nika)
Assignee | ||
Comment 4•6 years ago
|
||
MozReview-Commit-ID: 9cJ1cGuMzBJ
Attachment #8950739 -
Flags: review?(kmaglione+bmo)
Comment 5•6 years ago
|
||
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+
Assignee | ||
Comment 6•6 years ago
|
||
> Nit: Probably no need for the `readyThenIdle` variable at this point.
> This returns a Promise<IdleDeadline> now.
Both fixed.
Comment 7•6 years ago
|
||
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+
Updated•6 years ago
|
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce6f309f8dd8 https://hg.mozilla.org/mozilla-central/rev/5cfd2a4b70cc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: qe-verify-
Comment 12•6 years ago
|
||
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-
Comment 13•6 years ago
|
||
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)
Assignee | ||
Comment 14•6 years ago
|
||
I cannot, sorry. I have way too many other things on my plate right now.
Flags: needinfo?(bzbarsky)
Comment 15•6 years ago
|
||
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)
Assignee | ||
Comment 16•6 years ago
|
||
> 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.
Comment 17•6 years ago
|
||
Flags: needinfo?(kmaglione+bmo)
Comment 18•6 years ago
|
||
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.
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Comment 19•6 years ago
|
||
(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.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•