319326 - [FIX]Form <option> list javascript repopulate causes initial text to be blank

Status: RESOLVED FIXED

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

Description

[Matthew Schultz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915

Focusing on an option list right after that list has been repopulated using javascript causes the initial text to be blank.  It appears that that javascript sees the initial text and value of the first option, it is simply not painted on the screen properly.

This bug also exists on Firefox 1.5 (winxp).
Works as expected on Opera 8.51 and IE 6 SP2 (winxp).

Reproducible: Always

Actual Results:  
The list is repopulated but the first option text is blank.

Expected Results:  
List should be repopulated and not create a "ghost" blank option.

Comment 1

[Matthew Schultz]

Attachment: Option List Repopulate Testcase

Comment 2

[Matthew Schultz]

It seems this was fixed and then somehow regressed?  I did a little testing and on Seamonkey build 2005101005, the javascript repopulation works as expected but when I tested Seamonkey build 2005120508, the repopulation is broken again.

Comment 3

[Andrew Schultz]

confirmed with linux seamonkey trunk 2005120604

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 5

[Boris Zbarsky [:bzbarsky]]

Though note also bug 330554.  It seems to me that if we're coming from a selected index reset (whether scripted selectedIndex set, or when mSelectedIndex used to be -1) we should always update the display area, no?

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 276155 [details] [diff] [review]
Proposed fix

This would regress selectedIndex=-1 from script, hovering the dropdown
would then auto-selects an option, which I think is not desired.
The patch in bug 330554 looks good though, so I'll continue
the discussion over there.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hold on.  What would regress, exactly?  Bug 330554 testcase 3, which of the tests and what steps to reproduce?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I guess "onfocus= populate an empty select, and then set selectedIndex=-1" and then hover the list.... ok.

This should be simple to address.  Modified patch coming up.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix that problem

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #2

An improvement, but I still think something is fundamentally wrong
with the way we handle selectedIndex.  Here's a testcase to illustrate.
(it's a subset of bug 330554 Testcase #3, with extra tracing)

I don't think selectedIndex should change at all from just hovering -
IE7, Opera9 and Safari/Webkit don't change it, only if you actually
select something.  This bug also occur in Firefox 2.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That's because nsHTMLSelectElement::OnOptionSelected always sets the selected index if the current one is < 0, even if the call is coming from the frame.  Note that for a click in a listbox or combobox we _do_ want to do that.

This code is a mess.  :(

In any case, that problem is somewhat separate from this bug...

Comment 12

[Mats Palmgren (inactive)]

Boris, sorry for being difficult, but do you have a testcase where the
patch actually makes a difference?
I tried a lot of different tests and I couldn't find a single one
(now that bug 330554 is fixed).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Ah, hmm.  I guess SelectSomething calls SetSelectedIndex(), so this is in fact fixed by the checkin for bug 330554.  Much better than trying to get the conditions here to be right.  ;)

Fixed by that patch; I'll try to write a test for this based on the initial testcase in this bug.