Bug 187741 – [Datalist] Add button to TextFieldInputs with a datalist

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

There should be some indication that the input field has suggestions available.

[Aditya Keerthi]

Created attachment 345203 [details]
Patch

[Tim Horton]

Comment on attachment 345203 [details]
Patch

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

> Source/WebCore/html/TextFieldInputType.cpp:322
> +#if ENABLE(DATALIST_ELEMENT)

This seems weird at a glance but I haven't read enough to know what's up.

> Source/WebCore/html/TextFieldInputType.cpp:883
> +    return (m_suggestionPicker != nullptr);

no need for all these characters, we don't usually do explicit comparisons with 0 (`return m_suggestionPicker` should be fine)

> Source/WebCore/rendering/RenderThemeMac.mm:262
> +    NSData *data = [[NSData alloc] initWithBase64EncodedString:@"iVBORw0KGgoAAAANSUhEUgAAAA4AAAAJCAYAAAACTR1pAAAABGdBTUEAALGPC/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3CculE8AAABWWlUWHRYTUw6Y29tLmFkb2JlLnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIgeDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpyZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1ucyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAgICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYvMS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3JpZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpSREY+CjwveDp4bXBtZXRhPgpMwidZAAAAr0lEQVQoFYWOMQrCUBBEfwhomSqtkNoL6An0WLmHZzB4Au0VrBUsbW0sbFJ838Rd+H4iLkxmdnbmkxBj3IEnWIc/Q2Zl2S4geqB5geWvLreFZaDYq9hK2TzgeV6WB3TzaVUswMYd+A5mXpY2DxpG2WK4I0qwHezP5wrVhkvid+jSH/XyBHOfhE7oY7If0NOvki8cKnAG+cirPDfKBPSLt6QpXY+Gc5NgY2WVmvyu/Q2JwxWVRuNg0wAAAABJRU5ErkJggg==" options:NSDataBase64DecodingIgnoreUnknownCharacters];

It seems like there's a bunch of leaks here.

But also, I don't love the inline image data.

[Aditya Keerthi]

(In reply to Tim Horton from comment #2)
> Comment on attachment 345203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345203&action=review
> 
> > Source/WebCore/html/TextFieldInputType.cpp:883
> > +    return (m_suggestionPicker != nullptr);
> 
> no need for all these characters, we don't usually do explicit comparisons
> with 0 (`return m_suggestionPicker` should be fine)

I'll change it to `return !!m_suggestionPicker`. Otherwise the compiler complains.

> > Source/WebCore/rendering/RenderThemeMac.mm:262
> > +    NSData *data = [[NSData alloc] initWithBase64EncodedString:@"iVBORw0KGgoAAAANSUhEUgAAAA4AAAAJCAYAAAACTR1pAAAABGdBTUEAALGPC/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3CculE8AAABWWlUWHRYTUw6Y29tLmFkb2JlLnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIgeDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpyZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1ucyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAgICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYvMS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3JpZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpSREY+CjwveDp4bXBtZXRhPgpMwidZAAAAr0lEQVQoFYWOMQrCUBBEfwhomSqtkNoL6An0WLmHZzB4Au0VrBUsbW0sbFJ838Rd+H4iLkxmdnbmkxBj3IEnWIc/Q2Zl2S4geqB5geWvLreFZaDYq9hK2TzgeV6WB3TzaVUswMYd+A5mXpY2DxpG2WK4I0qwHezP5wrVhkvid+jSH/XyBHOfhE7oY7If0NOvki8cKnAG+cirPDfKBPSLt6QpXY+Gc5NgY2WVmvyu/Q2JwxWVRuNg0wAAAABJRU5ErkJggg==" options:NSDataBase64DecodingIgnoreUnknownCharacters];
> 
> It seems like there's a bunch of leaks here.
> 
> But also, I don't love the inline image data.

Is there an asset catalog I can move the data into?

[Tim Horton]

Comment on attachment 345203 [details]
Patch

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

>>> Source/WebCore/html/TextFieldInputType.cpp:883
>>> +    return (m_suggestionPicker != nullptr);
>> 
>> no need for all these characters, we don't usually do explicit comparisons with 0 (`return m_suggestionPicker` should be fine)
> 
> I'll change it to `return !!m_suggestionPicker`. Otherwise the compiler complains.

Sure!

>>> Source/WebCore/rendering/RenderThemeMac.mm:262
>>> +    NSData *data = [[NSData alloc] initWithBase64EncodedString:@"iVBORw0KGgoAAAANSUhEUgAAAA4AAAAJCAYAAAACTR1pAAAABGdBTUEAALGPC/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3CculE8AAABWWlUWHRYTUw6Y29tLmFkb2JlLnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIgeDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpyZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1ucyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAgICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYvMS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3JpZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpSREY+CjwveDp4bXBtZXRhPgpMwidZAAAAr0lEQVQoFYWOMQrCUBBEfwhomSqtkNoL6An0WLmHZzB4Au0VrBUsbW0sbFJ838Rd+H4iLkxmdnbmkxBj3IEnWIc/Q2Zl2S4geqB5geWvLreFZaDYq9hK2TzgeV6WB3TzaVUswMYd+A5mXpY2DxpG2WK4I0qwHezP5wrVhkvid+jSH/XyBHOfhE7oY7If0NOvki8cKnAG+cirPDfKBPSLt6QpXY+Gc5NgY2WVmvyu/Q2JwxWVRuNg0wAAAABJRU5ErkJggg==" options:NSDataBase64DecodingIgnoreUnknownCharacters];
>> 
>> It seems like there's a bunch of leaks here.
>> 
>> But also, I don't love the inline image data.
> 
> Is there an asset catalog I can move the data into?

We have lots of loose resources in the framework.

[Aditya Keerthi]

Created attachment 345253 [details]
Patch

[EWS Watchlist]

Comment on attachment 345253 [details]
Patch

Attachment 345253 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8578373

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 345289 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tim Horton]

