Bug 221572 – [iOS][FCR] Use UIColorPickerViewController for color inputs

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/72183130>

[Aditya Keerthi]

Created attachment 419620 [details]
Patch

[Tim Horton]

Comment on attachment 419620 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:73
> +- (UIColor *)colorFromHexString:(NSString *)string

Interesting. Do we not have a shared way to do this? Is this special because it only supports a specific format? How sad is Sam that the colorspace is not adjustable? :)

> LayoutTests/fast/forms/ios/choose-color-from-color-picker-expected.txt:1
> +This test verifies that tapping on a  element and selecting a color using the color picker updates element's value.

interesting/humorous that the <input type=color> disappears here. ("on a  element")

[Aditya Keerthi]

(In reply to Tim Horton from comment #3)
> Comment on attachment 419620 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419620&action=review
> 
> > Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:73
> > +- (UIColor *)colorFromHexString:(NSString *)string
> 
> Interesting. Do we not have a shared way to do this? Is this special because
> it only supports a specific format? How sad is Sam that the colorspace is
> not adjustable? :)

We have parseSimpleColorValue in ColorInputType in WebCore, but that would involve String -> Color -> CGColor -> UIColor, as well as a new WEBCORE_EXPORT.

I added this method to go straight to UIColor, which I don't think we have a shared way of doing. Do you think it's better to just use the WebCore method?

On another note, https://github.com/whatwg/html/issues/3400 is an open issue on adding other colorspaces to <input type=color>.

> > LayoutTests/fast/forms/ios/choose-color-from-color-picker-expected.txt:1
> > +This test verifies that tapping on a  element and selecting a color using the color picker updates element's value.
> 
> interesting/humorous that the <input type=color> disappears here. ("on a 
> element")

Will fix.

[Aditya Keerthi]

(In reply to Aditya Keerthi from comment #4)
> (In reply to Tim Horton from comment #3)
> > Comment on attachment 419620 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=419620&action=review
> > 
> > > LayoutTests/fast/forms/ios/choose-color-from-color-picker-expected.txt:1
> > > +This test verifies that tapping on a  element and selecting a color using the color picker updates element's value.
> > 
> > interesting/humorous that the <input type=color> disappears here. ("on a 
> > element")
> 
> Will fix.

I didn't realize this was on the expected.txt! Thought I omitted it in the test case.

I'll reword as "tapping a color input".

[Tim Horton]

Comment on attachment 419620 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:73
>>> +- (UIColor *)colorFromHexString:(NSString *)string
>> 
>> Interesting. Do we not have a shared way to do this? Is this special because it only supports a specific format? How sad is Sam that the colorspace is not adjustable? :)
> 
> We have parseSimpleColorValue in ColorInputType in WebCore, but that would involve String -> Color -> CGColor -> UIColor, as well as a new WEBCORE_EXPORT.
> 
> I added this method to go straight to UIColor, which I don't think we have a shared way of doing. Do you think it's better to just use the WebCore method?
> 
> On another note, https://github.com/whatwg/html/issues/3400 is an open issue on adding other colorspaces to <input type=color>.

I think you should see if weinig has opinions about the parsing

[Aditya Keerthi]

(In reply to Tim Horton from comment #6)
> Comment on attachment 419620 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419620&action=review
> 
> >>> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:73
> >>> +- (UIColor *)colorFromHexString:(NSString *)string
> >> 
> >> Interesting. Do we not have a shared way to do this? Is this special because it only supports a specific format? How sad is Sam that the colorspace is not adjustable? :)
> > 
> > We have parseSimpleColorValue in ColorInputType in WebCore, but that would involve String -> Color -> CGColor -> UIColor, as well as a new WEBCORE_EXPORT.
> > 
> > I added this method to go straight to UIColor, which I don't think we have a shared way of doing. Do you think it's better to just use the WebCore method?
> > 
> > On another note, https://github.com/whatwg/html/issues/3400 is an open issue on adding other colorspaces to <input type=color>.
> 
> I think you should see if weinig has opinions about the parsing

CC'd Sam.

[Sam Weinig]

Comment on attachment 419620 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:73
>>>>> +- (UIColor *)colorFromHexString:(NSString *)string
>>>> 
>>>> Interesting. Do we not have a shared way to do this? Is this special because it only supports a specific format? How sad is Sam that the colorspace is not adjustable? :)
>>> 
>>> We have parseSimpleColorValue in ColorInputType in WebCore, but that would involve String -> Color -> CGColor -> UIColor, as well as a new WEBCORE_EXPORT.
>>> 
>>> I added this method to go straight to UIColor, which I don't think we have a shared way of doing. Do you think it's better to just use the WebCore method?
>>> 
>>> On another note, https://github.com/whatwg/html/issues/3400 is an open issue on adding other colorspaces to <input type=color>.
>> 
>> I think you should see if weinig has opinions about the parsing
> 
> CC'd Sam.

String -> Color -> CGColor -> UIColor is actually not a very expensive set of conversions, given that WebCore::Color (at least in this case) is a value object, and UIColor's are generally just backed by CGColors anyway, sot act is quite cheap.

I would definitely prefer you keep this parsing in ColorInputType, where it has all the right comments explaining why it is using this parsing and is in one place. WebCore::Color is encodable and getting a UIColor from WebCore::Color should just be [[UIColor alloc] initWithCGColor:cachedCGColor(webCoreColor)]]; 

In general, if you find yourself interpreting web content in the UIProcess you have probably done something a bit wrong is a good rule of thumb.

[Aditya Keerthi]

Created attachment 419660 [details]
Patch

[Aditya Keerthi]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 419620 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419620&action=review
> 
> >>>>> Source/WebKit/UIProcess/ios/forms/WKFormColorControl.mm:73
> >>>>> +- (UIColor *)colorFromHexString:(NSString *)string
> >>>> 
> >>>> Interesting. Do we not have a shared way to do this? Is this special because it only supports a specific format? How sad is Sam that the colorspace is not adjustable? :)
> >>> 
> >>> We have parseSimpleColorValue in ColorInputType in WebCore, but that would involve String -> Color -> CGColor -> UIColor, as well as a new WEBCORE_EXPORT.
> >>> 
> >>> I added this method to go straight to UIColor, which I don't think we have a shared way of doing. Do you think it's better to just use the WebCore method?
> >>> 
> >>> On another note, https://github.com/whatwg/html/issues/3400 is an open issue on adding other colorspaces to <input type=color>.
> >> 
> >> I think you should see if weinig has opinions about the parsing
> > 
> > CC'd Sam.
> 
> String -> Color -> CGColor -> UIColor is actually not a very expensive set
> of conversions, given that WebCore::Color (at least in this case) is a value
> object, and UIColor's are generally just backed by CGColors anyway, sot act
> is quite cheap.
> 
> I would definitely prefer you keep this parsing in ColorInputType, where it
> has all the right comments explaining why it is using this parsing and is in
> one place. WebCore::Color is encodable and getting a UIColor from
> WebCore::Color should just be [[UIColor alloc]
> initWithCGColor:cachedCGColor(webCoreColor)]]; 
> 
> In general, if you find yourself interpreting web content in the UIProcess
> you have probably done something a bit wrong is a good rule of thumb.

I've updated the patch to remove the parsing from the UIProcess. Instead, I've added a new member (colorValue) to FocusedElementInformation, to pass the color from the WebProcess to the UIProcess.

[Aditya Keerthi]

Created attachment 419672 [details]
Patch

[EWS]

Committed r272597: <https://commits.webkit.org/r272597>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419672 [details].