In order to finish off bug 1522608, I'm converting our preferences UI to use preferencesBindings.js, which Firefox uses, instead of <preference> XBL bindings.

As part of this I had to replace document.getElementById with Preferences.get. I made a list of the ID of every <preference> and searched the code for uses of them, but it's possible some got missed. I can't see anything wrong in the preferences tab or any more errors, and Try was clean.

Another incarnation of bug 1468167?

I guess, but I am not going to get involved in that discussion.

I see there's a comment in preferencesBindings.js that I should call Preferences.forceEnableInstantApply(); in the pref tab. I'll do that when I get back to this bug.

I updated the patch to tip and removed the no more needed styles.

The dockoptions and notifications dialogs are empty. So something is wrong, only loading the prefs in the JS files seems not to be enough.

I also get "Missing preference for ID..." errors when opening the colors dialog.

On General page with ticked "Show an alert" and "Play sound" the "Customize..." and "Play" buttons are disabled. I need to untick/tick the checkboxes to enable them...until I reopen the prefs. I haven't checked the other panes if there are similar issues.

Frank, Ian, I'm taking common/bindings/preferences.xml away, and gutting it. I don't think anything else here affects you.

Geoff could you rename it to suite/components/bindings/preferences.xml to preserve the history. I will fixing up the suite build files after landing.

Thanks
frg

Updated based on comments here and some things I found with tests I am creating.

