608037 - Undo close tab in Panorama UI should keep you in Panorama UI

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Kevin Hanes]

Hitting undo close tab in the Panorama UI should reopen the tab, but keep the user in Panorama. This would keep the interaction similar to "undo close group" and prevent a "UI that moves in the night".

Currently you are placed directly in the tab (which has been placed in whatever group you have currently selected).

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Ian Gilman [:iangilman]]

Comment on attachment 490055 [details] [diff] [review]
v1

I wonder if it would be better to do this by watching SSTabRestoring events.  Any thoughts?  

>+      if (this.restoredClosedTab) {
>+        tab.linkedBrowser.addEventListener("load", function (event) {
>+          tab.linkedBrowser.removeEventListener("load", arguments.callee, true);
>+          // update canvas and fav icon for this tab in tabview UI.
>+          TabItems._update(tab);
>+        }, true);
>+      }

I assume this is because our normal update mechanism isn't working in this case.  Do you know why?  Perhaps we should fix the root cause instead.  

>+  tabOne = gBrowser.addTab("http://mochi.test:8888/");
>+  tabTwo = gBrowser.addTab("http://mochi.test:8888/browser/browser/base/content/test/tabview/dummy_page.html");

Why two tabs for this test?  Seems like the original tab plus a new tab should be sufficient.

>+  is(groupItems[0].getChildren().length, 2, "The group has three tab items");

The code doesn't agree with the string.  

