472020 - [FIX]xul:tab not being inserted at <children/> insertion point in tabbrowser.xml

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Nochum Sossonko [:Natch]]

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?

Comment 1

[Nochum Sossonko [:Natch]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Nochum Sossonko [:Natch]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Nochum Sossonko [:Natch]]

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. ;)

Comment 6

[Nochum Sossonko [:Natch]]

Attachment: binding

Sorry if I'm not doing this right, but here's the binding part, xul file to follow.

Comment 7

[Nochum Sossonko [:Natch]]

Attachment: xul file

This is the xul part. I think you have to download it to see the bug, but you don't need to use -chrome.

Comment 8

[Nochum Sossonko [:Natch]]

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?

Comment 9

[Nochum Sossonko [:Natch]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: XUL file pointing to the binding

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Nochum Sossonko [:Natch]]

Attachment: xml binding

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: minimized testcase

This pretty much shows the problem...

Comment 14

[Nochum Sossonko [:Natch]]

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.

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: binding

Comment 16

[Nochum Sossonko [:Natch]]

Attachment: xul file

So basically it needs to have an arrowscrollbox wrapped around the <children/> element. I'll try to reduce this further if necessary.

Comment 17

[Nochum Sossonko [:Natch]]

s/arrowscrollbox/scrollbox, it works with both (and the testcase uses a scrollbox).

Comment 18

[Boris Zbarsky [:bzbarsky]]

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...

Comment 19

[Nochum Sossonko [:Natch]]

Attachment: css+xbl+xul

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

[Dão Gottwald [:dao]]

Attachment: binding

Comment 21

[Dão Gottwald [:dao]]

Attachment: xul file

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: This helps a bit.

Things are still pretty broken, but at least this consolidates the codepaths so there's only one place we need to fix...

Comment 25

[Nochum Sossonko [:Natch]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yes.  You have to download the testcase locally and run it there.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c515b0a92558

Comment 28

[Nochum Sossonko [:Natch]]

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...

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

[Mike Kaply [:mkaply]]

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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Because I didn't realize it was there and no one bothered to remove it.

Comment 32

[Mike Kaply [:mkaply]]

(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.