Bug 187871 - [iOS] Add support for input[type=color]
Summary: [iOS] Add support for input[type=color]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-20 14:27 PDT by Aditya Keerthi
Modified: 2018-07-23 16:58 PDT (History)
8 users (show)

See Also:


Attachments
Patch (40.44 KB, patch)
2018-07-20 16:02 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.94 MB, application/zip)
2018-07-20 18:41 PDT, Build Bot
no flags Details
Patch (39.87 KB, patch)
2018-07-23 11:12 PDT, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (39.84 KB, patch)
2018-07-23 11:45 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-20 14:27:58 PDT
This bug tracks the work required to bring input[type=color] to iOS.
Comment 1 Aditya Keerthi 2018-07-20 16:02:03 PDT
Created attachment 345486 [details]
Patch
Comment 2 Build Bot 2018-07-20 18:41:21 PDT
Comment on attachment 345486 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 3 Build Bot 2018-07-20 18:41:32 PDT
Created attachment 345495 [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 4 Tim Horton 2018-07-23 10:27:52 PDT
Comment on attachment 345486 [details]
Patch

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

> Source/WebCore/html/ColorInputType.cpp:173
> +#if PLATFORM(MAC)

It feels like this ifdef is more appropriate in the platform-y Chrome code instead of here.

> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> +- (instancetype)initWithView:(WKContentView *)view;

Can this just be a UIView? It’s kind of weird to spread knowledge of WKContentView, and I don’t think it’s important in this case.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:109
> +        [colorButtons addObject:[colorButtonsRow copy]];

This looks like a leak. But it’s not, because you release them in dealloc. But the array was retaining them, so the extra ref was for naught. If you did things the WebKit way this would be

auto row = adoptNS([colorButtonsRow copy]);
[colorButtons addObject:row.get()];

and then nothing in dealloc.

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:145
> +    [_colorButtons release];
> +    [_colorMatrix release];

Why not RetainPtr instead of this?

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:219
> +    const CGFloat *components = CGColorGetComponents(uiColor.CGColor);

Why are you doing all this yourself? I’m sure we have UIColor->Color code elsewhere.
Comment 5 Aditya Keerthi 2018-07-23 10:46:41 PDT
(In reply to Tim Horton from comment #4)
> Comment on attachment 345486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345486&action=review
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> > +- (instancetype)initWithView:(WKContentView *)view;
> 
> Can this just be a UIView? It’s kind of weird to spread knowledge of
> WKContentView, and I don’t think it’s important in this case.

I made it a WKContentView because the view is passed through to another class which has WKContentView specific logic. If I changed it to a UIView here, I would need to cast to WKContentView later on. Do you prefer casting or keeping the definition as-is?
Comment 6 Tim Horton 2018-07-23 11:07:19 PDT
(In reply to Aditya Keerthi from comment #5)
> (In reply to Tim Horton from comment #4)
> > Comment on attachment 345486 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=345486&action=review
> > 
> > > Source/WebKit/UIProcess/ios/forms/WKFormColorControl.h:33
> > > +- (instancetype)initWithView:(WKContentView *)view;
> > 
> > Can this just be a UIView? It’s kind of weird to spread knowledge of
> > WKContentView, and I don’t think it’s important in this case.
> 
> I made it a WKContentView because the view is passed through to another
> class which has WKContentView specific logic. If I changed it to a UIView
> here, I would need to cast to WKContentView later on. Do you prefer casting
> or keeping the definition as-is?

No, no, leave it as is. I didn't see the WKContentView specific bit.
Comment 7 Aditya Keerthi 2018-07-23 11:12:45 PDT
Created attachment 345585 [details]
Patch
Comment 8 Tim Horton 2018-07-23 11:27:02 PDT
Comment on attachment 345585 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorPicker.mm:114
> +    _colorButtons = adoptNS([colorButtons copy]);

In retrospect, there's not even a real reason for this copy.
Comment 9 Aditya Keerthi 2018-07-23 11:45:03 PDT
Created attachment 345591 [details]
Patch
Comment 10 WebKit Commit Bot 2018-07-23 12:13:15 PDT
Comment on attachment 345591 [details]
Patch

Clearing flags on attachment: 345591

Committed r234105: <https://trac.webkit.org/changeset/234105>
Comment 11 WebKit Commit Bot 2018-07-23 12:13:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-07-23 12:15:11 PDT
<rdar://problem/42509854>
Comment 13 Ross Kirsling 2018-07-23 16:52:04 PDT
Comment on attachment 345591 [details]
Patch

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

> Source/WebCore/css/html.css:887
>  #endif // defined(ENABLE_DATALIST_ELEMENT) && ENABLE_DATALIST_ELEMENT

This #endif got left behind. :) It seems to be what's rendering text diffs invalid on WinCairo right now:
https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/r234105%20(694)/results.html
Comment 14 Aditya Keerthi 2018-07-23 16:58:11 PDT
Sorry about that! I'll put up a patch to remove it.