166168 - Last item of <option value=""> is selected on reloading

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[HARUNAGA Hirotoshi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.1b) Gecko/20020825
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.1b) Gecko/20020825

Last item of <option value=""> is selected on reloading,
if <option selected> has value="".

Reproducible: Always

Steps to Reproduce:
1. Go to testcase:
   http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1111
2. Reload the page (without pressing Shift-key)

Actual Results:  
5th <option> item that is last one of option[value=""] is selected.

Expected Results:  
1st <option> item that has "selected" attribute should be selected.

Confirming with 2002082503-trunk/Mac OS 9.2 and 2002083005-trunk/Linux.
Mozilla/1.1 final don't have this problem.

Comment 1

[Boris Zbarsky [:bzbarsky]]

yay framestate restoration.  ;)

Comment 2

[Mitch]

Confirm failure on linux with cvs 20020902

Comment 3

[John Keiser (jkeiser)]

Error why?  How are those options even distinguishable except by text?  What
would happen if you rearranged the options the second time the page was shown? 
Please to be explaining in what context you are actually using this.

My two bits: if two options will submit to the server in exactly the same way,
they are the same option.

Comment 4

[HARUNAGA Hirotoshi]

This is not submission error, but strange behavior for browser users.
option[value=""] item may be used instead of <optgroup>.

for example:
<select>
<option value="" selected>Select one of following</option>
<option value="">----Mozilla user: select one of following----</option>
<option value="a">a</option>
<option value="b">b</option>
<option value="">----IE user: select one of following----</option>
<option value="c">c</option>
<option value="d">d</option>
</select>

Comment 5

[John Keiser (jkeiser)]

All right, it's doable at least.  Not critical but worthwhile cleanup.

Comment 6

[John Keiser (jkeiser)]

This fixes it.  I also took the liberty of creating an nsCheapStringSet and
nsCheapInt32Set, which don't bother creating a hashtable until there is more
than 1 int / string.  I also moved the HashSets into their own nsHashSets.h and
created an nsCheapSets.h.  This should improve bloat / performance a lot on
select restore and hopefully will help others out who need similar performance
characteristics (I see this as similar to nsCheapVoidArray for a different set
of clients).  These new classes comprise the bulk of this patch: CC'ing dougt,
scc and brendan, to whom I have spoken about this.

dougt, are the name changes all right?  Any of you three want to review? :)

BTW, there are some debugging regression tests in nsHTMLSelectElement.cpp for
these classes, to make sure I did them right.  They pass those tests and
restoring works as it should.  I will remove them before checkin but wanted to
show what testing was done.

Comment 7

[John Keiser (jkeiser)]

Attachment: Patch

OK, I'm a limey non-patch-attachin' bastard.  scc, dougt, brendan? ;)

Comment 8

[John Keiser (jkeiser)]

dougt has approved moving stuff around in xpcom/ds, and scc too.  So that leaves
getting a review.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

+    mValOrHash = NS_INT32_TO_PTR(
+                     NS_PTR_TO_INT32((nsAString*) new nsString(aVal)) | 0x1);

     That will not work correctly on a 64-bit machine.  (There is another
instance of this in the patch).  You probably need to use PtrBits like in
nsVoidArray.cpp/etc.

    You also should comment/warn (like in nsVoidArray) that this only works on
machines where new/malloc/etc return even ((ptr&1) == 0) pointers.

+++ xpcom/ds/nsVoidArray.cpp	9 Sep 2002 23:44:50 -0000
@@ -1,4 +1,4 @@
-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2;
c-file-offsets: ((substatement-open . 0)) -*- */
+ /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2;
c-file-offsets: ((substatement-open . 0)) -*- */

    That looks like a typo

+  {
+    nsCheapInt32Set set;
+    NS_ASSERTION(!set.Contains(0), "empty set failed!");
+    set.Put(1);
+    NS_ASSERTION(set.Contains(1) && !set.Contains(0), "set with one failed");
+    set.Remove(1);
+    NS_ASSERTION(!set.Contains(1) && !set.Contains(0), "remove from one failed");
+    set.Put(12);

(etc)

     That looks like debug code that should have been stripped.

Fix those (include the 64-bit issue), and r=[e-mail]

Comment 10

[John Keiser (jkeiser)]

Attachment: Patch v1.1

Fix Kin's review.

Comment 11

[John Keiser (jkeiser)]

Sorry, that was Randell's review.

Comment 12

[John Keiser (jkeiser)]

Comment on attachment 99178 [details] [diff] [review]
Patch v1.1

(Carrying r=rjesup)

Comment 13

[m_mozilla]

confirming that I see it on reload in
    mac os x trunk build 2002091603

also noting that I don't see it when I shift-reload
...don't know if that is useful information or not

-matt

Comment 14

[John Keiser (jkeiser)]

Attachment: Patch v1.2

This patch makes most methods in CheapSet non-inline.  jst, I think you had
another nitpick but I can't remember what it was.

Comment 15

[John Keiser (jkeiser)]

CC'ing jst for sr, didn't realize he wasn't here

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 100522 [details] [diff] [review]
Patch v1.2

- In nsCheapStringSet::InitHash():

+  nsStringHashSet* newSet = new nsStringHashSet();
+  newSet->Init(10);

OOM -> crash. Check for non-null newSet and react accordingly. Same thing in
all InitHash() methods.

- In nsCheapInt32Set::~nsCheapInt32Set():

+  if (GetHash()) {
+    delete GetHash();
+  }

The if-check is pointless, delete is null safe. If you remove it, it's one less
call...

- In nsCheapInt32Set::Put()

+  if (GetHash()) {
+    return GetHash()->Put(aVal);

This really looks wrong in my eyes, no matter how fast GetHash() is, it's IMO
worth storing in a local and using the local in stead of making two calls to
GetHash(). Keep in mind, there are some really lame optimizers out there.

...

+    InitHash();
+
+    nsresult rv = GetHash()->Put(oldInt);

OOM -> crash. No checking that InitHash() succeeded in creating a hash.

+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return GetHash()->Put(aVal);

And come on, store the hash in a local in stead of calling GetHash() twice in a
row! :-) There's a ton of these, I'd like to see them all changed.

...

+  if (aVal < 0) {
+    InitHash();
+
+    return GetHash()->Put(aVal);

Same thing, OOM check...

Other than that it looks good, but I want to see a new diff.

Comment 17

[John Keiser (jkeiser)]

Attachment: Patch v1.2.1

Good catch on that OOM stuff.  Everything fixed.  Also made InitHash() return
the set it created so you don't have to waste a few instructions grabbing it
again.

Comment 18

[John Keiser (jkeiser)]

Attachment: Patch Patch v1.2.2

This is a littlediff with the stuff I missed in 1.2.1.	Apply on top of 1.2.1
to get it.

Comment 19

[John Keiser (jkeiser)]

Attachment: Patch Patch v1.2.2 (really)

Heh, fun with diff :)  This patch is not reversed like the other one :)

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 101496 [details] [diff] [review]
Patch Patch v1.2.2 (really)

sr=jst

Comment 21

[John Keiser (jkeiser)]

Fix checked in.