Closed Bug 595930 Opened 10 years ago Closed 10 years ago

closing a group breaks the keyboard shortcut for toggling panorama

Categories

(Firefox Graveyard :: Panorama, defect, P2)

x86
Linux
defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dietrich, Assigned: seanedunn)

References

Details

(Keywords: regression, relnote)

Attachments

(1 file, 4 obsolete files)

STR:

1. create a group with a tab in it
2. close the group
3. hit the keyboard shortcut

expected: toggles out of panorama into a tab view

actual: nothing
blocking, since we just got beta users used to this (and they skew power user).
blocking2.0: --- → beta6+
Keywords: regression
Blocks: 594094
Not sure that this blocks beta6, though - I can see this being renlote fodder.
blocking2.0: beta6+ → betaN+
Keywords: relnote
Assignee: nobody → seanedunn
Because you've just removed the only tab that existed, should it just create a new tab when it toggles back to browser view?
(In reply to comment #3)
> Because you've just removed the only tab that existed, should it just create a
> new tab when it toggles back to browser view?

It doesn't have to be the only tab; you can repro this with plenty of other tabs and groups open. 

Perhaps it has to do with the new "undo close group" feature?

Also, to repro, it appears you need to have just been to one of the tabs in the group you close. If you've just been to a tab from another group, exiting works just fine. 

Seems like what's going on is it's looking for the last accessed tab, in order to return to it, and when it finds it gone, it just sits there dumbly instead of finding the next one. This may (or may not) be complicated by "undo close group", because in that case, the last accessed tab is still there, but hidden, and will be closed as soon as you exit Panorama.
Blocks: 597043
BTW, for a while (seconds?) the Panorama button doesn't seem to respond, making me feel like there's no way out, although it eventually does.
Priority: -- → P2
Attached patch v1 (obsolete) — Splinter Review
The active tab wasn't being set when the group is closed. I now set the active tab to the closest tab upon closing the group.
Attachment #478902 - Flags: feedback?(ian)
Comment on attachment 478902 [details] [diff] [review]
v1

By the way, I believe bug 595374 will fix the symptoms of this bug as well, but this patch is a step towards fixing bug 599414. 

I think it would be faster to loop through the items once and keep a running tally of the shortest distance, rather than sorting them. 

Otherwise looking good.
Attachment #478902 - Flags: feedback?(ian) → feedback+
Attached patch v2 (obsolete) — Splinter Review
Attachment #478902 - Attachment is obsolete: true
Attachment #478927 - Flags: feedback?(ian)
Status: NEW → ASSIGNED
Comment on attachment 478927 [details] [diff] [review]
v2

Looks lovely
Attachment #478927 - Flags: feedback?(ian) → feedback+
Attached patch v3 (w/ test) (obsolete) — Splinter Review
Attachment #481142 - Flags: review?(dolske)
Attached patch v4 (w/ test) (obsolete) — Splinter Review
Added test.
Attachment #478927 - Attachment is obsolete: true
Attachment #481142 - Attachment is obsolete: true
Attachment #481143 - Flags: review?(dolske)
Attachment #481142 - Flags: review?(dolske)
Attachment #481143 - Flags: review?(dolske) → review?(dietrich)
Comment on attachment 481143 [details] [diff] [review]
v4 (w/ test)


>   // ----------
>+  // Function: getClosestTab
>+  // Convenience function to get the next tab closest to the entered position
>+  getClosestTab: function UI_getClosestTab(tabCenter) {
>+    let cl = null;
>+    let clDist;
>+    for each(item in TabItems.getItems()) {
>+      if (item.parent && item.parent.hidden) {
>+        continue;
>+      }

does hidden not trickle down to tabItems? would be nice than having to check the parent every time. don't need to fix that here though, or maybe there's a reason it's not like that.

>+      let testDist = tabCenter.distance(item.bounds.center());
>+      if (cl==null || testDist < clDist) {

nit: spaces before/after operators.

r=me
Attachment #481143 - Flags: review?(dietrich) → review+
pushed to try (011db6aad9c5)
Comment on attachment 481143 [details] [diff] [review]
v4 (w/ test)

Try successful (all errors were from previous/unrelated tests)
Attachment #481143 - Flags: approval2.0?
Attachment #481143 - Flags: approval2.0? → approval2.0+
Sean, looks good... passed try, got approval (and it was a blocker, so you don't need approval anyway). Please package it for landing as per items 8 and 9 in: 

https://wiki.mozilla.org/Firefox/Projects/TabCandy/Work#Landing
Attachment #481143 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9ae3465d61fa
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly minefield builds of 20101021
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.