870011 - Restore Default requires a restart on UX nightly

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

The "Restore Default" functionality in the new Australis customization currently requires a restart to work correctly. It should restore the toolbars and panel menu to their default state without requiring this restart.

Comment 1

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

Just a heads up that this will also mean clearing the currentset of all toolbars, and persisting that value. We're still using currentset for now, as we want it to be relatively painless to switch back and forth between builds until this thing gets shipped.

Comment 2

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

Hey Mike, any headway on this?

Comment 3

[Mike de Boer [:mikedeboer]]

Unfortunately not, I'd like to discuss this with you tomorrow as as possible!

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Restore Default implementation

Comment 5

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

Comment on attachment 753015 [details] [diff] [review]
Restore Default implementation

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1410,5 @@
>      LOG("State reset");
> +    // HACK: when all buttons are restored to their defaults, the Customize view
> +    // is out of whack. Exiting beforehand makes that behavior not noticable.
> +    for (let [window,] of gBuildWindows) {
> +      window.gCustomizeMode.exit();

Driveby - if this is really necessary, I think we should do it in CustomizeMode.jsm, but I'm curious *why* this is necessary.

Why does it get out of whack? Can you describe this out-of-whackness? :)

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Mike Conley (:mconley) from comment #5)
> 
> Driveby - if this is really necessary, I think we should do it in
> CustomizeMode.jsm, but I'm curious *why* this is necessary.
> 
> Why does it get out of whack? Can you describe this out-of-whackness? :)

I'll ask you on IRC about the move to CustomizeMode.jsm.

Out-of-whackness: when the toolbar buttons are restored/ rebuilt, the buttons in customize mode are not draggable anymore - the drag 'n drop mode/ attributes don't apply anymore because the nodes are replaced by new ones. The state of the bookshelf button is not maintained, so clicking it after a restore will result in opening an empty popup. So here I went for the quick 'n dirty solution; just exit customize mode and when the user enters it again the UI is in the proper state.

An alternative solution would be to re-initialize the customize mode when the toolbars and panels are rebuilt. This way we do not need to exit the mode. To implement this, I would like to have a bit more information to figure out what the best way forward is.

Comment 7

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

Comment on attachment 753015 [details] [diff] [review]
Restore Default implementation

Cancelling review request since, as mentioned in IRC, we want restore default state to work without having to exit (and/or subsequently re-enter) customization mode.

The problem Mike is running into is that we need to unwrap all of the widgets before rebuilding the areas. We may also need to de-populate and re-populate the palettes.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Restore Default implementation

Comment 9

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

Comment on attachment 753718 [details] [diff] [review]
Restore Default implementation

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

Thanks Mike! r=me with the following fixes.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1428,5 @@
> +    // Restore the state for each area to its defaults
> +    for (let [area, defaultPlacements] of gDefaultPlacements) {
> +      this.restoreStateForArea(area);
> +    }
> +    

Trailing whitespace here and on next line.

@@ +1435,5 @@
> +    for (let [areaId, areaNodes] of gBuildAreas) {
> +      let placements = gPlacements.get(areaId);
> +      for (let areaNode of areaNodes) {
> +        this.buildArea(areaId, placements, areaNode);
> +        areaNode.setAttribute("currentset", placements.join(","));

Hm - we just remove this attribute again in CustomizeMode.reset after calling this, so we probably don't need to set the attribute.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +482,5 @@
> +      toolbar.removeAttribute("currentset");
> +      LOG("[RESET] Removing currentset of " + toolbar.id);
> +      // Persist the currentset attribute directly on hardcoded toolbars.
> +      document.persist(toolbar.id, "currentset");
> +    }

After this, set the hidden state of the reset button via:

this.resetButton.hidden = CustomizableUI.inDefaultState;

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Restore Default implementation

Carrying over r=mconley

Comment 11

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

Landed in UX as https://hg.mozilla.org/projects/ux/rev/bab65fae9ff6

Comment 13

[:Gijs (he/him)]

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