Closed
Bug 358362
Opened 18 years ago
Closed 16 years ago
tab owner not always reset after tabs moved
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: tabmix.onemen, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061026 Minefield/3.0a1
its look to me that there is a bug in addTab methot
var self = this;
function attrChanged(event) {
if (event.attrName == "selectedIndex" &&
event.prevValue != event.newValue)
self.resetOwner(parseInt(event.prevValue));
}
if (!this.mTabChangedListenerAdded) {
this.mTabBox.addEventListener("DOMAttrModified", attrChanged, false);
this.mTabChangedListenerAdded = true;
}
as i understand it event.prevValue and event.newValue aren't prev and new tab index
thay are prev and new panel index in mPanelContainer.
after tabs are moved this indexes aren't the same so we end up reset owner for the wrong tab
this patch fix the bug
<method name="resetOwner">
<parameter name="oldIndex"/>
<body>
<![CDATA[
// Reset the owner property, since we're leaving the modally opened
// tab for another.
if (oldIndex < this.mTabContainer.childNodes.length) {
+ var oldBrowser = this.mPanelContainer.childNodes[oldIndex].firstChild;
+ oldIndex = this.getBrowserIndexForDocument(oldBrowser.contentDocument);
+ if (oldIndex < 0)
+ return;
var tab = this.mTabContainer.childNodes[oldIndex];
tab.owner = null;
}
]]>
</body>
</method>
Reproducible: Always
|
||
Comment 1•18 years ago
|
||
Would you like to make a patch from cvs and ask for review?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
|
Reporter | |
Comment 2•18 years ago
|
||
Attachment #243797 -
Flags: review+
|
|
Reporter | |
Updated•18 years ago
|
Attachment #243797 -
Flags: review+
|
||
Updated•18 years ago
|
Attachment #243797 -
Flags: review?(gavin.sharp)
|
|
||
Comment 3•17 years ago
|
||
onemen.one: does this problem still exist? I haven't gotten to reviewing the patch, which is entirely my fault, but if you could update this bug with the current status that'd be helpful.
|
|
Reporter | |
Comment 4•17 years ago
|
||
i'll check that this weekend and update the patch
|
|
Reporter | |
Comment 5•17 years ago
|
||
DOMAttrModified is triggered for both "tabbox" and "tabpanel" we need to call resetOwner only after tabbox index was changed
Assignee: nobody → onemen.one
Attachment #243797 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #266898 -
Flags: review?(gavin.sharp)
Attachment #243797 -
Flags: review?(gavin.sharp)
|
||
Updated•16 years ago
|
Whiteboard: [has patch] [needs review gavin]
|
|
||
Comment 6•16 years ago
|
||
Apologies for the delay here...
I think the test could be simplified by checking that target == self.mTabBox.
As far as I can tell, this patch is an optimization rather than a bug fix. We currently end up resetting the owner twice (once for the tabbox attrChange and once for the tabpanels attrChange) but they're both on the correct tab. Am I missing anything?
It would be real nice to have a browser chrome test that covers this case (and tab ownership resetting in general).
|
|
||
Comment 7•16 years ago
|
||
Comment on attachment 266898 [details] [diff] [review]
patch (1)
I'd r+ a patch with the changes from comment 6.
Attachment #266898 -
Flags: review?(gavin.sharp)
|
|
||
Updated•16 years ago
|
Whiteboard: [has patch] [needs review gavin]
|
|
Reporter | |
Updated•16 years ago
|
Assignee: onemen.one → nobody
|
||
Comment 8•16 years ago
|
||
Added a test for this in bug 489628, but it passed without that patch.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•