15841 - SELECT has the first option selected when it should have none

Status: VERIFIED FIXED

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

Description

[Daniel Bratell]

When opening http://bugzilla.mozilla.org/query.cgi with a current CVS build
(1999-10-08) all SELECT fields with no default have the first option selected.
They shouldn't have.

Comment 1

[karnaze (gone)]

Reassigning to Rod

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

BUG EXISTS ON:
 * Linux viewer, M10 release
 * Linux viewer 1999-10-08-08-M11

WORKS CORRECTLY ON:
 * Linux viewer 1999-09-25-08-M11
 * Linux viewer 1999-10-03-08-M11
 * Linux viewer 1999-10-05-11-M11

Raising severity to blocker since this blocks dogfood by preventing use of
bugzilla.  Yell if I shouldn't be doing this.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See the changes at line 1320 in:

http://cvs-mirror.mozilla.org/webtools/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/layout/html/forms/src&command=DIFF_FRAMESET&file=nsListControlFrame.cpp&rev1=1.69&rev2=1.70&root=/cvsroot

which seemed to introduce logic that should only be in the combobox (and was
added to the combobox to fix bug 9136 on 9/17/99 00:24) into the listbox, where
it should not be.

That is, I think the problem is currently what's at line 1337 of
layout/html/forms/src/nsListControlFrame.cpp .  I might even pull and test this
theory, but probably not.

cc:ing [e-mail]

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that seems to work

Comment 5

[rods (gone)]

This is in your new code. The patch looks good but you should review and check
it in.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Eric Pollmann]

Yep, this is my mistake.  I will check in the patch suggested by dbaron.  I'm
going to have to trace down why comboboxes with nothing initially selected
aren't getting 0 selected though, as it looks like this should be done in
nsComboboxControlFrame::InitTextStr(), but it isn't working.

Comment 8

[Eric Pollmann]

Ah, I see.  In InitTextStr, it just checks to see if
nsListControlFrame::GetSelectedItem returns "" before defaulting selection to
item 0.  In nsListControlFrame::GetSelectedItem, if nothing is selected, the
first item on the list is returned instead of "".

Since an item may actually have text of "", it might be safer to actually call
GetSelectedIndex, see if that returns -1, then set selectedindex to 0.  Rod,
does this seem like a reasonable change to you?  I tried it in my local tree and
it works fine.  (In addition to dbaron's change, of course)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: test you should check

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You should make sure the (text screen) output of the above test is sensible.
That is, it should probably be -1, -1, 0, 0, 0, both before and after the
display is changed.

(Right now, the output is 0,-1,0,0,0 before the display and 0,0,0,0,0 after it.
Back in the 8-16 build, it was -1,-1,0,0,0 and 0,-1,0,-1,-1 - that was before
you fixed bug 9136.)

BTW, in NN 4.x, a NON-MULTIPLE select with no 'selected' would automatically
have the first item selected.  However, this disagrees with HTML 4.0:
http://www.w3.org/TR/REC-html40/interact/forms.html#h-17.6.1

Comment 11

[Kathleen :Brade]

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

Comment 12

[rods (gone)]

Eric, yes to "it might be safer to actually call GetSelectedIndex, see if that
returns -1, then set selectedindex to 0"

Comment 13

[Eric Pollmann]

David, with your above-mentioned fix, plus the patch I mention, this make the
output of your testcase 0,-1,0,0,0 both before and after.  The case you make for
not preselecting option zero (-1,-1,0,0,0) is great, in that it is in line with
the spec.

We're currently deviating from the spec because both Nav and IE will preselect
option 0 in a combo with nothing selected.  This seems like reasonable behaviour
from a UI perspective, and many existing web pages are designed around this
model.

Imagine a drop-down list box with option 0 being "Select color" followed by
"red", "green", "blue", none of which are preselected.  Pages like this
frequently have an onChange handler that takes the user to a new web page when
"red", "green", or "blue" is selected.

If we follow the spec, we don't by default preselect "Select color".  A user
drops down the combo then clicks on "Select color", and onchange will be sent
out.  This breaks some existing pages.

If we follow precedent, and ignore the spec, we preselect "Select color".  A
user drops down the combo and clicks on "Select color.  Since there was no
change in selection, no onchange is sent out, and the page works.

I'm certain that the morally correct thing to do here is not to preselect
"Select color", let the people with broken pages clean them up and put the
SELECTED attribute on their first option.  Maybe the best thing to do would be
to determine if we are in Quirks mode or not before preselecting the option.
I'll look into this, as a separate bug.

Comment 14

[Eric Pollmann]

Also note that if we are to follow the spec and *not* preselect the first
option, we have another dilema on our hands.  What do we show as the text of the
drop down box when the page is first rendered?

If we show the first option, then when a form is submitted, a user would expect
the value of the first option to be submitted.  But since it is not selected, we
can't submit anything.

If we show an empty string, then cases like the "Select color" case that I
described above are going to be even more broken because the user won't even see
the string "Select color" when the page is rendered.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I didn't really want you to change that behavior - I just misunderstood your
comment at 11:48am.  Although the HTML spec does say so, I sometimes wonder how
much thought went into some parts of some specs.  (You could show a blank spot,
though, and leave it out of the form submission, for standard mode.)

Comment 16

[Eric Pollmann]

Good suggestion.  I'm opening up a new bug to track that.

I've checked in a fix for the rest of this bug.
To verify, visit the attachment I'm about to make.
The listbox should have no options selected when it is first rendered.

Comment 17

[Eric Pollmann]

New bug is bug 16157

Comment 18

[Eric Pollmann]

Attachment: Quick verification test

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 20

[cpratt]

Verified using attached test case. 1999101508 build, NT.

Comment 21

[Sean Richardson]

Suggest that regression-testing for this bug be done before M11, M12...

This bug blocked dogfood use of bugzilla in at least 2 ways: it blocked use
of the bugzilla query page with M10, and caused (and is still causing)
invalid data to be entered with new bug reports if the reporter leaves
Platform, OS, and Component as they come up using M10 (details: bug 16719).

This is no more than a hassle for anyone who knows bugzilla and form
controls well, but for first time bug reporters, likely using a milestone
release for testing, it can make using the query page an exercise in
frustration (they won't be able to guess why it's not working, never
having seen it before) and making their bug report less useful (possibly
unuseable) by corrupting some of the data unless they are observant.

Comment 22

["Zow"]

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