1526274 - Should enforce preference bindings have inited and updated all elements before calling pane init() methods, and only call updateAllElements() once

Status: NEW

(Firefox :: Settings UI, enhancement, P1)

Description

[:Gijs (he/him)]

As in summary. This is catching us out in bug 1524995 and bug 1525099. I'll propose small fixes that address those specific issues in that bug that are upliftable, but we should fix the underlying issue to avoid this gotcha and the fact that it's a pain to debug.

It's not clear why there's a microtask wait in updateAllElements (https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#127-133 ), as there's no comment in the code. Tim?

Comment 1

[:Gijs (he/him)]

To elaborate a little more here:

• we have code hanging off pane.init() methods that tries to read values from the DOM instead of the pref. That doesn't work if we haven't initialized the pref values
• we have code hanging off change event listeners to try to update values after the fact if prefs change. That doesn't help here because the pref isn't changing, we're just repeatedly initializing / updating the DOM representation without the pref changing.
• adding onsyncfrompreference handlers helps, but has a number of downsides:
  • performance because we call updateAllPreferences() repeatedly so it gets called repeatedly
  • requires faux inline event handling instead of "real" event dispatching (in order to return a different value for preferencesBindings.js to use to update the element with - setting it directly doesn't work because the event is dispatched before, so it'll just get overwritten otherwise), so requires inline DOM event handlers which we're trying to get rid of (deps of bug 1473549).

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The reason why I did that is because of a phabricator comment from :jaws. There is one callsite where we will call init() in the same tick.

https://searchfox.org/mozilla-central/rev/03ebbdab952409640c6857d835d3040bf6f9e2db/browser/components/preferences/in-content/findInPage.js#73

By queuing updateAllElement() we can avoid calling it multiple times on every init().

Sorry for not putting that in a comment.

Comment 3

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo) from comment #2)

By queuing updateAllElement() we can avoid calling it multiple times on every init().

In practice, for me at least, this doesn't seem to work, in that with the patches from bug 1524995 I see onsyncfrompreference called 3 times from updateAllElement for each of the 3 prefs.