870869 - (Australis) The "Restore Defaults" button in customization mode should get hidden if the toolbar is in the default state

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I do wonder if we should just hide the button instead of disabling it in this case. We could set it to visibility:hidden so that reshowing it doesn't cause any jumping in the content.

It might be a more elegant approach since it may not be obvious to a user why the "restore default" button is disabled.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Just checkpointing some work here - but this should be enough to show the direction I'm going.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Hey Gijs, feel comfortable reviewing this?

Comment 4

[:Gijs (he/him)]

Comment on attachment 751271 [details] [diff] [review]
Patch v1

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

Generally, this looks good. However, the bug summary says 'disabled' and you're hiding the button. Did we change our mind? If not, I'm afraid that'd make this an r-. If we did, read on. :-)

Have you tested what happens if add-ons insert stuff using DOM manipulation or XUL overlays? From my reading of the patch, the code will still believe we're in default state. I'd argue we probably do care about this case? OTOH, even if we were to remove the custom added stuff, it'd likely be back on browser restart... so that's a tricky case. If my reasoning seems correct to you, can you file a followup bug for that?

::: browser/components/customizableui/content/customizeMode.inc.xul
@@ +6,5 @@
>    <vbox flex="1" id="customization-palette-container">
>      <label id="customization-header" value="&customizeMode.menuAndToolbars.header;"/>
>      <vbox id="customization-palette" flex="1"/>
>      <hbox pack="start">
> +      <button id="customization-reset-button" oncommand="gCustomizeMode.reset();" label="&customizeMode.restoreDefaults;" class="customizationmode-button"/>

Nit: I'd prefer this line wrapped.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +473,5 @@
>    },
>  
>    _onUIChange: function() {
>      this._changed = true;
> +    this.resetButton.hidden = CustomizableUI.inDefaultState;

At the moment, this won't fire when you click the button. We should be careful that that does happen after bug 870011 is fixed (I could imagine it'll restore state in a way that might not trigger this).

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 751271 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 751271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally, this looks good. However, the bug summary says 'disabled' and
> you're hiding the button. Did we change our mind? If not, I'm afraid that'd
> make this an r-. If we did, read on. :-)

Ah, I missed comment #1. I'm not sure I agree; I could also be wondering where the "Restore Default" button is if it's not visible. I think if we think that either user worry is likely, maybe we could replace the button with some text that says something along the lines of "Your Firefox is in its default state. Customize away!" (to be wordsmithed by someone better at that than I am, obvs.) ? Don't feel strongly either way, but may want to ask UX...

Comment 6

[Mike Conley (:mconley) (:⚙️)]

shorlander said that he preferred hiding it in IRC, so I'll go that route.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Thanks Gijs, Landed on UX: https://hg.mozilla.org/projects/ux/rev/045da4704a6a

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Setting .hidden=true will cause the element to be display:none.

This patch should have used .style.visibility="hidden" so that the containing <hbox> doesn't shrink to a height of 0. Keeping the height unchanged will be important to making sure that the positions of items in the palette don't shift when the button changes visibility state.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Had to land a follow-up because the resetButton getter was using the wrong element ID. :/

https://hg.mozilla.org/projects/ux/rev/21686fd23a10

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/045da4704a6a

Comment 11

[:Gijs (he/him)]

And https://hg.mozilla.org/mozilla-central/rev/21686fd23a10, which had the wrong bug #. Oops.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

We no longer hide the button when the state is default, we just disable the button.