Remove prefwindow and preftab bindings

RESOLVED FIXED in Thunderbird 67.0

Status

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

(Blocks 1 bug, {perf})

unspecified
Thunderbird 67.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird66 fixed, thunderbird67 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Assignee

Description

5 months ago

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.

Assignee

Comment 1

5 months ago

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

That would be great.

Assignee

Comment 3

5 months ago

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.

Attachment #9039384 - Flags: ui-review?(richard.marti)
Attachment #9039384 - Flags: review?(mkmelin+mozilla)

Comment 4

5 months ago

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

Assignee

Comment 5

5 months ago

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

Comment 6

5 months ago

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

5 months ago
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 ;-)
Attachment #9039384 - Flags: review?(acelists)
Assignee

Updated

5 months ago
Keywords: perf
Assignee

Updated

5 months ago
Summary: Remove leftover handlers from prefwindow binding → Remove prefwindow and preftab bindings

Comment 8

5 months ago
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 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+
Attachment #9039384 - Flags: ui-review?(richard.marti) → ui-review+

Comment 10

5 months ago

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

Assignee

Comment 11

5 months ago

(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.)

Assignee

Comment 12

5 months ago

(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.

Assignee

Comment 13

5 months ago

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

Attachment #9039384 - Attachment is obsolete: true
Attachment #9039384 - Flags: review?(mkmelin+mozilla)
Attachment #9039384 - Flags: review?(acelists)
Attachment #9039433 - Flags: ui-review+
Attachment #9039433 - Flags: review?(mkmelin+mozilla)
Attachment #9039433 - Flags: review?(acelists)

Comment 14

5 months ago

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

Assignee

Comment 15

5 months ago

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

Comment 16

5 months ago

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".

Assignee

Comment 17

5 months ago

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

Attachment #9039433 - Attachment is obsolete: true
Attachment #9039433 - Flags: review?(mkmelin+mozilla)
Attachment #9039433 - Flags: review?(acelists)
Attachment #9039445 - Flags: ui-review+
Attachment #9039445 - Flags: review?(acelists)
Assignee

Comment 18

5 months ago

I missed changing the comment. :(

Assignee

Comment 19

5 months ago
Attachment #9039445 - Attachment is obsolete: true
Attachment #9039445 - Flags: review?(acelists)
Attachment #9039447 - Flags: ui-review+
Attachment #9039447 - Flags: review?(acelists)

(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 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
Attachment #9039447 - Flags: review+

Comment 22

5 months ago

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.

Assignee

Comment 23

5 months ago

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.

Assignee

Comment 24

5 months ago

Try this, aceman.

Attachment #9039655 - Flags: review?(acelists)
Blocks: 1523074

Comment 25

5 months ago
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.
Attachment #9039655 - Flags: review?(acelists) → review+
Duplicate of this bug: 1523915
Attachment #9039447 - Attachment is obsolete: true
Attachment #9039447 - Flags: review?(acelists)

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

Thx Richard, forgot about that.

Comment 29

5 months ago

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

Keywords: checkin-needed

Comment 30

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

We need to fix this in TB 66 beta.
Attachment #9039655 - Flags: approval-comm-beta+

Comment 31

5 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1f1658927d2d
Remove prefwindow and preftab bindings. r=aceman

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED

Updated

5 months ago
Target Milestone: --- → Thunderbird 67.0

Comment 32

5 months ago

(In reply to Pulsebot from comment #31)

Pushed by mozilla@jorgk.com:
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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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).

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED

Comment 34

5 months ago

(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).

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

5 months ago

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

5 months ago

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

5 months ago

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.

Assignee

Comment 39

5 months ago
Posted patch 1522778-onpaneload-1.diff (obsolete) — Splinter Review

:(

Attachment #9041307 - Flags: review?(acelists)

Comment 40

5 months ago
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
Attachment #9041307 - Flags: feedback+

Comment 41

5 months ago

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

Flags: needinfo?(geoff)

Updated

5 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 42

5 months ago

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.

Assignee

Comment 43

5 months ago
Attachment #9041307 - Attachment is obsolete: true
Attachment #9041307 - Flags: review?(acelists)
Flags: needinfo?(geoff)
Attachment #9042434 - Flags: review?(acelists)
Assignee

Comment 44

5 months ago

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

Comment 45

5 months ago
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.
Attachment #9042434 - Flags: review?(acelists) → review+

Comment 46

5 months ago

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

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED

Comment 47

5 months ago

File bug 1526304 for the "Clear Now" failure.

Comment 49

5 months ago
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

5 months ago

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.

Assignee

Comment 51

5 months ago

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

Comment 52

5 months ago

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

Comment 53

5 months ago

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

4 months ago

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.

(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

4 months ago

(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.

Assignee

Comment 57

4 months ago

(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.

You need to log in before you can comment on or make changes to this bug.