Issue 29565799: Noissue - Load options on DOMContentLoaded rather than jQuery.ready (Closed)
|
Created:
Oct. 5, 2017, 7:40 p.m. by Manish Jethani Modified:
Oct. 6, 2017, 1:35 p.m. CC:
kzar Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionNoissue - Load options on DOMContentLoaded rather than jQuery.ready
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use addEventListener #
Total comments: 3
MessagesTotal messages: 8
Patch Set 1 The way jQuery.ready works changed significantly in jQuery 3.0: https://jquery.com/upgrade-guide/3.0/#breaking-change-document-ready-handlers... Now it always calls the callback asynchronously, but more importantly it uses promises now. As a result of this, there is a delay between when the document object dispatches the "DOMContentLoaded" event and when the ready callback is called. The delay is only about ~30ms on my machine here, but a lot can happen in this time. It seems like this is the cause of the FOUC [1] that we've been seeing lately after the jQuery update. You can verify this by trying out this patch. The fix is to listen for DOMContentLoaded directly, which we know for sure hasn't been dispatched already at the time the event listener is added. [1]: https://en.wikipedia.org/wiki/Flash_of_unstyled_content
Nice find! Yeah, that would be better than the workaround we had first in mind. https://codereview.adblockplus.org/29565799/diff/29565800/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29565799/diff/29565800/desktop-options.js#... desktop-options.js:189: $(document).on("DOMContentLoaded", loadOptions); Why not avoiding jQuery at all and just use document.addEventListener? It seems other code in here does the same.
Patch Set 2: Use addEventListener https://codereview.adblockplus.org/29565799/diff/29565800/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29565799/diff/29565800/desktop-options.js#... desktop-options.js:189: $(document).on("DOMContentLoaded", loadOptions); On 2017/10/05 19:50:28, Sebastian Noack wrote: > Why not avoiding jQuery at all and just use document.addEventListener? It seems > other code in here does the same. Yes, that makes sense. Done. (I've also moved it towards the bottom of the file, after all the function definitions.) https://codereview.adblockplus.org/29565799/diff/29565827/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29565799/diff/29565827/desktop-options.js#... desktop-options.js:707: ext.onMessage.addListener(message => By the way I notice that this listener could get called before the DOM is ready.
https://codereview.adblockplus.org/29565799/diff/29565827/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29565799/diff/29565827/desktop-options.js#... desktop-options.js:707: ext.onMessage.addListener(message => On 2017/10/05 20:10:13, Manish Jethani wrote: > By the way I notice that this listener could get called before the DOM is ready. It won't in practice though because of the recent changes we made to showOptions; we can ignore this.
LGTM https://codereview.adblockplus.org/29565799/diff/29565827/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29565799/diff/29565827/desktop-options.js#... desktop-options.js:707: ext.onMessage.addListener(message => On 2017/10/05 20:10:13, Manish Jethani wrote: > By the way I notice that this listener could get called before the DOM is ready. Unlikely, as the document isn't loaded over the network, but possible. I guess let's ignore it until we run into any actually reproducible issue, caused by this, in particular since the options page is eventually replaced. The new options page might have the same theoretical race condition though.
On 2017/10/05 20:14:47, Sebastian Noack wrote: > LGTM Thanks! Thomas, I'd like to know if you think this is OK.
LGTM I'm happy to see that it was such a simple fix. 
|
