1542844 - [macOS] All wizards displays [Done] button from the point where they open

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression in UI we ship to users.

This is a regression from bug 1498569, noticed by Ian when working on bug 1518234. Jonas, can you take a look please? We shouldn't be showing the "Done" button until we're actually done with the wizard.

Ian: if you have pointers about why this is happening that'd be helpful. Thanks for flagging this up!

Comment 1

[Jonas Allmann [:jallmann]]

I can not reproduce this on Linux, the wizard looks fine to me.

It shows "Next" and "Cancel" on the first page, "Next", "Back" and "Cancel" on the second, and "Next", "Back" and "Cancel" on the last, "Next" being inactive and turning into an active "Finish" upon completing migration.

I assume this is not what you mean by your description. Can I get some more information?

Comment 2

[:Gijs (he/him)]

Attachment: Screenshot

(In reply to Jonas Allmann [:jallmann] from comment #1)

I can not reproduce this on Linux, the wizard looks fine to me.

It shows "Next" and "Cancel" on the first page, "Next", "Back" and "Cancel" on the second, and "Next", "Back" and "Cancel" on the last, "Next" being inactive and turning into an active "Finish" upon completing migration.

I assume this is not what you mean by your description. Can I get some more information?

Sure. I see the same thing in the profile creation wizard. On current nightly, if I open the profile manager and click 'create...', I see the attached thing.

If I open the browser, and then use cmd-shift-b to open the bookmarks manager / library, then open the migration wizard from there, I see the "Done" button as well.

Perhaps this is mac-specific if you can't reproduce on linux?

Comment 3

[:Gijs (he/him)]

Looking at this again, bug 1495357 also landed recently, and is perhaps more likely to have caused this. Surkov?

Comment 4

[:Gijs (he/him)]

OK, so this is mac-specific, not restricted to the migration wizard, and caused by bug 1495357...

Jonas, sorry for the confusion!

It looks like hidefinishbutton used to be inherited into a hidden attribute on the button, and I don't see that happening anymore.

Comment 5

[alexander :surkov (:asurkov)]

there are two problems here:

• wizard-buttons CE has wrong attribute inheritance, here's a fix:

@@ -56,18 +58,18 @@ class MozWizardButtons extends MozXULEle
       if (btn) {
         btn.addEventListener("command", listener);
       }
     }
   }
 
   static get inheritedAttributes() {
     return AppConstants.platform == "macosx" ? {
-      "[dlgtype='finish']": "hidefinishbutton",
-      "[dlgtype='next']": "lastpage",
+      "[dlgtype='finish']": "hidden=hidefinishbutton",
+      "[dlgtype='next']": "hidden=lastpage",
     } : null;
   }
 
   get _markup() {
     if (AppConstants.platform == "macosx") {
       return `
         <vbox flex="1">
           <hbox class="wizard-buttons-btm">

• @hidefinishbutton attribute is changed (onPageChange method is called) after attribute inheritance was initialized (i.e. initializeAttributeInheritance called, but before the CE is connected (isConnectedAndReady is false).

Brian, I assume attributeChangedCallback should have isConnectedAndReady check, otherwise we may miss attributes changing, correct?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #5)

there are two problems here:

• wizard-buttons CE has wrong attribute inheritance, here's a fix:

@@ -56,18 +58,18 @@ class MozWizardButtons extends MozXULEle
       if (btn) {
         btn.addEventListener("command", listener);
       }
     }
   }
 
   static get inheritedAttributes() {
     return AppConstants.platform == "macosx" ? {
-      "[dlgtype='finish']": "hidefinishbutton",
-      "[dlgtype='next']": "lastpage",
+      "[dlgtype='finish']": "hidden=hidefinishbutton",
+      "[dlgtype='next']": "hidden=lastpage",
     } : null;
   }
 
   get _markup() {
     if (AppConstants.platform == "macosx") {
       return `
         <vbox flex="1">
           <hbox class="wizard-buttons-btm">

• @hidefinishbutton attribute is changed (onPageChange method is called) after attribute inheritance was initialized (i.e. initializeAttributeInheritance called, but before the CE is connected (isConnectedAndReady is false).

Brian, I assume attributeChangedCallback should have isConnectedAndReady check, otherwise we may miss attributes changing, correct?

We have a check for !this.isConnectedAndReady on the base class: https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/toolkit/content/customElements.js#93. In the case where the attribute has changed before that, it will be picked up in the initial call to inheritAttributes (triggered by the initializeAttributeInheritance call which should be called in connectedCallback. So if you fix the inheritedAttributes static getter I'd expect this will just work - does it not?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #6)

We have a check for !this.isConnectedAndReady on the base class: https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/toolkit/content/customElements.js#93. In the case where the attribute has changed before that, it will be picked up in the initial call to inheritAttributes (triggered by the initializeAttributeInheritance call which should be called in connectedCallback. So if you fix the inheritedAttributes static getter I'd expect this will just work - does it not?

wizard doesn't have delayed check in connectedCallback, so initializeAttributeInheritance is called before isConnectedAndReady returns true.

Anyway it seems that isConnectedAndReady check is redundant in attributeChangedCallback, because if inheritedAttributesCache is initialized at this point, then we should propagate inherited attributes, right?

Comment 8

[alexander :surkov (:asurkov)]

btw, hidefinishbutton is used inside wizards-button, thus it doesn't make to user attr inheritance mechanism for it, let's use it directly https://searchfox.org/mozilla-central/search?q=hidefinishbutton&path=

Comment 9

[alexander :surkov (:asurkov)]

Attachment: Bug 1542844 - All wizards displays [Done] button from the point where they open on OS X, r=bgrins

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #7)

(In reply to Brian Grinstead [:bgrins] from comment #6)

We have a check for !this.isConnectedAndReady on the base class: https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/toolkit/content/customElements.js#93. In the case where the attribute has changed before that, it will be picked up in the initial call to inheritAttributes (triggered by the initializeAttributeInheritance call which should be called in connectedCallback. So if you fix the inheritedAttributes static getter I'd expect this will just work - does it not?

wizard doesn't have delayed check in connectedCallback, so initializeAttributeInheritance is called before isConnectedAndReady returns true.

Anyway it seems that isConnectedAndReady check is redundant in attributeChangedCallback, because if inheritedAttributesCache is initialized at this point, then we should propagate inherited attributes, right?

Yeah, I suppose we could drop isConnectedAndReady to support non-deferred connectedCallback.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3433e47144e
All wizards displays [Done] button from the point where they open on OS X, r=bgrins

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f3433e47144e