>   let onLoad = function() {
>-    this.removeEventListener("load", onLoad, true);
>+    event.target.removeEventListener("load", onLoad, true);

Why this change?  I thought "this" was the same as "event.target" in this context; is it not?  I'm doing this elsewhere, so it would be good to know.  

Also, does "event" exist at that point, even though it's not explicitly declared?

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #2)
> Comment on attachment 490055 [details] [diff] [review]
> v1
> 
> I wonder if it would be better to do this by watching SSTabRestoring events. 
> Any thoughts?

The TabSelect is fired before SSTabRestoring event so it doesn't help us to solve the problem. We need to set the "restoredClosedTab" flag before onTabSelect gets called to prevent the code to hide the tabView.

> 
> >+      if (this.restoredClosedTab) {
> >+        tab.linkedBrowser.addEventListener("load", function (event) {
> >+          tab.linkedBrowser.removeEventListener("load", arguments.callee, true);
> >+          // update canvas and fav icon for this tab in tabview UI.
> >+          TabItems._update(tab);
> >+        }, true);
> >+      }
> 
> I assume this is because our normal update mechanism isn't working in this
> case.  Do you know why?  Perhaps we should fix the root cause instead.  

Added a load listener to invoke TabItems_update().

> 
> >+  tabOne = gBrowser.addTab("http://mochi.test:8888/");
> >+  tabTwo = gBrowser.addTab("http://mochi.test:8888/browser/browser/base/content/test/tabview/dummy_page.html");
> 
> Why two tabs for this test?  Seems like the original tab plus a new tab should
> be sufficient.
> 
> >+  is(groupItems[0].getChildren().length, 2, "The group has three tab items");
> 
> The code doesn't agree with the string.  
> 

Updated the test code.  The undoCloseTab() would close the currently selected tab if it's blank so we need to ensure a URL is loaded in the original tab before calling the undoCloseTab().

> >   let onLoad = function() {
> >-    this.removeEventListener("load", onLoad, true);
> >+    event.target.removeEventListener("load", onLoad, true);
> 
> Why this change?  I thought "this" was the same as "event.target" in this
> context; is it not?  I'm doing this elsewhere, so it would be good to know.  
> 
> Also, does "event" exist at that point, even though it's not explicitly
> declared?

Just double check it.  "event.target" isn't exactly.  "event.target" refers to the #document (content document) and "this" refers to the browser element, therefore, using "this" is the right choice.

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 490897 [details] [diff] [review]
v1

(In reply to comment #3)
> > Why two tabs for this test?  Seems like the original tab plus a new tab should
> > be sufficient.
> 
> Updated the test code.  The undoCloseTab() would close the currently selected
> tab if it's blank so we need to ensure a URL is loaded in the original tab
> before calling the undoCloseTab().

I see. Then yes, I think creating two new tabs is probably better than changing the original tab... better to leave things the way we found them. Can you change it back? Sorry for the run-around... I didn't understand the situation.

>+    this._loadEventListener = function(event) {
>+      let index = gBrowser.getBrowserIndexForDocument(event.target);
>+      if (index > -1) {
>+        let tab = gBrowser.tabs[index];
>+        self.update(tab);
>+      }
>+    }

Can't we just do:

  self.update(event.target);
 
... or do we need to go through the getBrowserIndexForDocument step? Ok, upon further reflection, I suppose you're using that as a way to make sure that the target is a xul:tab and attached to our gBrowser... carry on then (unless there's a more efficient way to do those things).
 
Also, is this one of those places where event.target is better than "this"? I feel it reads better, but I'm afraid I don't fully understand the difference. I suppose if "this" is more appropriate, we could have a "let target = this" line to make it explicit what we're doing.

>-      let isCurrentTab = (
>-        !UI._isTabViewVisible() &&
>-        tab == gBrowser.selectedTab
>-      );
>+      let isCurrentTab = (tab == gBrowser.selectedTab);

Why the removal of the visible check? The original intention was to force immediate updating only for the current tab if we weren't in the Panorama UI (so we know the thumbnail will be ready for the zoom out animation). Perhaps the flag is named poorly, but I'm pretty sure we still want that only when the Panorama UI is not visible (and possibly not even then, but that's another bug). Remember that even if the update is deferred, it'll still happen. 

I suppose you just want it to happen as soon as possible, so that the undo feels responsive. This is too broad a change for that. Maybe what you had before (a special load handler for the undone tab) was the right thing for this situation. Once again I apologize for the churn... just making sure I understand the changes.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #4)
> Comment on attachment 490897 [details] [diff] [review]
> v1
> 
> (In reply to comment #3)
> > > Why two tabs for this test?  Seems like the original tab plus a new tab should
> > > be sufficient.
> > 
> > Updated the test code.  The undoCloseTab() would close the currently selected
> > tab if it's blank so we need to ensure a URL is loaded in the original tab
> > before calling the undoCloseTab().
> 
> I see. Then yes, I think creating two new tabs is probably better than changing
> the original tab... better to leave things the way we found them. Can you
> change it back? Sorry for the run-around... I didn't understand the situation.
> 

Changed it back.

> >+    this._loadEventListener = function(event) {
> >+      let index = gBrowser.getBrowserIndexForDocument(event.target);
> >+      if (index > -1) {
> >+        let tab = gBrowser.tabs[index];
> >+        self.update(tab);
> >+      }
> >+    }
> 
> Can't we just do:
> 
>   self.update(event.target);

We can't because load event return a xul:browser rather than a xul:tab. The above code would be removed since we are switching back to the special load handler for the undone tab so we don't need to concern about it

> 
> ... or do we need to go through the getBrowserIndexForDocument step? Ok, upon
> further reflection, I suppose you're using that as a way to make sure that the
> target is a xul:tab and attached to our gBrowser... carry on then (unless
> there's a more efficient way to do those things).
> 
> Also, is this one of those places where event.target is better than "this"? I
> feel it reads better, but I'm afraid I don't fully understand the difference. I
> suppose if "this" is more appropriate, we could have a "let target = this" line
> to make it explicit what we're doing.

I think "this" is same as as event.target.  Personally, I would use event.target if a event param is available, however, it's really down to personal preference.  

> 
> >-      let isCurrentTab = (
> >-        !UI._isTabViewVisible() &&
> >-        tab == gBrowser.selectedTab
> >-      );
> >+      let isCurrentTab = (tab == gBrowser.selectedTab);
> 
> Why the removal of the visible check? The original intention was to force
> immediate updating only for the current tab if we weren't in the Panorama UI
> (so we know the thumbnail will be ready for the zoom out animation). Perhaps
> the flag is named poorly, but I'm pretty sure we still want that only when the
> Panorama UI is not visible (and possibly not even then, but that's another
> bug). Remember that even if the update is deferred, it'll still happen. 
> 
> I suppose you just want it to happen as soon as possible, so that the undo
> feels responsive. This is too broad a change for that. Maybe what you had
> before (a special load handler for the undone tab) was the right thing for this
> situation. Once again I apologize for the churn... just making sure I
> understand the changes.

Switched back to the special load handler for the undone tab.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v1

Minor change.

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Created attachment 491445 [details] [diff] [review]
> v1
> 
> Minor change.

Passed try!

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 491445 [details] [diff] [review]
v1

Thanks, looks good.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 491445 [details] [diff] [review]
v1

a=beltzner, kinda wondering what happens if the last closed tab is in a different window - same as what we do for windows, which kinda sucks?

Comment 10

[Ian Gilman [:iangilman]]

(In reply to comment #9)
> a=beltzner, kinda wondering what happens if the last closed tab is in a
> different window - same as what we do for windows, which kinda sucks?

Beltzner, I'm not sure I know what you mean here. Tab undo happens on a per window basis, so the last closed tab is never in a different window. I'm pretty sure this patch doesn't change that. 

Raymond, can you package for landing?

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ce390d93ab9d

Comment 13

[Tracy Walker [:tracy]]

Verified in recent nightly minefield build

Comment 14

[Tracy Walker [:tracy]]

added addition expectation to existing test case.