701725 - Enable the ability to undo a closed tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Cristian Nicolae (:xti)]

Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111111
Firefox/10.0a1 Fennec/10.0a1
Devices: Motorola Droid 2
OS: Android 2.3.3

Steps to reproduce:
1. Open Fennec App
2. Open a new tab and go to www.google.com
3. Tap on Tab Menu button
4. Close the tab opened at step 2
5. Undo the closed tab at step 4

Expected result:
The position of the closed tab remains the same at it was before step 5. Instead of " X " button, now there is a counterclockwise sign.

Actual result:
After step 4, the tab is permanently closed. Step 5 cannot be performed.

Comment 1

[Madhava Enros [:madhava]]

We need to figure out the user flow here. Probably similar to what we do on the tablet.

Comment 2

[Madhava Enros [:madhava]]

My mistake - we should do what we'd designed for the tablet in portrait (didn't make it in).

Comment 3

[Ian Barlow (:ibarlow)]

Attachment: Mockup of an "undo closed tab" state looks like, which is the same as the intended behaviour on tablet.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

madhava, what's mozilla confidential about this?

Comment 7

[Madhava Enros [:madhava]]

The design would have to be updated for the new tabs UI -- I feel like I've seen these somewhere, but can't track it down. Ian?

Comment 8

[Ian Barlow (:ibarlow)]

Soft blocker nom?

Not sure if I have a mockup for this with the new tabs UI, but I can put one together easily enough if we mark this as a blocker. It'd be great to get in for a v1.0 if possible.

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Had a user ask about this in the forums. Do we want to track this?

Comment 11

[Brian Nicholson (:bnicholson)]

Ian, have any ideas for this? Maybe an undo button at the top of the tabs tray that shows a list of recently closed tabs?

Comment 12

[Lucas Rocha (:lucasr)]

I'd probably wait for the new tabs design to be in place before implementing this. Otherwise we might end up implementing this twice (once for the old and another for the new design).

Comment 13

[Paul [pwd]]

What is the bug number for the new tabs? Can that bug block this one?

Comment 14

[Ian Barlow (:ibarlow)]

We will likely follow an approach that matches the Android pattern (below) pretty closely. After a tab is deleted (or when a user clears all their tabs, once that feature is implemented), a little message like this will appear. 

 ---------------------------------
| 1 tab closed        | <- UNDO   |
 ---------------------------------

Bugs for the tab tray updates are coming soon. As will designs for the undo feature.

Comment 15

[Sriram Ramasubramanian [:sriram]]

Omg! I was just thinking of the same thing. Like in Gmail.
If anyone's interested, it's a Toast message that's shown, with a custom layout.
http://developer.android.com/reference/android/widget/Toast.html#setView%28android.view.View%29 <-- This could show a custom view.

Comment 16

[Brian Nicholson (:bnicholson)]

I believe this should actually block bug 817675 - the tab refinements bug - as I don't think this is directly related to bug 817716 (closing all tabs).

Comment 17

[Thomas Stache]

These Toasts always cover up what you're looking at, and would offer only a transient possibility to get back at a closed tab.
I think a persistent location should be provided, for instance on the Home Screen. (This would incidentally be similar to what Chrome/Android is doing, they have it on the New Tab screen)

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Finkle, why are we tracking this for 19?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Not needed for Fx19

Comment 20

[Robert Kaiser]

In XUL Fennec, we showed a dimmed tab thumbnail at the bottom of the (vertical) tab strip, with a green reload-style icon painted over. At least on the tablet UI, this would work nicely as well in the current native version.
Esp. with the UI that now allows swiping away tabs to close them, it's easy to do s light flick instead of a tap on a thumbnail and end up swipe-closing it away accidentally. Being able to undo would be really good there, but I also often end up closing a tab and a few seconds later remembering there was something else I wanted to check on that site - getting it back fast would be great there as well.

Comment 21

[Ian Barlow (:ibarlow)]

Let's stick to the plan in comment 14 of doing a more Androidy 'undo toast' for now

Comment 22

[Lucas Rocha (:lucasr)]

