Closed
Bug 585830
Opened 14 years ago
Closed 14 years ago
A closing tab can be switched to with Ctrl+PageUp/PageDown
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 4.0b5
| Tracking | Status | |
|---|---|---|
| blocking2.0 | --- | - |
People
(Reporter: dholbert, Assigned: sindrebugzilla)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
|
(deleted),
patch
|
dao
:
review+
johnath
:
approval2.0+
|
Details | Diff | Splinter Review |
STR:
1. Open three tabs at different sites, which we'll label:
[ A ] [ B ] [ C ]
for example:
[ http://mozilla.org ] [ http://amazon.com ] [ http://wikipedia.org ]
2. Focus the "Tab B" (amazon.com).
3. Hold Ctrl, and press W and then PageDown within a second of each other.
EXPECTED RESULTS:
I should end up having "Tab A" focused. (mozilla.org)
(Ctrl+W should close Tab B, moving me to Tab C, and then Ctrl+PageDown should shift me left one tab to Tab A.)
ACTUAL RESULTS:
I end up seeing "Tab C" (wikipedia.org).
It looks like Tab B is still selectable while its closing-animation is playing. This means the Ctrl+PageDown action will select the tab that's being closed, **as it's closing**, and then an instant later it _actually_ closes and forces me rightwards one tab.
(FWIW, the "Ctrl + W,PageDown" action is one that I perform quite frequently, to close a tab and see the one to its left, so I hit this bug all the time.)
Filing as a regression from bug 380960, since that's when this started appearing. (that's when tab-closure started taking a nonzero amount of time)
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100809 Minefield/4.0b4pre
|
Reporter | |
Comment 1•14 years ago
|
||
FWIW, I confirmed that this bug affects the first nightly build to have tab-closing-animations -- that build being:
20100807030716 81c119fb86c7
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/08/2010-08-07-03-mozilla-central/firefox-4.0b4pre.en-US.linux-x86_64.txt
|
|
Reporter | |
Updated•14 years ago
|
blocking2.0: --- → ?
|
Assignee | |
Comment 2•14 years ago
|
||
Attachment #468449 -
Flags: review?(dao)
|
||
Comment 3•14 years ago
|
||
Comment on attachment 468449 [details] [diff] [review]
Patch + Test
toolkit widgets can't use gBrowser. What you could do is implement a "_canAdvanceToTab" stub method that always returns true and let the tabs binding in tabbrowser.xml override it.
Attachment #468449 -
Flags: review?(dao) → review-
|
|
Assignee | |
Comment 4•14 years ago
|
||
Attachment #468449 -
Attachment is obsolete: true
Attachment #468491 -
Flags: review?(dao)
|
|
||
Comment 5•14 years ago
|
||
Comment on attachment 468491 [details] [diff] [review]
Patch v2 + Test v1
> <method name="advanceSelectedTab">
> <parameter name="aDir"/>
> <parameter name="aWrap"/>
> <body>
> <![CDATA[
> var startTab = this.selectedItem;
>- var next = startTab[aDir == -1 ? "previousSibling" : "nextSibling"];
>- if (!next && aWrap) {
>- next = aDir == -1 ? this.childNodes[this.childNodes.length - 1] :
>- this.childNodes[0];
>- }
>+ var next = startTab;
>+ do {
>+ next = next[aDir == -1 ? "previousSibling" : "nextSibling"];
>+ if (!next && aWrap) {
>+ next = aDir == -1 ? this.childNodes[this.childNodes.length - 1] :
>+ this.childNodes[0];
>+ }
>+ } while (next && next != startTab && !this._canAdvanceToTab(next));
> if (next && next != startTab) {
> this._selectNewTab(next, aDir, aWrap);
> }
This looks alright, but there's already a similar loop in _selectNewTab. Looks like you could just add the call to _canAdvanceToTab there.
Attachment #468491 -
Flags: review?(dao) → review-
|
|
||
Comment 6•14 years ago
|
||
(In reply to comment #5)
> This looks alright, but there's already a similar loop in _selectNewTab. Looks
> like you could just add the call to _canAdvanceToTab there.
This will also fix Ctrl+End for instance.
|
|
||
Comment 7•14 years ago
|
||
Comment on attachment 468491 [details] [diff] [review]
Patch v2 + Test v1
Just looked at the test for the first time. Can you add tabs with {skipAnimation: true} instead of waiting for the opening animation to complete?
>+ let tab1 = gBrowser.selectedTab;
>+ let tab2 = gBrowser.addTab();
>+ let tab3 = gBrowser.addTab();
>+ gBrowser.selectedTab = tab2;
>+ waitForExplicitFinish();
>+
>+ // Wait for fade in animation to complete
>+ window.setTimeout( function() {
|
|
Assignee | |
Comment 8•14 years ago
|
||
Attachment #468491 -
Attachment is obsolete: true
Attachment #468503 -
Flags: review?(dao)
|
|
Assignee | |
Comment 9•14 years ago
|
||
Comment on attachment 468503 [details] [diff] [review]
Patch v3 + Test v2
Oops, uploaded wrong patch.
Attachment #468503 -
Attachment is obsolete: true
Attachment #468503 -
Flags: review?(dao)
|
|
Assignee | |
Comment 10•14 years ago
|
||
Attachment #468504 -
Flags: review?(dao)
|
|
Assignee | |
Comment 11•14 years ago
|
||
This isn't really relevant to this bug, but a "null has no properties" exception is thrown in the loop in _selectNewTab when (!aNewTab && !aWrap). Is this something that should be fixed?
|
|
||
Comment 12•14 years ago
|
||
(In reply to comment #11)
> This isn't really relevant to this bug, but a "null has no properties"
> exception is thrown in the loop in _selectNewTab when (!aNewTab && !aWrap). Is
> this something that should be fixed?
It doesn't look like aNewTab should ever be null, so letting the code complain about it seems correct.
|
|
Assignee | |
Comment 13•14 years ago
|
||
(In reply to comment #12)
> (In reply to comment #11)
> > This isn't really relevant to this bug, but a "null has no properties"
> > exception is thrown in the loop in _selectNewTab when (!aNewTab && !aWrap). Is
> > this something that should be fixed?
>
> It doesn't look like aNewTab should ever be null, so letting the code complain
> about it seems correct.
aNewTab = aFallbackDir == -1 ? aNewTab.previousSibling : aNewTab.nextSibling;
aNewTab will become null here if there isn't a nextSibling/previousSibling (aka. we're all ready at the last tab). Without wrapping it will then remain null and the loop will continue.
Unless you meant that the function should never be called in such a way that this happened?
|
|
||
Updated•14 years ago
|
Attachment #468504 -
Flags: review?(dao)
Attachment #468504 -
Flags: review+
Attachment #468504 -
Flags: approval2.0?
|
|
||
Comment 14•14 years ago
|
||
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > This isn't really relevant to this bug, but a "null has no properties"
> > > exception is thrown in the loop in _selectNewTab when (!aNewTab && !aWrap). Is
> > > this something that should be fixed?
> >
> > It doesn't look like aNewTab should ever be null, so letting the code complain
> > about it seems correct.
>
> aNewTab = aFallbackDir == -1 ? aNewTab.previousSibling : aNewTab.nextSibling;
>
> aNewTab will become null here if there isn't a nextSibling/previousSibling
> (aka. we're all ready at the last tab). Without wrapping it will then remain
> null and the loop will continue.
Yeah, this looks wrong.
|
|
Assignee | |
Comment 15•14 years ago
|
||
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > This isn't really relevant to this bug, but a "null has no properties"
> > > > exception is thrown in the loop in _selectNewTab when (!aNewTab && !aWrap). Is
> > > > this something that should be fixed?
> > >
> > > It doesn't look like aNewTab should ever be null, so letting the code complain
> > > about it seems correct.
> >
> > aNewTab = aFallbackDir == -1 ? aNewTab.previousSibling : aNewTab.nextSibling;
> >
> > aNewTab will become null here if there isn't a nextSibling/previousSibling
> > (aka. we're all ready at the last tab). Without wrapping it will then remain
> > null and the loop will continue.
>
> Yeah, this looks wrong.
Can it be fixed in this bug, or is it necessary to start a new one?
|
|
||
Comment 16•14 years ago
|
||
Whatever you prefer.
|
|
Assignee | |
Comment 17•14 years ago
|
||
Attachment #468504 -
Attachment is obsolete: true
Attachment #468655 -
Flags: review?(dao)
Attachment #468504 -
Flags: approval2.0?
|
|
||
Comment 18•14 years ago
|
||
Comment on attachment 468655 [details] [diff] [review]
Patch v4 + Test v3
thanks!
Attachment #468655 -
Flags: review?(dao) → review+
|
||
Comment 20•14 years ago
|
||
Comment on attachment 468655 [details] [diff] [review]
Patch v4 + Test v3
But worth fixing - a=me
Attachment #468655 -
Flags: approval2.0+
|
|
||
Updated•14 years ago
|
Assignee: nobody → sindrebugzilla
Keywords: checkin-needed
|
|
||
Comment 21•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
|
|
||
Updated•14 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•