Last Comment Bug 712203 - "Next tab group" keyboard shortcut doesn't work after "restore previous session"
: "Next tab group" keyboard shortcut doesn't work after "restore previous session"
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- minor
: Firefox 14
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 736414
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-19 18:50 PST by Keith P. Johnson
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (9.59 KB, patch)
2012-03-13 05:25 PDT, Raymond Lee [:raymondlee]
ttaubert: review-
Details | Diff | Review
Patch for checkin (10.04 KB, patch)
2012-03-19 22:10 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Review
v3 (11.72 KB, patch)
2012-03-29 03:54 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Review

Description Keith P. Johnson 2011-12-19 18:50:34 PST
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 2011-12-19 18:53:14 PST
For the record, the current version of Tab Mix Plus doesn't seem to suffer from this problem.
Comment 2 Virgil Dicu [:virgil] [QA] 2011-12-28 02:55:57 PST
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] 2012-03-13 05:25:54 PDT
Created attachment 605358 [details] [diff] [review]
v1
Comment 4 Tim Taubert [:ttaubert] 2012-03-13 06:06:51 PDT
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] 2012-03-14 11:11:45 PDT
(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] 2012-03-16 02:39:05 PDT
(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] 2012-03-19 06:33:11 PDT
(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] 2012-03-19 22:10:51 PDT
Created attachment 607454 [details] [diff] [review]
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] 2012-03-26 04:46:10 PDT
(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] 2012-03-26 04:51:55 PDT
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] 2012-03-26 08:56:24 PDT
(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] 2012-03-28 12:31:54 PDT
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] 2012-03-28 17:59:31 PDT
Tim, did you mean to not r+ this?
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-03-28 18:03:16 PDT
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] 2012-03-29 03:15:00 PDT
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] 2012-03-29 03:54:15 PDT
Created attachment 610504 [details] [diff] [review]
v3
Comment 17 Tim Taubert [:ttaubert] 2012-03-29 04:51:47 PDT
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] 2012-03-29 05:01:55 PDT
https://hg.mozilla.org/integration/fx-team/rev/e91b6175dceb
Comment 19 Tim Taubert [:ttaubert] 2012-03-30 09:34:12 PDT
https://hg.mozilla.org/mozilla-central/rev/e91b6175dceb

Note You need to log in before you can comment on or make changes to this bug.