Closed Bug 280754 Opened 15 years ago Closed 15 years ago

Listbox does not display all its children if more than one is moved

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: jerfa, Assigned: bzbarsky)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files)

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.
Fails in 2005012011, too. The patch for bug 244366 seems a likely cause.
Blocks: 244366
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...
Flags: blocking1.8b?
Current builds fail at "rearrange" and "insert 2".
Attached patch Proposed fixSplinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #173382 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173382 - Flags: review?(dbaron)
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
Blocks: 279400
Flags: blocking1.8b?
*** Bug 279400 has been marked as a duplicate of this bug. ***
No longer blocks: 279400
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.