Bug 187741 - [Datalist] Add button to TextFieldInputs with a datalist
Summary: [Datalist] Add button to TextFieldInputs with a datalist
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-17 15:42 PDT by Aditya Keerthi
Modified: 2018-08-16 11:56 PDT (History)
10 users (show)

See Also:


Attachments
Patch (53.18 KB, patch)
2018-07-17 15:55 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (56.00 KB, patch)
2018-07-18 09:22 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (13.02 MB, application/zip)
2018-07-18 14:57 PDT, Build Bot
no flags Details
Patch (56.22 KB, patch)
2018-07-24 10:50 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (58.01 KB, patch)
2018-07-25 21:39 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (58.05 KB, patch)
2018-07-26 15:12 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (58.04 KB, patch)
2018-07-26 15:28 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (58.19 KB, patch)
2018-07-26 20:44 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (57.25 KB, patch)
2018-08-13 14:40 PDT, Aditya Keerthi
thorton: review+
Details | Formatted Diff | Diff
Patch (56.90 KB, patch)
2018-08-13 15:07 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.57 MB, application/zip)
2018-08-13 16:47 PDT, Build Bot
no flags Details
Patch for landing (57.83 KB, patch)
2018-08-15 09:52 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2018-07-17 15:42:12 PDT
There should be some indication that the input field has suggestions available.
Comment 1 Aditya Keerthi 2018-07-17 15:55:42 PDT
Created attachment 345203 [details]
Patch
Comment 2 Tim Horton 2018-07-17 16:07:40 PDT
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.
Comment 3 Aditya Keerthi 2018-07-17 16:34:49 PDT
(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?
Comment 4 Tim Horton 2018-07-17 16:47:03 PDT
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.
Comment 5 Aditya Keerthi 2018-07-18 09:22:04 PDT
Created attachment 345253 [details]
Patch
Comment 6 Build Bot 2018-07-18 14:56:56 PDT
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
Comment 7 Build Bot 2018-07-18 14:57:09 PDT
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
Comment 8 Tim Horton 2018-07-23 15:04:58 PDT
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.
Comment 9 Aditya Keerthi 2018-07-24 10:50:35 PDT
Created attachment 345689 [details]
Patch
Comment 10 Aditya Keerthi 2018-07-24 10:52:57 PDT
(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.
Comment 11 Tim Horton 2018-07-25 18:04:28 PDT
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.
Comment 12 Aditya Keerthi 2018-07-25 21:39:23 PDT
Created attachment 345818 [details]
Patch
Comment 13 WebKit Commit Bot 2018-07-26 13:35:17 PDT
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
Comment 14 Aditya Keerthi 2018-07-26 15:12:52 PDT
Created attachment 345878 [details]
Patch
Comment 15 Aditya Keerthi 2018-07-26 15:13:26 PDT
Fixed ChangeLog.
Comment 16 Aditya Keerthi 2018-07-26 15:28:49 PDT
Created attachment 345883 [details]
Patch
Comment 17 WebKit Commit Bot 2018-07-26 16:12:30 PDT
Comment on attachment 345883 [details]
Patch

Clearing flags on attachment: 345883

Committed r234281: <https://trac.webkit.org/changeset/234281>
Comment 18 WebKit Commit Bot 2018-07-26 16:12:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2018-07-26 16:14:11 PDT
<rdar://problem/42642093>
Comment 20 Ryan Haddad 2018-07-26 18:38:13 PDT
Reverted r234281 for reason:

Broke internal builds.

Committed r234289: <https://trac.webkit.org/changeset/234289>
Comment 21 Aditya Keerthi 2018-07-26 20:44:45 PDT
Created attachment 345900 [details]
Patch
Comment 22 Aditya Keerthi 2018-08-13 14:40:19 PDT
Created attachment 347041 [details]
Patch
Comment 23 Tim Horton 2018-08-13 14:46:49 PDT
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?
Comment 24 Aditya Keerthi 2018-08-13 15:07:47 PDT
Created attachment 347048 [details]
Patch
Comment 25 Build Bot 2018-08-13 16:47:16 PDT
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
Comment 26 Build Bot 2018-08-13 16:47:18 PDT
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
Comment 27 Aditya Keerthi 2018-08-13 16:58:39 PDT
(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.
Comment 28 Aditya Keerthi 2018-08-15 09:52:16 PDT
Created attachment 347171 [details]
Patch for landing
Comment 29 WebKit Commit Bot 2018-08-15 13:42:34 PDT
Comment on attachment 347171 [details]
Patch for landing

Clearing flags on attachment: 347171

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