712203 - "Next tab group" keyboard shortcut doesn't work after "restore previous session"

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Keith P. Johnson]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:8.0.1) Gecko/20111117 Firefox/8.0.1
Build ID: 20111117202623

Steps to reproduce:

1.  Start a fresh Firefox session.
2.  Create a tab, e.g. www.google.com.
3.  Create another tab, e.g. www.yahoo.com.
4.  Click tab group button.
5.  Move one of the tabs into a new tab group.
6.  Close Firefox.
7.  Open Firefox.
8.  Choose Firefox->History->Restore Previous Session.
9.  Try to navigate tab groups with control(-shift)-`


Actual results:

Keyboard shortcut is inoperative for tab groups.


Expected results:

Tab group should've swapped.  The keys become operative after you click on the tab groups button and select any page.

Comment 1

[Keith P. Johnson]

For the record, the current version of Tab Mix Plus doesn't seem to suffer from this problem.

Comment 2

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0

Confirming this with the steps from comment 0 (set "Show a blank page" when Firefox starts to trigger Session restore).

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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

Review of attachment 605358 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-tabview.js
@@ -102,5 @@
>  
>      if (this.firstUseExperienced) {
> -      if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> -        this._setBrowserKeyHandlers();
> -

This shouldn't be removed as we need to register key handlers also when visibility=false.

@@ +139,5 @@
>          gBrowser.tabContainer.addEventListener(
>            "TabClose", this._tabCloseEventListener, false);
> +
> +       if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) {
> +         this._setBrowserKeyHandlers();

(see above)

@@ +143,5 @@
> +         this._setBrowserKeyHandlers();
> +       } else {
> +         // for restoring last session and undoing recently closed window
> +         this._SSWindowStateReady = function(event) {
> +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)

We should move this condition to _setBrowserKeyHandlers().

@@ +147,5 @@
> +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> +             self._setBrowserKeyHandlers();
> +         };
> +         window.addEventListener(
> +           "SSWindowStateReady", this._SSWindowStateReady, false);

SSWindowStateReady is dispatched even when just restoring a single tab. We should observe "sessionstore-windows-restored" and "sessionstore-browser-state-restored".

@@ +183,5 @@
>        gBrowser.tabContainer.removeEventListener(
>          "TabClose", this._tabCloseEventListener, false);
>  
> +    if (this._SSWindowStateReady)
> +      window.addEventListener(

removeEventListener().

@@ +243,5 @@
>            "TabClose", self._tabCloseEventListener, false);
>          self._tabCloseEventListener = null;
>        }
> +      if (self._SSWindowStateReady) {
> +        window.addEventListener(

removeEventListener().

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Comment on attachment 605358 [details] [diff] [review]
> v1
> 
> Review of attachment 605358 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +147,5 @@
> > +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> > +             self._setBrowserKeyHandlers();
> > +         };
> > +         window.addEventListener(
> > +           "SSWindowStateReady", this._SSWindowStateReady, false);
> 
> SSWindowStateReady is dispatched even when just restoring a single tab. We
> should observe "sessionstore-windows-restored" and
> "sessionstore-browser-state-restored".
> 
 
"sessionstore-windows-restored" and "sessionstore-browser-state-restored" would fix the STR in comment 0, however, it does fix the following STR:

1) Open Firefox
2) Open another window and create one more tab (Two visible tabs).
3) Go into Panorama and drag a tab out to create another group (Two groups and each group has one tab).
4) Exit Panorama and close that window
5) Go to History > Recently Closed Window > Restore that window
6) Try to navigate tab groups with control(-shift)-`

