970511 - Allow users to undo the "restore default" action

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Right now, resetting toolbars to default in customize mode results in an immediate reset.

I think it'd be a good idea to double-check with the user that this is something they really want to do, with a confirmation dialog.

We're past the string freeze, so if Australis ships on the 29 train, this will have to wait a version.

Comment 1

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

We should do an "undo" instead of a confirm.

Comment 2

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

Oh, yes, I prefer that as well.

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] from comment #1)
> We should do an "undo" instead of a confirm.

That'd be preferred, but I think there's definitely a implementation cost trade-off between a simple confirmation prompt (horribly intrusive, of course) and a undo/ rollback solution.

With that taken into consideration, the prompt might still be preferable.

Comment 4

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

Well, we have all of the state in the browser.uiCustomization.state pref, so it would be as simple as storing a copy of that pref in memory while we are showing the undo option.

Comment 5

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

I'm hoping we can reuse the undoCmd.label string here for uplift.

Comment 6

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

Attachment: WIP Patch

Gijs, what do you think about this? It's not working yet, the buttons don't move back to their old position, do you see why?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8374367 [details] [diff] [review]
WIP Patch

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

Worked this one out over IRC.

Comment 8

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

Attachment: Patch

Madhava said to go with the "Undo" link next to the disabled "Restore Defaults" button.

Comment 9

[Mike de Boer [:mikedeboer]]

I'm sorry, but this solution seems incomplete to me... 'Undo' is tightly coupled with an action tracker which keeps track of all your actions and allows you to undo AND redo any one of them. I'm pretty sure quite a number of users will think the same.

Possible bugs I foresee filed will go something like 'Undo stays disabled, even when I moved an item' or 'Can not undo action in Customize Mode, even though feature seems available'.

The only solution I can come up with to remedy confusion is to change the label of the button/ link, e.g. 'Undo Restore'. I know, a string change. :/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8374448 [details] [diff] [review]
Patch

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

