Bug 201768 - Datalist option's label not used
Summary: Datalist option's label not used
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-13 12:15 PDT by shrpne
Modified: 2020-03-31 18:23 PDT (History)
15 users (show)

See Also:


Attachments
Datalist option labels (Left: Chrome, Right: Safari) (19.25 KB, image/png)
2019-10-17 12:29 PDT, Javan Makhmali
no flags Details
Patch (38.64 KB, patch)
2020-03-31 14:29 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix GTK build (39.80 KB, patch)
2020-03-31 14:57 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix flakiness on iOS (39.90 KB, patch)
2020-03-31 16:11 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Also fix a typo (39.90 KB, patch)
2020-03-31 16:45 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 shrpne 2019-09-13 12:15:55 PDT
Option's `label` is not shown in datalist suggestions, only `value` is shown.

However, spec suggests using both `value` and `label`
https://html.spec.whatwg.org/multipage/input.html#attr-input-list

Chrome follows the spec and shows both.

Here you can find example datalist, where label "The color of the sky" is not used for value "blue".
https://css-tricks.com/datalist-is-for-suggesting-values-without-enforcing-values/
Comment 1 Radar WebKit Bug Importer 2019-09-13 20:22:11 PDT
<rdar://problem/55361186>
Comment 2 Javan Makhmali 2019-10-17 12:29:13 PDT
Created attachment 381214 [details]
Datalist option labels (Left: Chrome, Right: Safari)

Labels can provide useful context to datalist options along with additional data to search and match against. Consider the following:

<input type="email" list="people">

<datalist id="people">
  <option value="tim@apple.com">
    Tim Cook (CEO)
  </option>
  <option value="katherine@apple.com">
    Katherine Adams (Senior Vice President and General Counsel)
  </option>
  <option value="eddy@apple.com">
    Eddy Cue (Senior Vice President, Internet Software and Services)
  </option>
</datalist>

Chrome displays option values *and* labels, and matches against both when typing in the text field. For example, typing "president" matches the second and third options.

Safari only displays option values currently. Typing "president" yields no matching options.
Comment 3 Wenson Hsieh 2020-03-31 14:29:23 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-03-31 14:39:19 PDT
Comment on attachment 395090 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:41
> +static const CGFloat dropdownRowHeightWithoutLabel = 20;
> +static const CGFloat dropdownRowHeightWithLabel = 40;
> +static const size_t maximumTotalHeightForDropdownCells = 120;

As we update code to be more modern, we could use "constexpr" instead of "static const".

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:189
> +    [_bottomDivider setWantsLayer:YES];
> +    [_bottomDivider setHidden:YES];
> +    [_bottomDivider layer].backgroundColor = NSColor.separatorColor.CGColor;

If we are going to use some "." syntax then I suggest using it even more:

    _bottomDivider.wantsLayer = YES;
    _bottomDivider.hidden = YES;
    _bottomDivider.layer.backgroundColor = NSColor.separatorColor.CGColor;

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:231
> +    [_valueField setStringValue:value];
> +    [_labelField setStringValue:label];
> +    [_labelField setHidden:!label.length];

Same thought here. If we start using "." syntax in some code it would be good to go all in:

    _valueField.stringValue = value;
    _labelField.stringValue = label;
    _labelField.hidden = !label.length;

> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:254
> +    [_valueField setTextColor:backgroundStyle == NSBackgroundStyleLight ? NSColor.textColor : NSColor.alternateSelectedControlTextColor];
> +    [_labelField setTextColor:NSColor.secondaryLabelColor];

And here we are actively switching some things *away* from "." syntax, but other things *to* "." syntax in the same line of code. Seems peculiar.
Comment 5 Tim Horton 2020-03-31 14:42:16 PDT
Comment on attachment 395090 [details]
Patch

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

>> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:189
>> +    [_bottomDivider layer].backgroundColor = NSColor.separatorColor.CGColor;
> 
> If we are going to use some "." syntax then I suggest using it even more:
> 
>     _bottomDivider.wantsLayer = YES;
>     _bottomDivider.hidden = YES;
>     _bottomDivider.layer.backgroundColor = NSColor.separatorColor.CGColor;

These are RetainPtrs, so no dot notation. (and AFAIK we prefer square brackets to extraneous .get()s)
Comment 6 Darin Adler 2020-03-31 14:42:50 PDT
Comment on attachment 395090 [details]
Patch

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

>>> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:189
>>> +    [_bottomDivider layer].backgroundColor = NSColor.separatorColor.CGColor;
>> 
>> If we are going to use some "." syntax then I suggest using it even more:
>> 
>>     _bottomDivider.wantsLayer = YES;
>>     _bottomDivider.hidden = YES;
>>     _bottomDivider.layer.backgroundColor = NSColor.separatorColor.CGColor;
> 
> These are RetainPtrs, so no dot notation. (and AFAIK we prefer square brackets to extraneous .get()s)

Aha! Subtle! I endorse this.
Comment 7 Wenson Hsieh 2020-03-31 14:50:40 PDT
Comment on attachment 395090 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:41
>> +static const size_t maximumTotalHeightForDropdownCells = 120;
> 
> As we update code to be more modern, we could use "constexpr" instead of "static const".

Fixed!

>>>> Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:189
>>>> +    [_bottomDivider layer].backgroundColor = NSColor.separatorColor.CGColor;
>>> 
>>> If we are going to use some "." syntax then I suggest using it even more:
>>> 
>>>     _bottomDivider.wantsLayer = YES;
>>>     _bottomDivider.hidden = YES;
>>>     _bottomDivider.layer.backgroundColor = NSColor.separatorColor.CGColor;
>> 
>> These are RetainPtrs, so no dot notation. (and AFAIK we prefer square brackets to extraneous .get()s)
> 
> Aha! Subtle! I endorse this.

👍🏻
Comment 8 Wenson Hsieh 2020-03-31 14:57:31 PDT Comment hidden (obsolete)
Comment 9 Wenson Hsieh 2020-03-31 16:11:29 PDT
Created attachment 395106 [details]
Fix flakiness on iOS
Comment 10 Wenson Hsieh 2020-03-31 16:45:15 PDT
Created attachment 395108 [details]
Also fix a typo
Comment 11 EWS 2020-03-31 18:23:05 PDT
Committed r259330: <https://trac.webkit.org/changeset/259330>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395108 [details].