This is a non-trivial feature, not really a UI polishing bug. Removing ui-hackathon.

Comment 23

[Brian Nicholson (:bnicholson)]

Unassigning myself since I have no plans to work on this in the immediate future. Bug also needs some finalized mockups.

Comment 24

[Russell Haley]

I'd just like to chime in and say I am also affected by this bug.  Fatfingering the tab list should not be a destructive action.

Comment 25

[Peter Retzer (:pretzer)]

This should be easier to do now with the SuperToast/ButtonToast work Wes did in bug 872388, right? Maybe someone even wants to mentor it?

(In reply to Brian Nicholson (:bnicholson) from comment #23)
> Bug also needs some finalized mockups.

Given the above I don't think this needs further design work. The toast design is given and Ian provided the strings in comment 14.

Comment 26

[Wesley Johnston (:wesj)]

I don't think we'd use super toast for this. We'd probably show the undo in the same place as the row that slid out, similar to gmail. Its non-trivial (but probably fun to work on!) We could make this a slightly more advanced mentor bug if someone is willing to work with UX and get it done.

Comment 27

[Ian Barlow (:ibarlow)]

(In reply to Wesley Johnston (:wesj) from comment #26)
> I don't think we'd use super toast for this. We'd probably show the undo in
> the same place as the row that slid out, similar to gmail. 

Gmail has it in the row when you swipe, but when you tap the archive icon they do a supertoasty kind of thing which is what I originally proposed in this bug, too

 ---------------------------------
| 1 tab closed        | <- UNDO   |
 ---------------------------------

I'd be open to either. Would be great to see both to compare how the interactions feel.

Comment 28

[:Margaret Leibovic]

We decided bug 817716 was something we want for Fx32, so I think it would be nice to try to land this at the same time. It sounds like this just needs some engineering effort.

Comment 29

[:Margaret Leibovic]

Attachment: WIP - undo close tab super toast

This still needs to be localized, but I want to know what you think.

I also want to think a bit more about how we would create an undo action for "close all tabs". This approach wouldn't actually work, because session restore only holds onto one tab to undo closing :/

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #29)
> Created attachment 8416222 [details] [diff] [review]
> WIP - undo close tab super toast
> 
> This still needs to be localized, but I want to know what you think.

Looks nice.

> I also want to think a bit more about how we would create an undo action for
> "close all tabs". This approach wouldn't actually work, because session
> restore only holds onto one tab to undo closing :/

I think the SessionStore can retrieve the full session, or it could. You could grab that and use it to restore if the user wants to "undo" the "close all".

Comment 31

[Brian Nicholson (:bnicholson)]

Comment on attachment 8416222 [details] [diff] [review]
WIP - undo close tab super toast

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

Looks good, though most of the changes here are for adding an allowUndo boolean, and I don't see the reason. Can you explain?

::: mobile/android/base/Tabs.java
@@ +303,5 @@
>          closeTab(tab, getNextTab(tab));
>      }
>  
> +    public synchronized void closeTab(Tab tab, Tab nextTab) {
> +        closeTab(tab, nextTab, false);

Why false by default?

@@ +306,5 @@
> +    public synchronized void closeTab(Tab tab, Tab nextTab) {
> +        closeTab(tab, nextTab, false);
> +    }
> +
> +    public synchronized void closeTab(Tab tab, boolean allowUndo) {

What's an example of when we don't want to allow an undo? Maybe it's obvious, but I can't think of any situations where closing a tab can't be undone.

Comment 32

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #31)

> ::: mobile/android/base/Tabs.java
> @@ +303,5 @@
> >          closeTab(tab, getNextTab(tab));
> >      }
> >  
> > +    public synchronized void closeTab(Tab tab, Tab nextTab) {
> > +        closeTab(tab, nextTab, false);
> 
> Why false by default?

I wanted to only change the behavior of the closeTab call in TabsTray, without affecting the other consumers.

> @@ +306,5 @@
> > +    public synchronized void closeTab(Tab tab, Tab nextTab) {
> > +        closeTab(tab, nextTab, false);
> > +    }
> > +
> > +    public synchronized void closeTab(Tab tab, boolean allowUndo) {
> 
> What's an example of when we don't want to allow an undo? Maybe it's
> obvious, but I can't think of any situations where closing a tab can't be
> undone.

There are other places we automatically close tabs, like when you hit the back button and we go back to a parent tab. We also call Tabs#closeTab in GeckoView, and we probably wouldn't want a toast to appear there.

I don't love boolean flags, so it would be nice to avoid this if we could. However, if we do decide to go with this approach, maybe I should rename it something like "showUndoToast" to make it more obvious what it's controlling.

Or alternately, I could make a separate method for this, and TabsTray could call that itself.

At one point I was trying to make a native ButtonToast in TabsTray, but that was actually hard because we maintain a ButtonToast instance (and helper methods) in GeckoApp, but we couldn't get at that from TabsTray without making some other architectural changes. But I like the abstraction of just using the JS API.

Comment 33

[Brian Nicholson (:bnicholson)]

(In reply to :Margaret Leibovic from comment #32)
> There are other places we automatically close tabs, like when you hit the
> back button and we go back to a parent tab. We also call Tabs#closeTab in
> GeckoView, and we probably wouldn't want a toast to appear there.

In certain situations, I'd actually find the ability to undo these *more* useful. If I click a link from Twitter, browse to several other pages in the same tab, then go back through my history a bit later (forgetting that this tab was "external"), the tab gets close unexpectedly on me with no way to recover it.

I agree that a toast probably wouldn't be too helpful in these cases (and even if we wanted one, our current super toast implementation probably doesn't show them outside the app anyway). But the ability to undo closing these tabs could definitely be useful since the tab close isn't a user-initiated action!

Comment 34

[:Margaret Leibovic]

Cc'ing Yuan, since she's been working on tab interaction design.

I think that bug 1004850 will help address a lot of the concerns in this bug. I worry that adding a toast any time any tab is closed could get annoying, especially if we do have a place in the UI to get back at those tabs.

Maybe instead of a super toast, we should look into the gmail-style inline undo. Although that will have a similar problem to the idea proposed in bug 817716, where this might not look right in the horizontal tabs tray.

Comment 35

[Paul [pwd]]

My proposal is that we add a tray context to Undo Last Closed Tab and then also can we have a toast that was say "Tab Closed | Undo", however should we close multiple tabs quickly, that Undo would change to "Show All" and we can have Recently Closed Tabs in the AwesomeScreen alongside History and Recent Tabs.

Comment 36

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #34)

> Maybe instead of a super toast, we should look into the gmail-style inline
> undo. Although that will have a similar problem to the idea proposed in bug
> 817716, where this might not look right in the horizontal tabs tray.

As I mentioned in comment 27, can we make builds with both options -- an inline Undo function, and a supertoast undo function, and try them both to compare? 

This is a pretty difficult decision to make in a bugzilla discussion without something usable to interact with :)

--

As for how to handle it in a horizontal tray, we could probably stack the layout instead of making it horizontal. So something like:



Portrait

-------------------------------
 1 tab closed        | <- UNDO   
-------------------------------


Landscape

|                    |
|   1 tab closed     |
|                    |
|      <- UNDO       |
|                    |

Comment 37

[Elbart]

Chrome for Android 35 comes with undo:
http://www.androidpolice.com/2014/05/20/chrome-stable-hits-version-35-brings-undo-close-tab-better-fullscreen-video-support-and-more/

Comment 38

[:Margaret Leibovic]

(In reply to Elbart from comment #37)
> Chrome for Android 35 comes with undo:
> http://www.androidpolice.com/2014/05/20/chrome-stable-hits-version-35-brings-
> undo-close-tab-better-fullscreen-video-support-and-more/

Super toast!

The super toast approach is definitely easier to implement than the in-place approach, so I would prefer to just move forward with that, especially if we're considering a bigger revamp of the tabs tray in the near future. I think we were actually pretty happy with that approach, but then I was confusing the indecision in bug 817716 with indecision in this bug.

I think we just need to decide if we should only show this when closing tabs from the tabs tray (what my current patch does) or if we should show it when any tab closes (as bnicholson suggested). Given that we'll give users a place to get back to recently closed tabs in bug 1004850, I think this undo toast should only happen when the user explicitly closes the tab themsevles in the tabs tray.

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

I agree with this approach, especially as a first phase:
* Super toast only
* Explicit tab close only

Comment 40

[:Margaret Leibovic]

Attachment: Create "undo close tab" super toast

I updated my previous patch to make it more clear that the extra option is just about showing a toast, not "allowing" undo.

I found the button toast message wrapped weirdly with long page titles, so I just updated its style to ellipsize at one line. I don't think we'd ever want a multi-line super toast, and this was easier than trying to figure out where to manually ellipsize this one message.

Comment 41

[:Margaret Leibovic]

One potential issue with this patch is that is that if you close a lot of tabs in a row, these toasts queue up and really fall behind the user interaction.

Looking at Chrome, they replace the current toast with a new one as soon as a new tab is closed. Not sure if we have a way to do that with our current JS toast API.

Comment 42

[Brian Nicholson (:bnicholson)]

Comment on attachment 8427309 [details] [diff] [review]
Create "undo close tab" super toast

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

::: mobile/android/chrome/content/browser.js
@@ +955,5 @@
>  #endif
>  
>    // Calling this will update the state in BrowserApp after a tab has been
>    // closed in the Java UI.
> +  _handleTabClosed: function _handleTabClosed(aTab, aShowUndoToast) {

I'm a bit torn about routing this through JS. It seems more straightforward to me to just show the toast directly in Java instead of passing this boolean around to JS, only to have JS go back to Java to show the toast. But as you said, ButtonToast isn't really designed to be used outside of GeckoApp as is, so we can save that for another day.

@@ +963,5 @@
>      let evt = document.createEvent("UIEvents");
>      evt.initUIEvent("TabClose", true, false, window, null);
>      aTab.browser.dispatchEvent(evt);
>  
> +    let title = aTab.browser.contentDocument.title;

Pages don't always have a title, so you should probably use the URL as a fallback.

Comment 43

[Paul [pwd]]

(In reply to :Margaret Leibovic from comment #41)
> One potential issue with this patch is that is that if you close a lot of
> tabs in a row, these toasts queue up and really fall behind the user
> interaction.
That was why I suggested that should tabs be closed in quick succession, we switch from an individual

_____________________
 title closed   | undo
_____________________

to a more global

______________________
 tabs closed    | show all
______________________

Comment 44

[:Margaret Leibovic]

(In reply to Paul [pwd] from comment #43)
> (In reply to :Margaret Leibovic from comment #41)
> > One potential issue with this patch is that is that if you close a lot of
> > tabs in a row, these toasts queue up and really fall behind the user
> > interaction.
> That was why I suggested that should tabs be closed in quick succession, we
> switch from an individual
> 
> _____________________
>  title closed   | undo
> _____________________
> 
> to a more global
> 
> ______________________
>  tabs closed    | show all
> ______________________

Unfortunately we can't technically do that right now, since we only store the most recently closed tab. Perhaps this can be a follow-up to fix after we finish bug 1004850.

Comment 45

[:Margaret Leibovic]

Pushed to try to make sure this doesn't break anything:
https://tbpl.mozilla.org/?tree=Try&rev=899c01436c0b

Comment 46

[:Margaret Leibovic]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

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

Comment 48

[:Margaret Leibovic]

This might be worth mentioning in the release notes.

Comment 49

[Lukas Blakk [:lsblakk] use ?needinfo]

please use the relnote-firefox flag (set to ?) to help bring this to release manager attention.

Comment 50

[:Margaret Leibovic]

This was disabled in Fx32 by bug 1023406.

We may look to re-enable it after the follow-up work bakes on Nightly.

Comment 51

[Cristina Madaras, QA [:CristinaM]]

Test cases added in Moztrap:
https://moztrap.mozilla.org/manage/case/14068/ - Undo a closed tab
https://moztrap.mozilla.org/manage/case/14069/ - Undo a closed private tab