There should be some indication that the input field has suggestions available.
Created attachment 345203 [details] Patch
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.
(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 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.
Created attachment 345253 [details] Patch
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
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 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.
Created attachment 345689 [details] Patch
(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 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.
Created attachment 345818 [details] Patch
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
Created attachment 345878 [details] Patch
Fixed ChangeLog.
Created attachment 345883 [details] Patch
Comment on attachment 345883 [details] Patch Clearing flags on attachment: 345883 Committed r234281: <https://trac.webkit.org/changeset/234281>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42642093>
Reverted r234281 for reason: Broke internal builds. Committed r234289: <https://trac.webkit.org/changeset/234289>
Created attachment 345900 [details] Patch
Created attachment 347041 [details] Patch
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?
Created attachment 347048 [details] Patch
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
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
(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.
Created attachment 347171 [details] Patch for landing
Comment on attachment 347171 [details] Patch for landing Clearing flags on attachment: 347171 Committed r234898: <https://trac.webkit.org/changeset/234898>
*** Bug 72006 has been marked as a duplicate of this bug. ***