Comment on attachment 345253 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:206
> +- (void)drawWithFrame:(NSRect)cellFrame inView:(__unused NSView *)controlView

A bit sad that this can’t just use a system cell.

> Source/WebCore/rendering/RenderThemeMac.mm:226
> +    if ([self userInterfaceLayoutDirection] == NSUserInterfaceLayoutDirectionRightToLeft) {

Does something really propagate UILayoutDirection into here!?

> Source/WebCore/rendering/RenderThemeMac.mm:230
> +        [transform translateXBy:-1*NSMidX(cellFrame) yBy:-1*NSMidY(cellFrame)];

Spaces around operators

> Source/WebCore/rendering/RenderThemeMac.mm:237
> +    if (accentColor == NSUserAccentColorRed)

I continue to not love this.

> Source/WebCore/rendering/RenderThemeMac.mm:261
> +    RefPtr<WebCore::Image> image = WebCore::Image::loadPlatformResource("ListButtonArrow");

Much better!

> Source/WebCore/rendering/RenderThemeMac.mm:1085
> +    NSRect listButtonFrame = NSMakeRect(r.maxX() - 16, r.y(), 16, r.height());

What’s this random 16? Would be nice to have a named constant.

[Aditya Keerthi]

Created attachment 345689 [details]
Patch

[Aditya Keerthi]

(In reply to Tim Horton from comment #8)
> Comment on attachment 345253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345253&action=review
>
> > Source/WebCore/rendering/RenderThemeMac.mm:226
> > +    if ([self userInterfaceLayoutDirection] == NSUserInterfaceLayoutDirectionRightToLeft) {
> 
> Does something really propagate UILayoutDirection into here!?

The UserInterfaceLayoutDirection is set in RenderThemeMac::paintListButtonForInput.

> > Source/WebCore/rendering/RenderThemeMac.mm:1085
> > +    NSRect listButtonFrame = NSMakeRect(r.maxX() - 16, r.y(), 16, r.height());
> 
> What’s this random 16? Would be nice to have a named constant.

It's the width of the button. I put it in a constant.

[Tim Horton]

Comment on attachment 345689 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeMac.mm:264
> +    RefPtr<WebCore::Image> image = WebCore::Image::loadPlatformResource("ListButtonArrow");

You need a 2x version of this asset, and need to figure out how to load that correctly.

> Source/WebCore/rendering/RenderThemeMac.mm:1105
> +    // Margin to place button at end of input

Comments should be complete sentences and have punctuation.

> Source/WebCore/rendering/RenderThemeMac.mm:2363
> +NSCell* RenderThemeMac::listButton() const

Star's on the wrong side.

[Aditya Keerthi]

Created attachment 345818 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345818 [details]
Patch

Rejecting attachment 345818 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 345818, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebInspectorUI/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/8665129

[Aditya Keerthi]

Created attachment 345878 [details]
Patch

[Aditya Keerthi]

Fixed ChangeLog.

[Aditya Keerthi]

Created attachment 345883 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345883 [details]
Patch

Clearing flags on attachment: 345883

Committed r234281: <https://trac.webkit.org/changeset/234281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42642093>

[Ryan Haddad]

Reverted r234281 for reason:

Broke internal builds.

Committed r234289: <https://trac.webkit.org/changeset/234289>

[Aditya Keerthi]

Created attachment 345900 [details]
Patch

[Aditya Keerthi]

Created attachment 347041 [details]
Patch

[Tim Horton]

Comment on attachment 347041 [details]
Patch

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

> Source/WebCore/html/shadow/DataListButtonElement.cpp:49
> +    : HTMLDivElement(divTag, document), m_owner(owner)

I think we usually always wrap these.

> Source/WebCore/rendering/RenderThemeMac.mm:2081
> +    } else {
> +        if (box.style().direction() == TextDirection::RTL)

Why not fold these into the outer if?

[Aditya Keerthi]

Created attachment 347048 [details]
Patch

[EWS Watchlist]

Comment on attachment 347041 [details]
Patch

Attachment 347041 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8849118

New failing tests:
fast/forms/datalist/datalist-textinput-appearance.html
fast/forms/datalist/datalist-searchinput-appearance.html

[EWS Watchlist]

Created attachment 347054 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Aditya Keerthi]

(In reply to Build Bot from comment #25)
> Comment on attachment 347041 [details]
> Patch
> 
> Attachment 347041 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/8849118
> 
> New failing tests:
> fast/forms/datalist/datalist-textinput-appearance.html
> fast/forms/datalist/datalist-searchinput-appearance.html

I'm going to have to [ Skip ] these in platform/ios/TestExpectations until ENABLE(DATALIST_ELEMENT) is true.

[Aditya Keerthi]

Created attachment 347171 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 347171 [details]
Patch for landing

Clearing flags on attachment: 347171

Committed r234898: <https://trac.webkit.org/changeset/234898>

[Ryosuke Niwa]

*** Bug 72006 has been marked as a duplicate of this bug. ***