Allow users to undo the "restore default" action

VERIFIED FIXED in Firefox 29

Status

()

VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: mconley, Assigned: jaws)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete, ux-error-recovery})

29 Branch
Firefox 30
dev-doc-complete, ux-error-recovery
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P2])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
We should do an "undo" instead of a confirm.
Keywords: ux-error-recovery
Summary: Ask user to confirm before resetting toolbars to default → Allow users to undo the "restore default" action
(Reporter)

Comment 2

5 years ago
Oh, yes, I prefer that as well.
(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.
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.
I'm hoping we can reuse the undoCmd.label string here for uplift.
Status: NEW → ASSIGNED
Whiteboard: [strings][Australis:P?] → [Australis:P2]
Hardware: x86 → All
Version: Trunk → 29 Branch
Assignee: nobody → jaws
Created attachment 8374367 [details] [diff] [review]
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?
Attachment #8374367 - Flags: feedback?(gijskruitbosch+bugs)

Comment 7

5 years ago
Comment on attachment 8374367 [details] [diff] [review]
WIP Patch

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

Worked this one out over IRC.
Attachment #8374367 - Flags: feedback?(gijskruitbosch+bugs)
Created attachment 8374448 [details] [diff] [review]
Patch

Madhava said to go with the "Undo" link next to the disabled "Restore Defaults" button.
Attachment #8374367 - Attachment is obsolete: true
Attachment #8374448 - Flags: review?(gijskruitbosch+bugs)
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

5 years ago
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...
Attachment #8374448 - Flags: review?(gijskruitbosch+bugs) → feedback+
Created attachment 8374487 [details] [diff] [review]
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.
Attachment #8374448 - Attachment is obsolete: true
Attachment #8374487 - Flags: review?(gijskruitbosch+bugs)
Attachment #8374487 - Flags: review?(gijskruitbosch+bugs)

Comment 12

5 years ago
(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.
Created attachment 8374612 [details] [diff] [review]
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.
Attachment #8374487 - Attachment is obsolete: true
Attachment #8374612 - Flags: review?(gijskruitbosch+bugs)

Comment 14

5 years ago
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?
Attachment #8374612 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 15

5 years ago
(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... :-(
Why was "Undo" made a link? Seems like this should be a button.
(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
(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).
(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
Flags: in-testsuite+

Updated

5 years ago
Depends on: 971825
(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.
https://hg.mozilla.org/mozilla-central/rev/0d95354a2ad5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
status-firefox30: --- → affected
tracking-firefox30: --- → ?
status-firefox29: --- → fixed
status-firefox29: fixed → affected
status-firefox30: affected → fixed
tracking-firefox30: ? → ---
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
Attachment #8374612 - Flags: approval-mozilla-aurora?
Attachment #8374612 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
Keywords: dev-doc-complete

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Duplicate of this bug: 886450
Blocks: 1287980
You need to log in before you can comment on or make changes to this bug.