Closed Bug 1498769 Opened 2 years ago Closed 2 years ago
<option> elements with "display: none" style can still be selected using the keyboard
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
This makes us behave like Chrome for the most part. Chrome handles display:contents "as normal" per spec though: https://drafts.csswg.org/css-display/#unbox-html 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)
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).
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>
You're good. Thanks for fixing it!
I filed https://bugs.chromium.org/p/chromium/issues/detail?id=895171 on Chrome and bug 1498936 on our handling of display:contents on <option>/<optgroup>.
Comment on attachment 9017038 [details] [diff] [review] fix+test 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)
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...
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffba3ac1d2bb Also require a frame for an <option> to be interactively selectable. r=enndeakin
You need to log in before you can comment on or make changes to this bug.