Bug 187871 – [iOS] Add support for input[type=color]

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

This bug tracks the work required to bring input[type=color] to iOS.

[Aditya Keerthi]

Created attachment 345486 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Tim Horton]

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.

[Aditya Keerthi]

(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?

[Tim Horton]

(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.

[Aditya Keerthi]

Created attachment 345585 [details]
Patch

[Tim Horton]

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.

[Aditya Keerthi]

Created attachment 345591 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345591 [details]
Patch

Clearing flags on attachment: 345591

Committed r234105: <https://trac.webkit.org/changeset/234105>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42509854>

[Ross Kirsling]

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

[Aditya Keerthi]

Sorry about that! I'll put up a patch to remove it.