Bug 199485 - Single selection <select> with <optgroups> shows multiple selected options
Summary: Single selection <select> with <optgroups> shows multiple selected options
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari 12
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 211987 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-07-03 17:34 PDT by Joao
Modified: 2020-05-18 13:02 PDT (History)
10 users (show)

See Also:


Attachments
Multiple selected option on a single-selection <select> (101.03 KB, image/png)
2019-07-03 17:34 PDT, Joao
no flags Details
Patch (23.21 KB, patch)
2020-05-17 00:32 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joao 2019-07-03 17:34:49 PDT
Created attachment 373443 [details]
Multiple selected option on a single-selection <select>

For a <select> using <optgroup>'s and without multiple option, user is able to select multiple items. Reproduced on an iPhone 12.3.1 but I can see the same behaviour in previous versions.

How to reproduce:
1. Go to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/optgroup#Example (or any other website with select/optgroup)
2. Select first option
3. Scroll all the way down to the last option
4. Select last option

Both options are now selected.

The trick to reproduce seems to be scrolling enough to hide the current selection from the screen and then select another option.

It seems a visual bug as JavaScript correctly indicates the current selection. Also using JavaScript to un-select all options but the current one doesn't seem to work (as a workaround).

Screenshot in attachment.
Comment 1 Radar WebKit Bug Importer 2019-07-07 17:52:27 PDT
<rdar://problem/52757531>
Comment 2 Wenson Hsieh 2020-05-16 14:39:12 PDT
*** Bug 211987 has been marked as a duplicate of this bug. ***
Comment 3 Wenson Hsieh 2020-05-16 18:01:03 PDT
This happens because -pickerView:row:column:checked: is not invoked for non-visible picker items, so if an item is checked, is scrolled offscreen, and then another item is checked, we will never update the content view’s FocusedElementInformation to reflect the fact that the previously checked item should now be unchecked.

This is pretty simple to fix — we just need to change the `item.isSelected = false;` line in -pickerView:row:column:checked: (in WKFormSelectPicker.mm) to uncheck whatever item was previously checked.

…writing a test for this is going to require some finagling, though :|
Comment 4 James Please 2020-05-16 19:09:15 PDT
Thanks for taking the time to look into this and offer such a detailed writeup, Wenson! I really appreciate it.

Let me know if I can help out in any way. I found the source on the GitHub mirror, but admittedly I'm not too familiar with this programming language so I'm not sure if I can be useful. I'm willing to give it a try, though!

I view this as a high priority bug because it's going to be difficult for anyone who wishes to use the platform to justify using native selects+optgroups if they're aware of this bug.

Thanks again!
Comment 5 Wenson Hsieh 2020-05-17 00:32:55 PDT
Created attachment 399585 [details]
Patch
Comment 6 EWS 2020-05-18 09:23:33 PDT
Committed r261815: <https://trac.webkit.org/changeset/261815>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399585 [details].
Comment 7 Darin Adler 2020-05-18 12:59:41 PDT
Comment on attachment 399585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399585&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8763
> +    if ([_inputPeripheral isKindOfClass:[WKFormSelectControl self]])

Normally I see people write one of these:

    [WKFormSelectControl class]
    WKFormSelectControl.class

I didn’t know that [WKFormSelectControl self] worked. I did a search and see 44 uses of +self in WebKit. But 1083 uses of +class.
Comment 8 Wenson Hsieh 2020-05-18 13:02:31 PDT
(In reply to Darin Adler from comment #7)
> Comment on attachment 399585 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399585&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:8763
> > +    if ([_inputPeripheral isKindOfClass:[WKFormSelectControl self]])
> 
> Normally I see people write one of these:
> 
>     [WKFormSelectControl class]
>     WKFormSelectControl.class
> 
> I didn’t know that [WKFormSelectControl self] worked. I did a search and see
> 44 uses of +self in WebKit. But 1083 uses of +class.

Good point. I just did this to match the style of -selectFormAccessoryPickerRow: above, but we should really just standardize on one (i.e. +class) and use it throughout WebKit.

I’ll address this up in a followup.