Closed Bug 712203 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: k33f3r, Assigned: raymondlee)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
For the record, the current version of Tab Mix Plus doesn't seem to suffer from this problem.
Severity: normal → minor
Component: Keyboard Navigation → Panorama
QA Contact: keyboard.navigation → panorama
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: 8 Branch → Trunk
Assignee: nobody → marcos
Assignee: marcos → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #605358 - Flags: review?(ttaubert)
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().
Attachment #605358 - Flags: review?(ttaubert) → review-
(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?
(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.
Depends on: 736414
(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.
Attached patch Patch for checkin (obsolete) — Splinter Review
(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
Attachment #607454 - Flags: review?(ttaubert)
Attachment #605358 - Attachment is obsolete: true
(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 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()) {
Attachment #607454 - Flags: review?(ttaubert) → feedback+
(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
I have pushed to try again and the oranges are not related to this patch.
https://tbpl.mozilla.org/?tree=Try&rev=9e7a03affa8b
Keywords: checkin-needed
Tim, did you mean to not r+ this?
Keywords: checkin-needed
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
Attachment #607454 - Flags: review?(ttaubert)
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.
Attachment #607454 - Flags: review?(ttaubert)
Attached patch v3Splinter Review
Attachment #607454 - Attachment is obsolete: true
Attachment #610504 - Flags: review?(ttaubert)
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.
Attachment #610504 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/integration/fx-team/rev/e91b6175dceb
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/e91b6175dceb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.