Bug 201768

<rdar://problem/55361186>

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

Created attachment 395106 [details] Fix flakiness on iOS

Created attachment 395108 [details] Also fix a typo

Committed r259330: <https://trac.webkit.org/changeset/259330> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395108 [details].