Closed
Bug 472020
Opened 16 years ago
Closed 15 years ago
[FIX]xul:tab not being inserted at <children/> insertion point in tabbrowser.xml
Categories
(Core :: XBL, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: Natch, Assigned: bzbarsky)
References
()
Details
(Keywords: testcase)
Attachments
(3 files, 8 obsolete files)
551 bytes,
text/xml
|
Details | |
547 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
16.32 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
While patching bug 457651 I had to use <children includes="tab"/> to get the tabs inserted at the <children/> point, otherwise new tabs (added via appendChild on the element that it's bound to) were being inserted after the new tab button, is this ok?
Reporter | ||
Updated•16 years ago
|
Reporter | ||
Comment 1•16 years ago
|
||
Also note, if this behavior is incorrect there may some perf loss here, as I imagine the xpath query when using | includes | is somewhat slower than just a generic, catch-all children tag.
Assignee | ||
Comment 2•16 years ago
|
||
Is it possible to attach a smallish XUL file testcase (using <tabbrowser> if needed, running with -chrome)? "The whole Firefox UI" isn't very nice to work with. It's odd that the includes= would affect this; otherwise this looks a lot like bug 272646.
Reporter | ||
Comment 3•16 years ago
|
||
Not really sure how to make a testcase, adding keyword. Also, bug 272646 is about insertBefore, tabbrowser.xml uses appendChild although that may be the same.
Keywords: testcase-wanted
Assignee | ||
Comment 4•16 years ago
|
||
appendChild() basically calls insertBefore(null) internally. A testcase would be what I said: a XUL file with a tabbrowser, which makes the relevant appendChild calls. If that shows the problem, great. If it doesn't, then what's different about the Firefox UI?
Reporter | ||
Comment 5•16 years ago
|
||
Problem is I have to sneak in a toolbarbutton into the tabs binding (basically the Firefox tabs is bound to an arrowscrollbox which has the <children/> element, after that element I put in a toolbarbutton and the tabs were consistently being appended after the toolbarbutton), which is proving a little difficult for me atm. ;)
Reporter | ||
Comment 6•16 years ago
|
||
Sorry if I'm not doing this right, but here's the binding part, xul file to follow.
Reporter | ||
Comment 7•16 years ago
|
||
This is the xul part. I think you have to download it to see the bug, but you don't need to use -chrome.
Reporter | ||
Updated•16 years ago
|
Keywords: testcase-wanted → testcase
Reporter | ||
Comment 8•16 years ago
|
||
fwiw, adding the tabs via an external button (i.e. not from the binding itself) also exhibits this problem. Is that what bug 272646 about? Can that bug also be fixed with the includes= hack?
Reporter | ||
Comment 9•16 years ago
|
||
Could it be that there's confusion with http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml?mark=247#227 , the includes= hack would explain that as it would make the tabbrowser.xml's children tag more specific.
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #355337 -
Attachment is obsolete: true
Assignee | ||
Comment 11•16 years ago
|
||
OK, well. The build I have which has the fix for bug 272646 in it does NOT fix this bug. Confirming bug. It would be awfully nice to have a standalone testcase (that is, one that doesn't rely on the browser tab binding)... Even better to have one that's reduced, though that's hard to work on until the patch in bug 307394 lands.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 12•16 years ago
|
||
Attachment #355336 -
Attachment is obsolete: true
Reporter | ||
Comment 13•16 years ago
|
||
This pretty much shows the problem...
Attachment #357756 -
Attachment is obsolete: true
Reporter | ||
Updated•16 years ago
|
Attachment #359581 -
Attachment is obsolete: true
Reporter | ||
Comment 14•16 years ago
|
||
Comment on attachment 359584 [details]
minimized testcase
These are actually invalid as it should be calling this.parentNode.appendChild(...), I'll continue trying to nail this down.
Attachment #359584 -
Attachment is obsolete: true
Reporter | ||
Comment 15•16 years ago
|
||
Reporter | ||
Comment 16•16 years ago
|
||
So basically it needs to have an arrowscrollbox wrapped around the <children/> element. I'll try to reduce this further if necessary.
Reporter | ||
Comment 17•16 years ago
|
||
s/arrowscrollbox/scrollbox, it works with both (and the testcase uses a scrollbox).
Assignee | ||
Comment 18•16 years ago
|
||
If you can make it not depend on the scrollbox binding (so just use a different element name with a similar binding), that would be great...
Reporter | ||
Comment 19•16 years ago
|
||
I can't make it available online since all the files depend on the url another file will get so I zipped it up, all made up elements.
Comment 20•16 years ago
|
||
Attachment #359604 -
Attachment is obsolete: true
Attachment #359606 -
Attachment is obsolete: true
Attachment #359634 -
Attachment is obsolete: true
Comment 21•16 years ago
|
||
Assignee | ||
Comment 22•16 years ago
|
||
Hmm. That last testcase is NOT fixed by the checkin for bug 307394. I'll do some digging into what's going on with it.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 23•16 years ago
|
||
Alright. So this case has nothing to do with bug 307394 because in this case the insertion point data structure is actually broken. In particular, the box ends up after the toolbarbutton in the nsXBLInsertionPoint. The issue is that nsBindingManager::ContentAppended doesn't really handle the case when the nested insertion point is not directly under the given content correctly. In particular, it will append kids to the end of a non-multiple insertion point. That's wrong if the insertion point really is nested, since in that case it can contain content after our DOM kids (content from the inner binding, as in this case). Note that there are some XXX comments in HandleChildInsertion about all this not working right if there are no existing kids in the parent, so even if we made ContentAppended follow the HandleChildInsertion() codepath in this case (which I think we should) it would break as soon as you removed the one <box> that's already in the DOM... We really need a better data structure of some sort for managing the flattened tree here. :(
Assignee | ||
Comment 24•16 years ago
|
||
Things are still pretty broken, but at least this consolidates the codepaths so there's only one place we need to fix...
Attachment #360041 -
Flags: superreview?(jonas)
Attachment #360041 -
Flags: review?(jonas)
Assignee | ||
Updated•16 years ago
|
Summary: xul:tab not being inserted at <children/> insertion point in tabbrowser.xml → [FIX]xul:tab not being inserted at <children/> insertion point in tabbrowser.xml
Reporter | ||
Comment 25•15 years ago
|
||
I'm getting: Security Error: Content at https://bug472020.bugzilla.mozilla.org/attachment.cgi?id=359647 may not load data from https://bugzilla.mozilla.org/attachment.cgi?id=359646#outer. on 3.0.8 now with the testcase.
Assignee | ||
Comment 26•15 years ago
|
||
Yes. You have to download the testcase locally and run it there.
Attachment #360041 -
Flags: superreview?(jonas)
Attachment #360041 -
Flags: superreview+
Attachment #360041 -
Flags: review?(jonas)
Attachment #360041 -
Flags: review+
Assignee | ||
Comment 27•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/c515b0a92558
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 28•15 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090408 Minefield/3.6a1pre Note: this fixed the testcase bug, not the tabbrowser bug...
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 29•15 years ago
|
||
If we need a new bug with a better testcase that better reflects the tabbrowser issue, please file it! Presumably one of the "fails" reftests here represents that issue better?
Comment 30•9 years ago
|
||
If this is fixed, why is this code: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3759 Still in tabbrowser.xml?
Assignee | ||
Comment 31•9 years ago
|
||
Because I didn't realize it was there and no one bothered to remove it.
Comment 32•9 years ago
|
||
(In reply to Not doing reviews right now from comment #31) > Because I didn't realize it was there and no one bothered to remove it. I'll open a separate bug and put together a patch.
You need to log in
before you can comment on or make changes to this bug.
Description
•