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

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
P1
major
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Andrew Schultz, Assigned: mats)

Tracking

({regression, testcase})

Trunk
x86
Linux
regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 175097 [details]
testcase

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

Comment 2

13 years ago
Created attachment 175098 [details]
actual testcase

oops.  the previous attachment was without the "selected" attribute.
Attachment #175097 - Attachment is obsolete: true
Flags: blocking1.8b2?
(Assignee)

Comment 3

13 years ago
Yep, this is a regression from bug 282607. I will have a fix later today.
Blocks: 282607
Severity: normal → major
Status: NEW → ASSIGNED
No longer depends on: 282607
Priority: -- → P1
(Assignee)

Comment 4

13 years ago
Created attachment 175218 [details] [diff] [review]
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.
Attachment #175218 - Flags: superreview?(bzbarsky)
Attachment #175218 - Flags: review?(bzbarsky)
(Assignee)

Updated

13 years ago
Summary: SELECT doesn't display all options from source → [FIX] SELECT doesn't display all options from source
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.
Attachment #175218 - Flags: superreview?(bzbarsky)
Attachment #175218 - Flags: superreview-
Attachment #175218 - Flags: review?(bzbarsky)
Attachment #175218 - Flags: review-
(Assignee)

Comment 6

13 years ago
Created attachment 175238 [details] [diff] [review]
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.
Attachment #175218 - Attachment is obsolete: true
Attachment #175238 - Flags: superreview?(bzbarsky)
Attachment #175238 - Flags: review?(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?
(Assignee)

Comment 8

13 years ago
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.
Attachment #175238 - Attachment is obsolete: true
Attachment #175238 - Flags: superreview?(bzbarsky)
Attachment #175238 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

13 years ago
Created attachment 175672 [details] [diff] [review]
Patch rev. 3
Attachment #175672 - Flags: superreview?(bzbarsky)
Attachment #175672 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

13 years ago
*** Bug 283953 has been marked as a duplicate of this bug. ***
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).
Attachment #175672 - Flags: superreview?(bzbarsky)
Attachment #175672 - Flags: superreview+
Attachment #175672 - Flags: review?(bzbarsky)
Attachment #175672 - Flags: review+
(Assignee)

Comment 12

13 years ago
Created attachment 176459 [details] [diff] [review]
Patch rev. 4

Using the event owner instead of a member.
(Assignee)

Updated

13 years ago
Attachment #175672 - Attachment is obsolete: true
Attachment #176459 - Flags: review?(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.
Attachment #176459 - Flags: superreview+
Attachment #176459 - Flags: review?(bzbarsky)
Attachment #176459 - Flags: review+
*** Bug 284969 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 15

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

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 16

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

Updated

13 years ago
Flags: blocking1.8b2?
You need to log in before you can comment on or make changes to this bug.