(In reply to :aceman from comment #5)

What about the sanitize dialog? Also uses <preference> elements that you
remove the binding for.

I forgot it, again! In my new patch I've removed all the preferences stuff in that dialog and made it just an ordinary dialog that does things with Services.prefs.

::: mail/components/preferences/advanced.js
@@ +487,5 @@

/**
* When the user toggles the layers.acceleration.disabled pref,
* sync its new value to the gfx.direct2d.disabled pref too.

• • Note that layers.acceleration.disabled is inverted.

I wonder why doesn't the 'inverted' attribute in the Preferences.addAll()
call not handle this automatically somehow.

It does handle it, but that basically means it's layers.acceleration.enabled. Checking the patch now, I seem to have removed this comment. Putting it back in.

::: mail/components/preferences/colors.xul
@@ -23,5 @@

#endif

• <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>
• <script type="application/javascript"><![CDATA[
• function init() {

Why removing this? This initializes the color picker from the pref values.
E.g. element id="foregroundtextmenu" still exists.

It still works.

::: mail/components/preferences/messagestyle.js
@@ +65,5 @@

         .addEventListener("CheckboxStateChange",
                           previewObserver.showHeaderChanged);

• setTimeout(() => {

•  previewObserver.displayTheme(themeName.value);
  

•  this._loaded = true;
  

Why?

Added a comment. I don't really know why, but it's not worth my time investigating.

::: mail/components/preferences/preferences.css
@@ +11,4 @@

-moz-box-orient: vertical;
}

prefwindow > .paneDeckContainer {

Are there any <prefwindow>s left? .paneDeckContainer is under a <preftab>.

Richard has dealt with this.

::: mail/components/preferences/security.js
@@ +18,5 @@

• { id: "mail.spam.logging.enabled", type: "bool" },
• { id: "mail.phishing.detection.enabled", type: "bool" },
• { id: "browser.safebrowsing.enabled", type: "bool" },
• { id: "mailnews.downloadToTempFile", type: "bool" },
• // { id: "pref.privacy.disable_button.view_passwords", type: "bool" },

Why is this commented out?

Because this preferences is already added in chat.js, and I forgot this copy was commented. I've removed the one in chat.js instead.

FX seems to have the same problem. Could this help? https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/colors.xul#90

(In reply to Richard Marti (:Paenglab) from comment #14)

FX seems to have the same problem. Could this help? https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/colors.xul#90

Yeah, that's how I'm solving it now.

I've removed all the preferences binding stuff from the sanitize dialog. It just uses Services.prefs now. (Which I did say in comment 12.)

Fixed the chat sound and the "auto mark as read" radios too.

Why do you sometimes use Preferences and sometimes the elements?

I'm glad you asked that because I was starting to ask myself a similar question.

I was just doing what the original code did, which was sometimes an onchange attribute on a <preference> and sometimes on the element itself. I preferred the latter but now I've gone the other way because it does make the code less ugly.

(In reply to :aceman from comment #22)

Comment on attachment 9048078 [details] [diff] [review]
1527770-preferences-bindings-5.diff

Review of attachment 9048078 [details] [diff] [review]:

Thanks, the updatePlaySound() now looks much better.

Some dialogs (e.g. General->Customize) still has Cancel and OK buttons. Are
those useful, don't preferences apply immediately on all platforms now? So
maybe the Cancel could be removed. Please file a new bug for that if it is
wanted.

At least on Windows, not tested on Linux, "Cancel" reverts the changes. Maybe this is because they are now <dialog> and no more <prefwindow>.

(In reply to :aceman from comment #22)

Comment on attachment 9048078 [details] [diff] [review]
1527770-preferences-bindings-5.diff

Review of attachment 9048078 [details] [diff] [review]:

Thanks, the updatePlaySound() now looks much better.

Some dialogs (e.g. General->Customize) still has Cancel and OK buttons. Are
those useful, don't preferences apply immediately on all platforms now? So
maybe the Cancel could be removed. Please file a new bug for that if it is
wanted.

The comments in preferencesBindings.jsm say:

// The about:preferences page forces instantApply.
and
// Dialogs of type="child" are never instantApply.

This is consistent with what I'm seeing and with v60. They probably should've had buttons when they were <prefwindow>s since I've not added the dlgbuttons attribute, it was already there.

I'm happy with the buttons coming back, now that I've seen it. It makes more sense, I think.

::: mail/components/preferences/display.js
@@ -280,3 @@

 document.getElementById("markAsReadAutoPreferences").disabled = autoMarkDisabled;
 document.getElementById("secondsLabel").disabled = autoMarkDisabled;

• this.updateMarkAsReadTextbox();

Why can this be removed here?

It can't, I messed up. :/

@@ +311,1 @@

   delayTextbox.focus();

This is strange. We toggled a radio and suddenly get the focus forced to
some element? We do not do that in any other similar preferences set.

That is how it's supposed to work. I don't know who made it that way, but it does make sense to me.

::: mail/components/preferences/notifications.xul
@@ +40,3 @@

• <script type="application/javascript" src="chrome://global/content/preferencesBindings.js"/>
• <script type="application/javascript" src="chrome://messenger/content/preferences/notifications.js"/>

Why at the bottom?

That's how to fix the empty dialogs Richard mentions in comment 8 and discussed in comments 13-16.

::: mail/components/preferences/preferences.js
@@ -99,5 @@

 let prefPane = document.getElementById(paneID);

• let tabOnEvent = false;
• // The prefwindow element selects the pane specified in window.arguments[0]
• // automatically. But let's check it and if the prefs window was already
• // open, the current prefpane may not be the wanted one.

What is the replacement for this removal?

Nothing. We never wait for this event because it's already happened. I probably should've changed this in bug 1522778 but I hadn't noticed it.

Tidying up some mistakes. I'm going to do another Try run of this and bug 1530271 together.

(In reply to Geoff Lankow (:darktrojan) from comment #24)

I'm happy with the buttons coming back, now that I've seen it. It makes more sense, I think.

I don't think we should add ok/cancel. Firefox doesn't have that, and even web applications are moving more to instant applying prefs, like is also done in the mobile scene.

(In reply to Magnus Melin [:mkmelin] from comment #26)

(In reply to Geoff Lankow (:darktrojan) from comment #24)

I'm happy with the buttons coming back, now that I've seen it. It makes more sense, I think.

I don't think we should add ok/cancel. Firefox doesn't have that, and even web applications are moving more to instant applying prefs, like is also done in the mobile scene.

Checked Firefox under Windows and Linux and it has the ok/cancel buttons in the dialogs. Check "Advanced..." and "Colors" for example.

Ah, those. I guess I skimmed the comments too fast.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/333fa90d259d
Copy preferences.xml into suite to preserve history; rs=me
https://hg.mozilla.org/comm-central/rev/4a4cb3456840
Convert Thunderbird preferences to use preferencesBindings.js; r=aceman

I'm note sure if it's caused by this bug, but some of the strings in Daily's Preferences don't show the accesskey. For instance _W_hen Daily launches, show the Start Page in the message area doesn't show accesskey W, but _L_ocation and _R_estore Default on the next line do...

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #31)

I'm note sure if it's caused by this bug, but some of the strings in Daily's Preferences don't show the accesskey. For instance _W_hen Daily launches, show the Start Page in the message area doesn't show accesskey W, but _L_ocation and _R_estore Default on the next line do...

Looks like for all the checkboxes the access key isn't shown...

I think it's far more likely to be bug 1455433 that caused missing access key display.

Onno, mind checking Firefox trunk and filing a bug if that's the case? (Or a new bug for tb if thunderbird-specific.)

I already checked this and it's also borken in Furefox 67.0a1.
I added a comment to bug 1455433, but haven't filed a new bug yet.