Closed Bug 286170 Opened 17 years ago Closed 16 years ago

User cannot select an OPTION after javascript deletes or adds some others

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: crawley, Assigned: mats)

References

Details

(Keywords: testcase, Whiteboard: [sg:nse])

Attachments

(6 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040803
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040803

When a web page uses Javascript to delete unwanted OPTION elements from a 
SELECT, the user may not be able to select other options from the menu,
depending on their position.

Reproducible: Always

Steps to Reproduce:
1. Obtain demo web page.  (I will try to attach it to the submitted bug ...)
2. Follow instructions on page.
3.

Actual Results:  
The user is unable to select the "Equal to" entry in the first menu.
But if the user selects another entry first, the "Equal to" entry
can then be selected.

Expected Results:  
The user should be able to select "Equal to" every time.

I will try to attach the demo web page to the submitted bug.  If
that fails, please contact me.
Attached file A web page that demonstrates the bug (obsolete) —
I should add that this bug was identified (and the demo page was written) by
Steve Crow at DSTC.
Summary: User cannot select an OPTION after javascript deletes some others → User cannot select an OPTION after javascript deletes some others
testcase WFM in Firefox 1.0.1, what am I missing? Could you retest with a more
recent build?

Not an exploit, clearing security-confidential flag.
Group: security
Whiteboard: [sg:nse]
Arghhh.  I did a 'SaveAs' to snarff the demobug.html.  This has saved a copy
of the DOM after the Javascript has done its surgery on it ... and that defeats
the test.  I'll replace the file for you.
Attachment #177448 - Attachment is obsolete: true
Attachment #177453 - Attachment mime type: text/plain → text/html
Hmm... What's special about the "Equal To" option?  The text?  The value?  Its
position in the <select>?
Six options are removed, "Equal To" is the sixth element following the selected
element. I'm guessing the control thinks this element is the selected element.
Assignee: general → nobody
Status: UNCONFIRMED → NEW
Component: DOM → Layout: Form Controls
Ever confirmed: true
QA Contact: ian → layout.form-controls
Attached file Testcase #2
If you select the "unselectable" option and then drop down the menu again it
is actually hightlighted. Also, the DOM 'selectedIndex' is correct too,
so the only thing that does not work is the updating of the text in the
select control.
Attached file Testcase #3
Assignee: nobody → mats.palmgren
Keywords: testcase
OS: Linux → All
Hardware: PC → All
Summary: User cannot select an OPTION after javascript deletes some others → User cannot select an OPTION after javascript deletes or adds some others
Interesting.... In testcase #2, after load the selectedIndex is 0, but if I
click twice on the select (to drop it down and roll it back up), it becomes 1...
 Sounds to me like some miscommunication between content and frame.
So the problem is that the combobox has an mDisplayedIndex member that gets out
of sync with the mSelectedIndex member of the <select> in this case....  We
should be updating that member when the DOM mutates.

Mats, do you have time to take a stab at this by any chance?
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attachment #179088 - Flags: superreview?(bzbarsky)
Attachment #179088 - Flags: review?(bzbarsky)
Comment on attachment 179088 [details] [diff] [review]
Patch rev. 1

>Index: layout/forms/nsComboboxControlFrame.cpp
>+    // When aIndex == mDisplayedIndex the element will select something else,
>+    // see nsHTMLSelectElement::RemoveOptionsFromList().

That's only true if mDisplayIndex is always the same as the select's
mSelectedIndex (in which case there is really no need for mDisplayIndex..).

But the two are out of sync if someone tabs to a combobox and hits the down
arrow, say, no?  If the DOM is then mutated (off a timer, say) and an option is
removed, what behavior do we want, exactly?  I think we want to preserve the
current user selection as much as possible (so most of this patch is right),
but do something sane in the case when our displayed option is removed.
Attached file Testcase #6
Attached file Testcase #7
Attached patch Patch rev. 2Splinter Review
> ... do something sane in the case when our displayed option is removed.

I chose to do what IE6/win32 does - select the first option (Testcase #7).

I had a larger patch at first that addressed the case when the hovered option
was removed in a dropdown (Testcase #6) - it re-highlighted what was under the
mouse after the change, but I chose to be compatible with IE6 here also -
select the first option (which is the current behaviour).
The "IsCombobox()" is a leftover from that version...
Attachment #179325 - Flags: superreview?(bzbarsky)
Attachment #179325 - Flags: review?(bzbarsky)
Attachment #179088 - Attachment is obsolete: true
Attachment #179088 - Flags: superreview?(bzbarsky)
Attachment #179088 - Flags: review?(bzbarsky)
Comment on attachment 179325 [details] [diff] [review]
Patch rev. 2

r+sr=bzbarsky.	Thanks for fixing this and for cleaning up the content code
some in the bargain!
Attachment #179325 - Flags: superreview?(bzbarsky)
Attachment #179325 - Flags: superreview+
Attachment #179325 - Flags: review?(bzbarsky)
Attachment #179325 - Flags: review+
Attachment #179325 - Flags: approval1.8b3?
Attachment #179325 - Flags: approval1.8b3? → approval1.8b3+
Checked in to trunk at 2005-06-20 16:26 PDT

-> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I tested with testcases 1 through 7, and all are working fine for me using build
2005-07-07-06 of SeaMonkey trunk on Windows XP.

Verified FIXED.
Status: RESOLVED → VERIFIED
Depends on: 313260
No longer depends on: 313260
Duplicate of this bug: 236539
Can we get tests for this checked in?  This is historically very fragile behavior that we should be testing for, imo.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.