Nothing happens when using "sessionstore-windows-restored" and "sessionstore-browser-state-restored" but using "SSWindowStateReady" solves both STRs.  @Tim: what do you think?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Raymond Lee [:raymondlee] from comment #5)
> 1) Open Firefox
> 2) Open another window and create one more tab (Two visible tabs).
> 3) Go into Panorama and drag a tab out to create another group (Two groups
> and each group has one tab).
> 4) Exit Panorama and close that window
> 5) Go to History > Recently Closed Window > Restore that window
> 6) Try to navigate tab groups with control(-shift)-`
> 
> Nothing happens when using "sessionstore-windows-restored" and
> "sessionstore-browser-state-restored" but using "SSWindowStateReady" solves
> both STRs.  @Tim: what do you think?

Yeah... there *should* be a notification. Filed bug 736414, waiting for review.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Raymond Lee [:raymondlee] from comment #5)
> Nothing happens when using "sessionstore-windows-restored" and
> "sessionstore-browser-state-restored" but using "SSWindowStateReady" solves
> both STRs.  @Tim: what do you think?

I just closed bug 736414 as WONTFIX because zpao said that this notification isn't fired intentionally. Looks like we should rather use SSWindowStateReady.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #4)
> ::: browser/base/content/browser-tabview.js
> @@ -102,5 @@
> >  
> >      if (this.firstUseExperienced) {
> > -      if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> > -        this._setBrowserKeyHandlers();
> > -
> 
> This shouldn't be removed as we need to register key handlers also when
> visibility=false.
> 
> @@ +139,5 @@
> >          gBrowser.tabContainer.addEventListener(
> >            "TabClose", this._tabCloseEventListener, false);
> > +
> > +       if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) {
> > +         this._setBrowserKeyHandlers();
> 
> (see above)

I don't see any issue to move the above.  When visibility=true, the lines would be executed this.show() -> this._initFrame() -> this._setBrowserKeyHandlers().  When visibility=false, the above would get executed. if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) this._setBrowserKeyHandlers()

> 
> @@ +143,5 @@
> > +         this._setBrowserKeyHandlers();
> > +       } else {
> > +         // for restoring last session and undoing recently closed window
> > +         this._SSWindowStateReady = function(event) {
> > +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> 
> We should move this condition to _setBrowserKeyHandlers().

We don't want to move the condition to this _setBrowserKeyHandlers() because it would be invoked when the tabview frame is initialized for the first time in the browser session regardless how many hidden tabs.

> 
> @@ +147,5 @@
> > +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> > +             self._setBrowserKeyHandlers();
> > +         };
> > +         window.addEventListener(
> > +           "SSWindowStateReady", this._SSWindowStateReady, false);
> 
> SSWindowStateReady is dispatched even when just restoring a single tab. We
> should observe "sessionstore-windows-restored" and
> "sessionstore-browser-state-restored".

We are keeping the SSWindowStateReady.

> 
> @@ +183,5 @@
> >        gBrowser.tabContainer.removeEventListener(
> >          "TabClose", this._tabCloseEventListener, false);
> >  
> > +    if (this._SSWindowStateReady)
> > +      window.addEventListener(
> 
> removeEventListener().

Fixed

> 
> @@ +243,5 @@
> >            "TabClose", self._tabCloseEventListener, false);
> >          self._tabCloseEventListener = null;
> >        }
> > +      if (self._SSWindowStateReady) {
> > +        window.addEventListener(
> 
> removeEventListener().

Fixed

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Raymond Lee [:raymondlee] from comment #8)
> I don't see any issue to move the above.  When visibility=true, the lines
> would be executed this.show() -> this._initFrame() ->
> this._setBrowserKeyHandlers().  When visibility=false, the above would get
> executed. if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> this._setBrowserKeyHandlers()

Yes, you're correct. I didn't see the _setBrowserKeyHandlers() call in "tabviewframeinitialized".

> We don't want to move the condition to this _setBrowserKeyHandlers() because
> it would be invoked when the tabview frame is initialized for the first time
> in the browser session regardless how many hidden tabs.

Same here.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 607454 [details] [diff] [review]
Patch for checkin

Review of attachment 607454 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-tabview.js
@@ +138,5 @@
>            "TabShow", this._tabShowEventListener, false);
>          gBrowser.tabContainer.addEventListener(
>            "TabClose", this._tabCloseEventListener, false);
> +
> +       if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) {

We should add a method for this to not repeat ourselves and to make it more readable:

if (this._tabBrowserHasHiddenTabs()) {
  // ...
}

@@ +142,5 @@
> +       if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) {
> +         this._setBrowserKeyHandlers();
> +       } else {
> +         // for restoring last session and undoing recently closed window
> +         this._SSWindowStateReady = function(event) {

Nit: please call this _SSWindowStateReadyListener.

@@ +143,5 @@
> +         this._setBrowserKeyHandlers();
> +       } else {
> +         // for restoring last session and undoing recently closed window
> +         this._SSWindowStateReady = function(event) {
> +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)

if (self._tabBrowserHasHiddenTabs()) {

Comment 11

[Raymond Lee [:raymondlee]]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 607454 [details] [diff] [review]
> Patch for checkin
> 
> Review of attachment 607454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-tabview.js
> @@ +138,5 @@
> >            "TabShow", this._tabShowEventListener, false);
> >          gBrowser.tabContainer.addEventListener(
> >            "TabClose", this._tabCloseEventListener, false);
> > +
> > +       if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) {
> 
> We should add a method for this to not repeat ourselves and to make it more
> readable:
> 
> if (this._tabBrowserHasHiddenTabs()) {
>   // ...
> }
> 
> @@ +142,5 @@
> > +       if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0) {
> > +         this._setBrowserKeyHandlers();
> > +       } else {
> > +         // for restoring last session and undoing recently closed window
> > +         this._SSWindowStateReady = function(event) {
> 
> Nit: please call this _SSWindowStateReadyListener.
> 
> @@ +143,5 @@
> > +         this._setBrowserKeyHandlers();
> > +       } else {
> > +         // for restoring last session and undoing recently closed window
> > +         this._SSWindowStateReady = function(event) {
> > +           if ((gBrowser.tabs.length - gBrowser.visibleTabs.length) > 0)
> 
> if (self._tabBrowserHasHiddenTabs()) {

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=d46b364933b3

Comment 12

[Raymond Lee [:raymondlee]]

I have pushed to try again and the oranges are not related to this patch.
https://tbpl.mozilla.org/?tree=Try&rev=9e7a03affa8b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Tim, did you mean to not r+ this?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Also, to make life easier for those checking in patches for you, please follow
the directions below for any future patches you submit. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 607454 [details] [diff] [review]
Patch for checkin

Review of attachment 607454 [details] [diff] [review]:
-----------------------------------------------------------------

Please attach the new patch with the issues from comment #10 addressed.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 610504 [details] [diff] [review]
v3

Review of attachment 610504 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good to me! I'll push it in a sec.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/e91b6175dceb

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e91b6175dceb