283117 - [FIX] SELECT doesn't display all options from source

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Andrew Schultz]

With linux trunk build 2005022105, many of the milestones are missing from the
SELECT dropdown on bug pages.  It seems important for one of the options to have
the "selected" attribute.

This regressed between linux trunk 2005021905 and 2005022005, indicating a
regression from bug 282607.

Comment 1

[Andrew Schultz]

Attachment: testcase

the dropdown in this page often only contains "M1" and "Future".  Other times,
it will contain all items.  (reload to hit the bug)

Comment 2

[Andrew Schultz]

Attachment: actual testcase

oops.  the previous attachment was without the "selected" attribute.

Comment 3

[Mats Palmgren (inactive)]

Yep, this is a regression from bug 282607. I will have a fix later today.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

We need to flush out any pending frames first, so they don't get redirected
as well. (They ended up at the mDisplayFrame Overflow-list).
I also moved the whole redirection thing closer around the code that actually
updates the text to avoid flushing if we don't need to.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175218 [details] [diff] [review]
Patch rev. 1

Flush_Frames can destroy |this| (and when called on the document like this can
also destroy our presshell, etc); calling it from inside frame code is no good.

That call needs to happen before we enter frame code.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

There are three paths leading up to nsComboboxControlFrame::RedisplayText():
nsHTMLSelectElement::InsertOptionsIntoList()
nsHTMLSelectElement::RemoveOptionsFromList()
nsHTMLOptionElement::NotifyTextChanged()

I added Flush_Frames to all of those instead.

Backing out bug 282607 is also a solution - your call.

Comment 7

[Boris Zbarsky [:bzbarsky]]

This will cause content sink flushes and such on every option added to a select,
no?  Just looking at the callers of InsertOptionsIntoList()....

That seems undesirable.  Either we should be smarter about when we actually need
to flush, or we should come up with a better solution here in general.

One other option would be to mess with the anonymous content off an event
instead of synchronously.  As things stand, we're reentering frame construction
from inside frame construction, right?

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 175238 [details] [diff] [review]
Patch rev. 2

(In reply to comment #7)
> One other option would be to mess with the anonymous content off an event
> instead of synchronously.  As things stand, we're reentering frame construction
> from inside frame construction, right?

Yes. I will try using an event.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 10

[Mats Palmgren (inactive)]

*** Bug 283953 has been marked as a duplicate of this bug. ***

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175672 [details] [diff] [review]
Patch rev. 3

r+sr=bzbarsky (though note that we could avoid the mComboboxControlFrame member
by just using the owner, if desired).

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Using the event owner instead of a member.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176459 [details] [diff] [review]
Patch rev. 4

>Index: layout/forms/nsComboboxControlFrame.cpp
>+    NS_STATIC_CAST(nsComboboxControlFrame*, PL_GetEventOwner(this)) ->

  NS_STATIC_CAST(nsComboboxControlFrame*, owner)

should work just fine.	;)

r+sr=bzbarsky with that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

*** Bug 284969 has been marked as a duplicate of this bug. ***

Comment 15

[Mats Palmgren (inactive)]

Checked in (using plain 'owner') at 2005-03-20 17:58 PDT

-> FIXED

Comment 16

[Hermann Schwab]

This fix seems to have caused regression Bug 288821 Select cleared on reload.