restore focus after "Undo Close Group" of last group

VERIFIED FIXED

Status

P4
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: AlexLakatos, Assigned: ttaubert)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Version: unspecified → Trunk
(Assignee)

Updated

8 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

8 years ago
Created attachment 514041 [details] [diff] [review]
patch v1
Attachment #514041 - Flags: review?(ian)
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.
Attachment #514041 - Flags: review?(ian) → review-
Blocks: 627096
Priority: -- → P4
(Assignee)

Comment 3

8 years ago
Created attachment 514067 [details] [diff] [review]
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.
Attachment #514041 - Attachment is obsolete: true
Attachment #514067 - Flags: review?(ian)
Comment on attachment 514067 [details] [diff] [review]
patch v2

Lovely!
Attachment #514067 - Flags: review?(ian) → review+
(Assignee)

Comment 5

8 years ago
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.
Attachment #514067 - Flags: approval2.0?
Comment on attachment 514067 [details] [diff] [review]
patch v2

a=beltzner
Attachment #514067 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 8

8 years ago
Created attachment 514255 [details] [diff] [review]
patch for checkin
Attachment #514067 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

8 years ago
http://hg.mozilla.org/mozilla-central/rev/41297d3b8511
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 10

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

Verified issue and it's no longer present.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.