1522778 - Remove prefwindow and preftab bindings

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task)

Description

[Geoff Lankow (:darktrojan)]

We have a bunch of stuff left from when prefs were in a window. Now that prefs are in a tab, this causes the the tab to close if you press enter or escape.

Comment 1

[Geoff Lankow (:darktrojan)]

Actually we can get rid of the whole binding if we move stuff around a bit.

Comment 2

[Richard Marti (:Paenglab)]

That would be great.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-preferences-bindings-1.diff

Richard, please give this a check on all platforms to make sure I haven't missed or broken anything. Especially the two code paths to the dock options on Mac that I can't check.

Comment 4

[Jorg K (CEST = GMT+2)]

So this is part of what was planned in bug 1468167?

Comment 5

[Geoff Lankow (:darktrojan)]

I guess, but I've never seen that bug before.

Comment 6

[Jorg K (CEST = GMT+2)]

Kindly back out https://hg.mozilla.org/comm-central/rev/63b3267ac3ba when you land this with this commit message:
Bug 1523074 - Backed out changeset 63b3267ac3ba to re-enable tests. a=backout

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039384 [details] [diff] [review]
1522778-preferences-bindings-1.diff

I think we want to land this before the merge on Monday, so first reviewer wins ;-)

Comment 8

[:aceman]

Comment on attachment 9039384 [details] [diff] [review]
1522778-preferences-bindings-1.diff

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

So this inlines used code from aboutPreferences.xml into aboutPreferences.xul and preferences.js?

::: mail/components/preferences/aboutPreferences.xul
@@ +101,5 @@
> +  <script type="application/javascript" src="chrome://messenger/content/customElements.js"/>
> +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> +  <script type="application/javascript" src="chrome://communicator/content/contentAreaClick.js"/>
> +  <script type="application/javascript" src="chrome://messenger/content/preferences/preferences.js"/>
> +  <script type="application/javascript" src="chrome://messenger/content/preferences/subdialogs.js"/>

Why scripts at the bottom?

Comment 9

[Richard Marti (:Paenglab)]

Comment on attachment 9039384 [details] [diff] [review]
1522778-preferences-bindings-1.diff

Tested on all platforms. When a other than the "General" pane is selected the last time and I now open the prefs, I see first for around half a second the "General" pane, then it switches to the pane that was selected the last time. Didn't we had the same problem the lightning pane when the overlay loader was introduced? Fixing this is okay for a follow-up bug.

Now to the Mac prefs opened from the Dock: the subdialog doesn't open through the dock menu. Only the pref is opened and the "General" pane is shown. But this is already the case before this patch.

When the TB window is hidden, the the Dock menu has no effect, but this is already with TB 60 the case.

So for me ui-r+

Comment 10

[:aceman]

Just opening preferences tab with this patch, I get new errors in console:
ReferenceError: reference to undefined property "value" in display.js:139:5
preference.setElementValue is not a function display.js:117

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #8)

Why scripts at the bottom?

The scripts have always been at the bottom due to the way the binding was constructed, but now I want them there so the function in preferences.js can run ASAP. If it waits even until DOMContentLoaded it has to wait for the Lightning overlays which is slooowww. (Probably also the cause of the delay Richard mentioned.)

Comment 12

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #10)

Just opening preferences tab with this patch, I get new errors in console:
ReferenceError: reference to undefined property "value" in display.js:139:5
preference.setElementValue is not a function display.js:117

I've seen that but thought it had gone away. I'll look into it more, suspect it may be a timing thing.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-preferences-bindings-2.diff

We have to wait for preferences.xml to load when adding <preference>s before moving on.

Comment 14

[:aceman]

Why does this help with bug 1523074? Different timing now? Plain .js code run at different time than the original XBL bindings?

Comment 15

[Geoff Lankow (:darktrojan)]

Probably that, and because we're not adding custom elements (<radio>) to anonymous content.

Comment 16

[:aceman]

I still get "ReferenceError: reference to undefined property "value" in display.js:145:5" sometimes.
Try e.g. opening Account manager -> account -> Return receipts -> Global preferences.
Preftab opens in the background but isn't accessible as Account manager is modal. The prefs window in the past could open above it.

Now when you close AM, you get "TypeError: document.getElementById(...) is null in preferences.js:131:7".

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-preferences-bindings-3.diff

That didn't work anyway, but it should be fixed.

Comment 18

[Geoff Lankow (:darktrojan)]