f+ because the button should be reset when you exit customize mode. Also, I'm proposing a different way of abstracting where we decide whether to show the button. Let me know what you think.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2871,5 @@
> +
> +  /**
> +   * Undo the previous reset, can only be called immediately after a reset.
> +   */
> +  undoReset: function() {

These should both be documented in jsdoc comments, and on MDN

@@ +2875,5 @@
> +  undoReset: function() {
> +    CustomizableUIInternal.undoReset();
> +  },
> +
> +  canUndoReset: function() {

This should be a getter.

More importantly, Madhava said on IRC that we should reset this when leaving customize mode. I agree.

The hard part is how to implement this. It's kind of weird that CUI has this state right now. It's customizemode that wants to allow you to undo this. But making CustomizeMode know internal details of how undoReset works is also icky.

I *think* that the right thing to do would be for CUI to always have the backup when reset() is called, and have CustomizeMode just control when you "allow" the user to use it, rather than having things inside CUI setting gUIStateBeforeReset to null (also because in theory add-ons could do stuff after a reset and before you hit 'undo' - in which case I don't think they should be allowed to 'take away' the undo option from the user). You could reset the 'is this allowed' flag after onDrop or similar in CustomizeMode, as well as when exiting customize mode.

Does that make sense or do you disagree? :-)

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +7,5 @@
>  this.EXPORTED_SYMBOLS = ["CustomizeMode"];
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
> +const kPrefCustomizationState = "browser.uiCustomization.state";

You don't seem to use this?

@@ +841,5 @@
>      // Disable the reset button temporarily while resetting:
>      let btn = this.document.getElementById("customization-reset-button");
>      BrowserUITelemetry.countCustomizationEvent("reset");
>      btn.disabled = true;
> +

Nit: no need for a newline

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +104,5 @@
>                inset 0 1px rgb(196, 196, 196);
>  }
>  
> +#customization-undo-reset {
> +  padding: 6px 12px;

Argh. So here we have the classic problem with these kinds of UIs... how do you make this line up with the 'real' button. It's not perfect on OS X (by like 1-2px or less). I'm not sure if we care enough to fix it. I can live with this, but if you have ideas, let me know...

Comment 11

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

Attachment: Patch (unbitrotted)

We can't change the string, but it would be great if we can ship *something* and maybe get a better solution for AustralisV2.

Comment 12

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> I'm sorry, but this solution seems incomplete to me... 'Undo' is tightly
> coupled with an action tracker which keeps track of all your actions and
> allows you to undo AND redo any one of them. I'm pretty sure quite a number
> of users will think the same.
> 
> Possible bugs I foresee filed will go something like 'Undo stays disabled,
> even when I moved an item' or 'Can not undo action in Customize Mode, even
> though feature seems available'.

I'm not sure I understand. The link only shows when you click reset, and hides when you click it. How does that lead to "undo stays disabled even when I moved an item" ?

Plus, moving an item yourself is easy to undo. Restore defaults is the nuclear option. Depending on what the user has done, restoring it might be very time-intensive.

Comment 13

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

Attachment: Patch v2

I don't like the idea of CustomizeMode controlling whether the undo button is shown. A lot can happen purely through CustomizableUI methods, and CustomizeMode wouldn't know.

I tweaked the padding so that it will match on OSX, although the previous value that I had of 6px padding-top should have been correct on OSX according to my screenshots. I needed to adjust Windows to have a padding-top of 7px. Linux is good with the 7px padding.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8374612 [details] [diff] [review]
Patch v2

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +3046,5 @@
>     * Customize Mode only, do not use otherwise.
>     * @param aWindow the window exiting customize mode
>     */
>    notifyEndCustomizing: function(aWindow) {
> +    gUIStateBeforeReset = null; /* Hack, need to find a better place for this. */

:-(

Don't know where it should go, sadly.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +110,5 @@
> +%ifdef XP_MACOSX
> +  padding-top: 6px;
> +%else
> +  padding-top: 7px;
> +%endif

Did you intend to leave out padding-bottom?

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8374612 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8374612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +3046,5 @@
> >     * Customize Mode only, do not use otherwise.
> >     * @param aWindow the window exiting customize mode
> >     */
> >    notifyEndCustomizing: function(aWindow) {
> > +    gUIStateBeforeReset = null; /* Hack, need to find a better place for this. */
> 
> :-(
> 
> Don't know where it should go, sadly.

Actually, I guess I would even prefer a separate method. Still seems really ugly, however... :-(

Comment 16

[Dão Gottwald [:dao]]

Why was "Undo" made a link? Seems like this should be a button.

Comment 17

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

(In reply to Dão Gottwald [:dao] from comment #16)
> Why was "Undo" made a link? Seems like this should be a button.

http://logbot.glob.com.au/?c=mozilla%23fx-team&s=11+Feb+2014&e=11+Feb+2014&h=link#c103049

Comment 18

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #17)
> (In reply to Dão Gottwald [:dao] from comment #16)
> > Why was "Undo" made a link? Seems like this should be a button.
> 
> http://logbot.glob.com.au/?c=mozilla%23fx-
> team&s=11+Feb+2014&e=11+Feb+2014&h=link#c103049

So consistency with Panorama, or some other reason that I missed? Things that execute action are usually buttons, whereas hyperlinks in the Firefox UI most of the time open web pages. There are some exceptions, but they're usually considered bugs (like in the Sync pref pane).

Comment 19

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

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to Jared Wein [:jaws] from comment #17)
> > (In reply to Dão Gottwald [:dao] from comment #16)
> > > Why was "Undo" made a link? Seems like this should be a button.
> > 
> > http://logbot.glob.com.au/?c=mozilla%23fx-
> > team&s=11+Feb+2014&e=11+Feb+2014&h=link#c103049
> 
> So consistency with Panorama, or some other reason that I missed?

It's consistency with the New Tab Page thumbnails.

(In reply to :Gijs Kruitbosch from comment #14)
> 
> Did you intend to leave out padding-bottom?

Yeah, setting a padding-bottom had no effect.

As discussed on IRC, I switched to using the onCustomizedEnd listener for removing the gUIStateBeforeReset.

https://hg.mozilla.org/integration/fx-team/rev/0d95354a2ad5

Comment 20

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #18)
> > (In reply to Jared Wein [:jaws] from comment #17)
> > > (In reply to Dão Gottwald [:dao] from comment #16)
> > > > Why was "Undo" made a link? Seems like this should be a button.
> > > 
> > > http://logbot.glob.com.au/?c=mozilla%23fx-
> > > team&s=11+Feb+2014&e=11+Feb+2014&h=link#c103049
> > 
> > So consistency with Panorama, or some other reason that I missed?
> 
> It's consistency with the New Tab Page thumbnails.

Thanks for the response, but this ignored the rest of what I wrote. You shouldn't have landed this in the middle of this conversation. For the time being, I filed bug 971825.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d95354a2ad5

Comment 22

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

Comment on attachment 8374612 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature for Australis
User impact if declined: users can't recover data loss
Testing completed (on m-c, etc.): on m-c and includes tests
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/9a6a11fe8658

Comment 24

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

(In reply to :Gijs Kruitbosch from comment #10)
> > +  undoReset: function() {
> 
> These should both be documented in jsdoc comments, and on MDN

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm$revision/523025