272646 - tabs insertBefore lastChild random insert

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[andreas]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20041008
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20041008

The attached snipplet (follows) should append a <tab> to a tabbox, using
insertBefore(newnode,lastChild).

Problem: The new <tab> is inserted in a strange way. (First before first, then
before last, ..)

Checked in Firefox 1.0, Mozilla 1.8 + 1.7

Reproducible: Always
Steps to Reproduce:
1. open attached testcase
2. click 'add tab' a few times
3. profit ;-)

Actual Results:  
Tabs get inserted but not at expected position.

Expected Results:  
Insert the new <tab> element right before the last node in <tabs>.

Hope this is a coding error on my side ;-)

I put it on major - just because it's an issue with my xul app. If there's a
workaround, feel free to downgrade.

Comment 1

[andreas]

Attachment: Testcase

Comment 2

[andreas]

Attachment: Testcase2 - replaceChild + append

foo = replaceChild(new,lastviaid); appendChild(foo);
doesn't work neither

Comment 3

[andreas]

Attachment: Testcase3 remove append append works

old = removeChild(foo); appendChild(new); appendChild(old);
Works... seems like a workaround ;-)

Comment 4

[andreas]

Downgrading severity. Workaround found.

Comment 5

[Andrew Schultz]

I see the bug with the first testcase using linux trunk 2004120106, but
testcase2 worksforme.

Comment 6

[Xavier]

I have the same problem on a Windows platform, with Firefox 1.0, using 
insertBefore (to implement drag and drop).
With 3 tab elements A, B, C, removing C and then insertBefore B, C ends up 
before A. But the DOM explorator is OKAY !!! The DOM says C is between A and B.

In the code below, replace tab elements with textbox elements and all works 
fine when you click the button:

<?xml version="1.0"?>
<window id="reivax" title="reivax" windowtype="reivax:main"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
   <tabbox>
      <tabs id="t">
         <tab id="A" label="A"/>
         <tab id="B" label="B"/>
         <tab id="C" label="C"/>
      </tabs>
   </tabbox>
   <button onclick="go()" label="go"/>
   <script type="application/x-javascript">
      <![CDATA[
      function go() {
         var C = document.getElementById('C');
         var B = document.getElementById('B');
         var t = document.getElementById('t');
         
         var newTab = t.removeChild(C);
         newTab = t.insertBefore(newTab, B);
      }
      ]]>
   </script>
</window>

Comment 7

[Neil Deakin]

Attachment: Possible patch

This issue appears when you have:

<xul:box>
  <sometag/>
  <children/>
</xul:box>

The frame insertion code isn't accounting for <sometag> when inserting at the
insertion point.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Neil, it'll take me a few days to get to this... does this happen to fix bug
261826, too?

Comment 9

[Neil Deakin]

Heh. I thought I'd seen a similar bug before.

Yes, that bug is fixed by this patch too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So... if you add the assert patch in that bug to your tree, does the assert fire
when opening menus from the menubar?

Note that it looks to me like either this patch or the documentation about what
this index is in nsIBindingManager is wrong...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 168228 [details] [diff] [review]
Possible patch

Marking r- to get answer to question in comment 10.

Comment 13

[Neil Deakin]

It would be nice to fix this, but not sure if it's worth fixing this now, or waiting for xbl2. There are two issues, first is that the index passed to FindPrevious/NextSibling isn't accurate. That's bug 307394. The other is that the code in nsBindingManager::ContentAppended/ContentInserted doesn't create child lists properly when there are multiple insertion points. Not sure if there's a bug on that, but there are comments in the code to that effect.

Comment 14

[Daniel Kirsch]

Something similar happends to tab elements that gets hidden and shown again. When a hidden tab gets shown again, it will be shown before it's previous sibling when beeing the last tab in the tabbox.
Current workaround: use "collapsed" instead of "hidden"

Comment 15

[Daniel Kirsch]

Attachment: Testcase for hiding and showing tab elements

Hiding the last tab in the testcase and showing it again, moves the tab before it's original previous sibling.
For the "best experience", hide and show the "extension" tab first than hide/show the "overview" tab.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed by checkin for bug 307394.  Checked in test.

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39400e032624
Backed out changeset fc813bf68348 for failing reftest layout/reftests/bugs/272646-1.xul on OS X. r=backout