Closed Bug 870869 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M5][qa-])

Attachments

(1 file, 1 obsolete file)

No description provided.
No longer blocks: 869104
Whiteboard: [Australis:M5]
Assignee: nobody → mconley
Status: NEW → ASSIGNED
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.
Attached patch WIP Patch 1 (obsolete) — Splinter Review
Just checkpointing some work here - but this should be enough to show the direction I'm going.
Attached patch Patch v1Splinter Review
Hey Gijs, feel comfortable reviewing this?
Attachment #751215 - Attachment is obsolete: true
Attachment #751271 - Flags: review?(gijskruitbosch+bugs)
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).
Attachment #751271 - Flags: review?(gijskruitbosch+bugs) → review+
(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...
shorlander said that he preferred hiding it in IRC, so I'll go that route.
Summary: (Australis) The "Restore Defaults" button in customization mode should get disabled if the toolbar is in the default state → (Australis) The "Restore Defaults" button in customization mode should get hidden if the toolbar is in the default state
Thanks Gijs, Landed on UX: https://hg.mozilla.org/projects/ux/rev/045da4704a6a
Whiteboard: [Australis:M5] → [Australis:M5][fixed-in-ux]
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.
Depends on: 874140
Had to land a follow-up because the resetButton getter was using the wrong element ID. :/

https://hg.mozilla.org/projects/ux/rev/21686fd23a10
https://hg.mozilla.org/mozilla-central/rev/045da4704a6a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M5][fixed-in-ux] → [Australis:M5]
Target Milestone: --- → Firefox 28
And https://hg.mozilla.org/mozilla-central/rev/21686fd23a10, which had the wrong bug #. Oops.
Whiteboard: [Australis:M5] → [Australis:M5][good first verify]
We no longer hide the button when the state is default, we just disable the button.
Whiteboard: [Australis:M5][good first verify] → [Australis:M5][qa-]
You need to log in before you can comment on or make changes to this bug.