Closed Bug 280754 Opened 15 years ago Closed 15 years ago
Listbox does not display all its children if more than one is moved
Testcase is attachment 173089 [details] : The listbox has 6 listitems, three of them are moved using insertBefore. Only the last one moved is rendered. This is a fairly recent regression, it works in 1.8b 2005011816 and fails in 20050125. FWIW, FF 1.0 also fails the testcase: it renders all six elements but in the wrong order, 1-4-2-5-36. Correct is 4-3-1-5-2-6.
bz said so in bug 276698 comment 10, but you beat me to it ;-)
So we have a problem here. Listbox does this bizarre lazy frame creation. To handle content insertion and removal and the like it sets some flags that can only handle _one_ content node and then does a synchronous reflow to force frame creation. All this is happening from inside frame construction. Now reflow from inside frame construction crashes, in general. It may not in a pure-XUL document, but I could pretty easily write up some testcases where it would happen using mixed-namespace documents. The patch in bug 244366 added some safety-checks that prevent reflow flushes inside frame construction. Hence this bug, since the listbox can only handle one content at a time. I've sent mail to hewitt in an attempt to figure out exactly what this code is trying to accomplish, but if anyone happens to know, please share that knowledge...
Current builds fail at "rearrange" and "insert 2".
This fixes the bug (and I tested it on the various DOM manipulation, and it seems to do fine). The basic change is that on ContentRemoved we don't need to flush reflow at all, and on ContentInserted we replace flushing reflows by a direct call to CreateRows. That constructs the new frames, if they are needed.
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta
a) Can we move the declaration of "state" down a bit? b) What about the other calls to Flush (especially the XXXbz one!)
That one only happens when we're scrolling the list via the scrollbar mediator stuff, I think... I'd be perfectly happy to remove those calls, on general principle, but I'm not sure I'd be happy doing that for 1.8b1 at this point. Separate bug, perhaps?
Attachment #173382 - Flags: review?(dbaron) → review+
Comment on attachment 173382 [details] [diff] [review] Proposed fix > nsBoxLayoutState state(mPresContext); > > // now create or destroy any rows as needed >- CreateRows(state); >+ CreateRows(); So, could the declaration be moved nearer the remaining use?
Attachment #173382 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
> So, could the declaration be moved nearer the remaining use? Yeah, will do.
Fix checked in. I've filed bug 281147 on removing the remaining flushes.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
*** Bug 279400 has been marked as a duplicate of this bug. ***
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.