590606 - Switching among groups always selects the first tab of group

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Ed Lee :Mardak]

Presing ctrl-` won't go back to the previously selected tab of that group. And if you happen to have an app tab, switching to another group and going to the "previous tab" to the left will hit the bug where selecting the app tab switches to the group.

Comment 1

[Dietrich Ayala (:dietrich)]

app tabs problem is bug 578553. let's keep this one solely on restoring group state.

Comment 2

[Dietrich Ayala (:dietrich)]

this doesn't actually depend on the session restore support. just need to keep flag somewhere saying what the last-active-tab for each group is, and make it active when the group is opened.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: v1

this works in the current session, but doesn't persist the data across restarts yet.

Comment 4

[Ed Lee :Mardak]

Is the active tab an attribute that only makes sense in the context of Panorama or should it be on xul tabs? I suppose it probably won't be useful for css to match [hidden]:not([active]) tabs differently as they're.. hidden ;)

Comment 5

[Dão Gottwald [:dao]]

I don't think it's going to be useful on xul tabs...

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: v1

oops, forgot to qrefresh.

Comment 8

[Dietrich Ayala (:dietrich)]

weird. the feedback flag looked set on the main bug page, but wasn't in the attachment page.

Comment 9

[Ian Gilman [:iangilman]]

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

 looks good to me. I don't know that it needs to save anyway.

Comment 10

[Dietrich Ayala (:dietrich)]

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

Ok, thanks. Asking for review then, so can push it directly m-c (and t-c if you want).

Will file a separate bug for persisting position across sessions.

Comment 11

[Dietrich Ayala (:dietrich)]

filed bug 593283.

Comment 12

[Dietrich Ayala (:dietrich)]

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

canceling request, patch needs test.

Comment 13

[Raymond Lee [:raymondlee]]

Going to write a test for this patch.

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v1

With test

Comment 15

[Raymond Lee [:raymondlee]]

(In reply to comment #14)
> Created attachment 477428 [details] [diff] [review]
> v1
> 
> With test

Passed try.

Comment 16

[Ian Gilman [:iangilman]]

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

+  is(gBrowser.selectedTab, groupItemOne.getChild(1).tab,
+    "The currently selected tab should be the first tab in the groupItemTwo");

The string should read "second tab" rather than "first tab".

F+ with that fixed. Assigning review to Dolske (Dietrich wrote the original patch).

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v1

Fixed the comment suggested by Ian

f+=ian

Comment 18

[Raymond Lee [:raymondlee]]

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/151d90b45e46

Comment 19

[Ian Gilman [:iangilman]]

Landed on mozilla-central: 

http://hg.mozilla.org/mozilla-central/rev/151d90b45e46

Comment 20

[Tracy Walker [:tracy]]

verified with recent nightly build of minefield