595930 - closing a group breaks the keyboard shortcut for toggling panorama

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Dietrich Ayala (:dietrich)]

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

Comment 1

[Dietrich Ayala (:dietrich)]

blocking, since we just got beta users used to this (and they skew power user).

Comment 2

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

Not sure that this blocks beta6, though - I can see this being renlote fodder.

Comment 3

[Sean Dunn]

Because you've just removed the only tab that existed, should it just create a new tab when it toggles back to browser view?

Comment 4

[Ian Gilman [:iangilman]]

(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.

Comment 5

[juan becerra [:juanb]]

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.

Comment 6

[Sean Dunn]

Attachment: v1

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.

Comment 7

[Ian Gilman [:iangilman]]

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.

Comment 8

[Sean Dunn]

Attachment: v2

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 478927 [details] [diff] [review]
v2

Looks lovely

Comment 10

[Sean Dunn]

Attachment: v3 (w/ test)

Comment 11

[Sean Dunn]

Attachment: v4 (w/ test)

Added test.

Comment 12

[Dietrich Ayala (: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

Comment 13

[Sean Dunn]

pushed to try (011db6aad9c5)

Comment 14

[Sean Dunn]

Comment on attachment 481143 [details] [diff] [review]
v4 (w/ test)

Try successful (all errors were from previous/unrelated tests)

Comment 15

[Ian Gilman [:iangilman]]

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

Comment 16

[Sean Dunn]

Attachment: refreshed for check-in

Comment 17

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/9ae3465d61fa

Comment 18

[Tracy Walker [:tracy]]

verified with nightly minefield builds of 20101021