Closed Bug 1498769 Opened 2 years ago Closed 2 years ago

<option> elements with "display: none" style can still be selected using the keyboard


(Core :: Layout: Form Controls, defect)

64 Branch
Not set



Tracking Status
firefox64 --- fixed


(Reporter: zrhoffman, Assigned: mats)




(2 files, 2 obsolete files)

Attached file testcase.html
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

1. Open the testcase (or visit data:text/html,<select size="2"><option>Option one</option><option style="display:none">Option two</option><option>Option three</option></select>)

2. Select "Option one"

3. Press the "down" arrow on your keyboard

Actual results:

No visible <option> element is selected.

Notice that if you press "down" a second time, "Option three" will be selected.

Expected results:

"Option three" should be selected.
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Summary: When <select> "size" attribute is greater than 1, <option> elements with display: none can still be selected → <option> elements with "display: none" style can still be selected using the keyboard
Huh, indeed. Weirdly enough we do jump over <option disabled>, so there's code handling this already which probably just needs to be extended to handle this.
Ever confirmed: true
Attached patch fix+test (obsolete) — Splinter Review
This makes us behave like Chrome for the most part.
Chrome handles display:contents "as normal" per spec though:

It appears we currently handle it as display:none, so we
need to tweak this a bit once we handle it per spec.
Assignee: nobody → mats
Attachment #9017038 - Flags: review?(enndeakin)
Attached patch patch and tests (obsolete) — Splinter Review
The code that handles disabled options is nsListControlFrame::AdjustIndexForDisabledOpt().

nsListControlFrame::KeyPress() tests for visibility already by checking if a given option has a frame. We can add that same test to nsListControlFrame::AdjustIndexForDisabledOpt() (and nsListControlFrame::GetNonDisabledOptionFrom() as long as unselecting options when they become invisible is fine).
Attachment #9017039 - Flags: review?(emilio)
Mats's fix is better, feel free to obsolete mine.
Comment on attachment 9017039 [details] [diff] [review]
patch and tests

Thanks for the patch Zach - always nice to see new contributors!
(sorry to steal this - I didn't know you were working on a fix)

Fixing it on a lower level (HTMLSelectElement::IsOptionDisabled)
seems better since then we cover all callers, not just keyboard
navigation. For example, HTMLSelectElement::SelectSomething etc.

Although, Chrome handles that case incorrectly!
data:text/html,<select size=1><option style="display:none">Option one</option><option>Option Two</option><option>Option three</option></select>
Attachment #9017039 - Attachment is obsolete: true
Attachment #9017039 - Flags: review?(emilio)
You're good. Thanks for fixing it!
I filed on Chrome
and bug 1498936 on our handling of display:contents on <option>/<optgroup>.
Comment on attachment 9017038 [details] [diff] [review]

Review of attachment 9017038 [details] [diff] [review]:

Hmm, doesn't this patch break something like setting selectedIndex on something that is out of the document or that has just been added to it but has no frames?
I think Zach's fix is slightly nicer since it only affects keyboard navigation, not DOM APIs and such. I think DOM APIs should not depend on layout.
(Also note that my comment above only applies if it breaks stuff as I describe, which I _think_ it does, but only from reading the code, not from actually applying the patch)
See Also: → 1498936
Yeah, this patch definitely breaks testing/web-platform/tests/html/semantics/forms/the-select-element/selected-index.html
Hmm, yeah I think you're right.  It seems a bit weird that a <select size=1>
auto-selects a display:none <option> that can't be selected by the user,
but it seems all UAs agrees on that behavior so...
Attachment #9017038 - Attachment is obsolete: true
Attachment #9017038 - Flags: review?(enndeakin)
Attached patch fix+testSplinter Review
Attachment #9017384 - Flags: review?(enndeakin)
Attachment #9017384 - Attachment is patch: true
Attachment #9017384 - Attachment mime type: application/octet-stream → text/plain
Attachment #9017384 - Flags: review?(enndeakin) → review+
Pushed by
Also require a frame for an <option> to be interactively selectable.  r=enndeakin
Flags: in-testsuite+
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.