Closed Bug 701725 Opened 8 years ago Closed 6 years ago

Enable the ability to undo a closed tab

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- disabled
relnote-firefox --- 33+
blocking-fennec1.0 --- -
fennec 32+ ---

People

(Reporter: xti, Assigned: Margaret)

References

(Depends on 1 open bug)

Details

(Keywords: feature, relnote, Whiteboard: [testday-20111111], tabs-ux[mentor=lucasr])

Attachments

(1 file, 2 obsolete files)

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.
We need to figure out the user flow here. Probably similar to what we do on the tablet.
Assignee: nobody → madhava
My mistake - we should do what we'd designed for the tablet in portrait (didn't make it in).
Priority: -- → P4
Assignee: madhava → mark.finkle
Group: mozilla-confidential
madhava, what's mozilla confidential about this?
Group: mozilla-confidential
Duplicate of this bug: 711574
Assignee: mark.finkle → bnicholson
Duplicate of this bug: 715209
Whiteboard: [testday-20111111] → [testday-20111111], tabs-ux
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?
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.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → -
Duplicate of this bug: 715208
tracking-fennec: --- → ?
Had a user ask about this in the forums. Do we want to track this?
tracking-fennec: ? → 20+
tracking-fennec: 20+ → 19+
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?
Flags: needinfo?(ibarlow)
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).
What is the bug number for the new tabs? Can that bug block this one?
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.
Flags: needinfo?(ibarlow)
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.
Blocks: 817716
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).
Blocks: 817675
No longer blocks: 817716
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)
Finkle, why are we tracking this for 19?
Flags: needinfo?(mark.finkle)
Not needed for Fx19
tracking-fennec: 19+ → +
Flags: needinfo?(mark.finkle)
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.
Let's stick to the plan in comment 14 of doing a more Androidy 'undo toast' for now
Whiteboard: [testday-20111111], tabs-ux → [testday-20111111], tabs-ux, ui-hackathon
This is a non-trivial feature, not really a UI polishing bug. Removing ui-hackathon.
Whiteboard: [testday-20111111], tabs-ux, ui-hackathon → [testday-20111111], tabs-ux
Unassigning myself since I have no plans to work on this in the immediate future. Bug also needs some finalized mockups.
Assignee: bnicholson → nobody
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.
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.
Attachment #574636 - Attachment is obsolete: true
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.
(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.
Flags: needinfo?(ibarlow)
Whiteboard: [testday-20111111], tabs-ux → [testday-20111111], tabs-ux[mentor=lucasr]
Hardware: ARM → All
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.
tracking-fennec: + → ?
tracking-fennec: ? → 32+
Assignee: nobody → margaret.leibovic
Attached patch WIP - undo close tab super toast (obsolete) — Splinter Review
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 :/
Attachment #8416222 - Flags: feedback?(bnicholson)
(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 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.
Attachment #8416222 - Flags: feedback?(bnicholson) → feedback+
(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.
(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!
Status: NEW → ASSIGNED
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.
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.
(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       |
|                    |
Flags: needinfo?(ibarlow)
(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.
I agree with this approach, especially as a first phase:
* Super toast only
* Explicit tab close only
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.
Attachment #8416222 - Attachment is obsolete: true
Attachment #8427309 - Flags: review?(bnicholson)
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 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.
Attachment #8427309 - Flags: review?(bnicholson) → review+
(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
______________________
(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.
Depends on: 1015421
Pushed to try to make sure this doesn't break anything:
https://tbpl.mozilla.org/?tree=Try&rev=899c01436c0b
https://hg.mozilla.org/mozilla-central/rev/df3099046034
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
This might be worth mentioning in the release notes.
Keywords: relnote
Keywords: feature
Depends on: 1017045
Depends on: 1017047
Depends on: 1017129
Depends on: 1017501
Depends on: 1017582
Depends on: 1017912
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(fennec)
Depends on: 1018337
Depends on: 1018661
Depends on: 1018417
Depends on: 1019155
Depends on: 1019735
Depends on: 1020585
Depends on: 1020730
please use the relnote-firefox flag (set to ?) to help bring this to release manager attention.
Depends on: 1023406
This was disabled in Fx32 by bug 1023406.

We may look to re-enable it after the follow-up work bakes on Nightly.
Depends on: 1030323
Blocks: 1042138
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
Flags: in-moztrap?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.