635696 - restore focus after "Undo Close Group" of last group

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Alex Lakatos[:AlexLakatos]]

Panorama does not restore focus after "Undo Close Group" of last group

Steps To Reproduce:
1.Close the last goup in Panorama
2.Undo Close Group
3.Press Enter

Expected Results:
3.The last focused tab is opened in Tab View

Actual Results:
3.The last focused tab is not opened in Tab View, the browser remains in Panorama View

As for a regression window, this happens as far as Fx4b7, the first time "Undo Close Group" was introduced.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Ian Gilman [:iangilman]]

Comment on attachment 514041 [details] [diff] [review]
patch v1

>   _makeClosestTabActive: function GroupItem__makeClosestTabActive() {
>     let closeCenter = this.getBounds().center();
>     // Find closest tab to make active
>     let closestTabItem = UI.getClosestTab(closeCenter);
>-    UI.setActiveTab(closestTabItem);
>-
>     // set the active group or orphan tabitem.
>     if (closestTabItem) {
>+      UI.setActiveTab(closestTabItem);

This change is a problem; even if there's no closest tab, we need to null the active tab when its group gets hidden, or we'll run into trouble when we exit tab view. 

>+    if (!GroupItems.getActiveGroupItem() && !GroupItems.getActiveOrphanTab())
>+      GroupItems.setActiveGroupItem(this);

I would say this shouldn't be an if... when you undo a close group, it should always become the active group. Also its active tab should become the UI activeTab.

>+function hideGroup(groupItem, callback) {
>+  groupItem.addSubscriber(groupItem, "groupHidden", function () {
>+    groupItem.removeSubscriber(groupItem, "groupHidden");
>+    callback();
>+  });
>+  groupItem.closeAll();
>+}
>+
>+function unhideGroup(groupItem, callback) {
>+  groupItem.addSubscriber(groupItem, "groupShown", function () {
>+    groupItem.removeSubscriber(groupItem, "groupShown");
>+    callback();
>+  });
>+  groupItem._unhide();
>+}

Candidates for head.js? They should be called hideGroupItem and unhideGroupItem.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #2)
> This change is a problem; even if there's no closest tab, we need to null the
> active tab when its group gets hidden, or we'll run into trouble when we exit
> tab view.

Removed as this isn't needed anymore when removing the "if" at the bottom.

> I would say this shouldn't be an if... when you undo a close group, it should
> always become the active group. Also its active tab should become the UI
> activeTab.

Fixed.

> Candidates for head.js? They should be called hideGroupItem and
> unhideGroupItem.

Done.

Comment 4

[Ian Gilman [:iangilman]]

Comment on attachment 514067 [details] [diff] [review]
patch v2

Lovely!

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 514067 [details] [diff] [review]
patch v2

Note to approvers:

Very small and low-risk patch with a test that ensures consistent UI behavior.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 514067 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=aba17c0eb518

Comment 7

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

Comment on attachment 514067 [details] [diff] [review]
patch v2

a=beltzner

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/41297d3b8511

Comment 10

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110224 Firefox/4.0b13pre

Verified issue and it's no longer present.