Closed
Bug 712203
Opened 12 years ago
Closed 12 years ago
"Next tab group" keyboard shortcut doesn't work after "restore previous session"
Categories
(Firefox Graveyard :: Panorama, defect)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 14
People
(Reporter: k33f3r, Assigned: raymondlee)
References
Details
Attachments
(1 file, 2 obsolete files)
11.72 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
For the record, the current version of Tab Mix Plus doesn't seem to suffer from this problem.
Severity: normal → minor
Updated•12 years ago
|
Component: Keyboard Navigation → Panorama
QA Contact: keyboard.navigation → panorama
Comment 2•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: nobody → marcos
Assignee | ||
Updated•12 years ago
|
Assignee: marcos → raymond
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #605358 -
Flags: review?(ttaubert)
Comment 4•12 years ago
|
||
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-
Assignee | ||
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
(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
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
(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)
Assignee | ||
Updated•12 years ago
|
Attachment #605358 -
Attachment is obsolete: true
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 years ago
|
||
(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
Assignee | ||
Comment 12•12 years ago
|
||
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
Comment 14•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #607454 -
Flags: review?(ttaubert)
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #607454 -
Attachment is obsolete: true
Attachment #610504 -
Flags: review?(ttaubert)
Comment 17•12 years ago
|
||
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+
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e91b6175dceb
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e91b6175dceb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•