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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Zach Hoffman [:zrhoffman]]

Attachment: 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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix+test

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.

Comment 3

[Zach Hoffman [:zrhoffman]]

Attachment: patch and tests

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).

Comment 4

[Zach Hoffman [:zrhoffman]]

Mats's fix is better, feel free to obsolete mine.

Comment 5

[Mats Palmgren (inactive)]

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>

Comment 6

[Zach Hoffman [:zrhoffman]]

You're good. Thanks for fixing it!

Comment 7

[Mats Palmgren (inactive)]

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 8

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(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)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yeah, this patch definitely breaks testing/web-platform/tests/html/semantics/forms/the-select-element/selected-index.html

Comment 12

[Mats Palmgren (inactive)]

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...

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix+test

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffba3ac1d2bb
Also require a frame for an <option> to be interactively selectable.  r=enndeakin

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ffba3ac1d2bb