I missed changing the comment. :(

Comment 19

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-preferences-bindings-4.diff

Comment 20

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #16)

Try e.g. opening Account manager -> account -> Return receipts -> Global preferences.
Preftab opens in the background but isn't accessible as Account manager is modal. The prefs window in the past could open above it.

That's why we should move the Account manager into the prefs tab.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9039447 [details] [diff] [review]
1522778-preferences-bindings-4.diff

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

Looks good to me!

::: mail/base/content/macMessengerMenu.js
@@ +60,2 @@
>    } else {
> +    Services.ww.registerNotification(new PrefWindowObserver());

PrefWindowObserver - looks like this can potentially be removed now

Comment 22

[:aceman]

Yes it is a matter of timing.
I still get "ReferenceError: reference to undefined property "value"[Learn More] display.js:145:5" when I have debugging output about what initializes, from radio.xml to the error console.

Comment 23

[Geoff Lankow (:darktrojan)]

Really? I thought that would go away when we waited for the earlier binding to load. :(

I'm going to have to cheat to fix that one, because it needs to return synchronously.

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-preferences-bindings-5.diff

Try this, aceman.

Comment 25

[:aceman]

Comment on attachment 9039655 [details] [diff] [review]
1522778-preferences-bindings-5.diff

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

Much better now, thank you. I hope the hack does not hide some generic problem.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: 1522778-preferences-bindings-5.diff updated after bug 1520643

Updated patch that applies after bug 1520643's changes of imports in preferences.js.

Comment 28

[Magnus Melin [:mkmelin]]

Thx Richard, forgot about that.

Comment 29

[Jorg K (CEST = GMT+2)]

It's on my radar, but since the tree is currently busted, I'm not intending to land this right now.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039655 [details] [diff] [review]
1522778-preferences-bindings-5.diff

We need to fix this in TB 66 beta.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1f1658927d2d
Remove prefwindow and preftab bindings. r=aceman

Comment 32

[Ian Neal]

(In reply to Pulsebot from comment #31)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1f1658927d2d
Remove prefwindow and preftab bindings. r=aceman

Please backout these changes now as you have just removed shared content with SeaMonkey in common/bindings/preferences.xml

Comment 33

[Magnus Melin [:mkmelin]]

Sorry Ian, common/ is dead, we can't keep suite stuff there just for the point of it. You'll need to move it into suite/ if you need it for something. There is likely little point with that though, as the porting effort is significant, in this case it's not even porting, but close to impossible (see dependent bug).

Comment 34

[Ian Neal]

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

Sorry Ian, common/ is dead, we can't keep suite stuff there just for the point of it. You'll need to move it into suite/ if you need it for something. There is likely little point with that though, as the porting effort is significant, in this case it's not even porting, but close to impossible (see dependent bug).
It would have been courteous to have at least cc'd some SM developers (which usually does happen).

Comment 35

[Magnus Melin [:mkmelin]]

Sure. I'd be curious to know if there is even an effort to port any of this to SeaMonkey? At current change rates I'd estimate you need at least 2-3 people working full time to have any chance of surviving. At the moment, there are roughly a handful of checkins for SeaMonkey per month. That needs to be the daily rate to even stay near the surface...

Comment 36

[Jorg K (CEST = GMT+2)]

Sorry to add insult to injury. SM has received great help from the TB teal in the past. Whenever there is bustage, we try to cover SM as well which in itself is risky business since we can't test.

Factually SM is dead on trunk, as far as I'm aware you haven't even removed overlays which won't load now. I don't even what to talk about XBL. Magnus gave you an estimate of how many developers you need to keep SM running, but he didn't give an estimate of what it would take to work through your huge backlog. Asking for a backout to mitigate item 867 on you backlog list to drop back to 866 is just not a realistic suggestion.

This was a blocker for TB 66 beta which I intend to ship real soon now.

Please don't be me wrong, I very much appreciate the collaboration of FRG, Bill and yourself and whoever else is left from the SM team (now that Neil joined the Owl) effort. We try to help wherever we can, but that doesn't mean that we can help something that already totally dysfunctional.

Comment 37

[Ian Neal]

Yes, we all try to help each other, and sometimes we are busy and forget to include the other. Just sometimes, when you see something big land that removes shared code, one can get a bit overexcited. I would say anything that isn't Firefox does not have an easy life on trunk.

Comment 38

[:aceman]

This removed "prefwindow" binding, but we still use that in some prefs subdialogs. See https://dxr.mozilla.org/comm-central/search?q=prefwindow+%2Bpath%3Acomm&redirect=false
Now e.g. their onpaneload handlers do not run (which init the panes) and the dialogs are broken.

Comment 39

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-onpaneload-1.diff

:(

Comment 40

[:aceman]

Comment on attachment 9041307 [details] [diff] [review]
1522778-onpaneload-1.diff

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

Thanks, that's better.
But buttons missing in sanitize dialog:
TypeError: document.documentElement.getButton is not a function in sanitizeDialog.js:51:5

Comment 41

[Jorg K (CEST = GMT+2)]

Wow, I came to uplift this and I see that it's not finished yet :-( - So what do I do now?

Comment 42

[Jorg K (CEST = GMT+2)]

TB 66 beta:
https://hg.mozilla.org/releases/comm-beta/rev/96ed115c265a

I landed what we had, but please provide a patch for the missing work soon.

Comment 43

[Geoff Lankow (:darktrojan)]

Attachment: 1522778-onpaneload-2.diff

Comment 44

[Geoff Lankow (:darktrojan)]

Open a new bug next time please, aceman. :-/

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 9042434 [details] [diff] [review]
1522778-onpaneload-2.diff

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

OK, the sanitise dialogue works now, however, I see:
JavaScript strict warning: chrome://global/content/bindings/dialog.xml, line 94: ReferenceError: reference to undefined property "_buttons"

And when clicking the "Clear Now" button I get:
Error sanitizing history: TypeError: PlacesUtils.history.removeVisitsByTimeframe is not a function :-(

OK, time for a new bug.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/88730121e453
Fix broken loading of preference tab subdialogs. r=aceman,jorgk DONTBUILD

Comment 47

[Jorg K (CEST = GMT+2)]

File bug 1526304 for the "Clear Now" failure.

Comment 48

[Jorg K (CEST = GMT+2)]

TB 66 beta:
https://hg.mozilla.org/releases/comm-beta/rev/6dbb5631f6aef9ce800b7715dafd332c8a043910

Comment 49

[:aceman]

Comment on attachment 9042434 [details] [diff] [review]
1522778-onpaneload-2.diff

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

::: mail/components/preferences/attachmentReminder.xul
@@ +12,5 @@
>  <!ENTITY % sendOptionsDTD SYSTEM "chrome://messenger/locale/preferences/attachmentReminder.dtd">
>  %sendOptionsDTD;
>  ]>
>  
>  <prefwindow id="attachmentReminderOptionsDialog" type="child"

Why was only sanitize converted to <dialog> and not these others? Do we have <prefwindow> defined anywhere?

Comment 50

[Jorg K (CEST = GMT+2)]

Hmm, sanitise is a stand-alone window, the remaining ones:
https://searchfox.org/comm-central/search?q=%3Cprefwindow&case=false&regexp=false&path=
appear to be subdialogues of the overall pref window. I tried colours and connections and they still work in 66 beta we've just built. Geoff, can you please clarify and file a new bug if necessary.

Comment 51

[Geoff Lankow (:darktrojan)]

They're not real dialogs any more, just documents in an iframe. XUL doesn't care what the element is named.

Comment 52

[Jorg K (CEST = GMT+2)]

Hmm, maybe room for a clean-up in a new bug ;-)

Comment 53

[:aceman]

Then how do the 'onload' and 'ondialogaccept' and 'dlgbuttons' attributes work, if it isn't a dialog?
But we have a ton of css applied to <prefwindow> so migrating would be a lot of churn.

m-c has <dialog class="prefwindow"> in its tests and in https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/colors.xul . But maybe we would then have to convert more to the m-c style of doing things in prefs.

Comment 54

[Dave Royal]

I'm updating a xul addon which has preferences including buttons:
https://github.com/revad/use_bcc_instead_C/blob/master/chrome/content/AddonOptions.xul

The buttons no longer show (67.0a1 2019-02-21). Is that because of this change? If so what to do? One of the buttons displays help text.

Comment 55

[Onno Ekker [:nONoNonO UTC+1]]

(In reply to Dave Royal from comment #54)

I'm updating a xul addon which has preferences including buttons:
https://github.com/revad/use_bcc_instead_C/blob/master/chrome/content/AddonOptions.xul

The buttons no longer show (67.0a1 2019-02-21). Is that because of this change? If so what to do? One of the buttons displays help text.

That is probably because of the inContent options, which now instantApply. That removes the OK and Cancel buttons.

Comment 56

[Dave Royal]

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

The cancel button is expendable, it's the 'extra' button I'd like to retain.

Comment 57

[Geoff Lankow (:darktrojan)]

(In reply to Dave Royal from comment #54)

I'm updating a xul addon which has preferences including buttons:
https://github.com/revad/use_bcc_instead_C/blob/master/chrome/content/AddonOptions.xul

The buttons no longer show (67.0a1 2019-02-21). Is that because of this change? If so what to do? One of the buttons displays help text.

I'm switching our <prefwindow>s to <dialog>s as part of bug 1527770. I'm also moving away from <preferences> and <preference>, so you might want to do the same.