Bug 221572 - [iOS][FCR] Use UIColorPickerViewController for color inputs
Summary: [iOS][FCR] Use UIColorPickerViewController for color inputs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-08 13:20 PST by Aditya Keerthi
Modified: 2021-02-09 11:25 PST (History)
10 users (show)

See Also:


Attachments
Patch (45.57 KB, patch)
2021-02-08 13:27 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (49.82 KB, patch)
2021-02-08 18:03 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff
Patch (50.96 KB, patch)
2021-02-08 20:40 PST, 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 2021-02-08 13:20:43 PST
...
Comment 1 Aditya Keerthi 2021-02-08 13:20:56 PST
<rdar://problem/72183130>
Comment 2 Aditya Keerthi 2021-02-08 13:27:31 PST
Created attachment 419620 [details]
Patch
Comment 3 Tim Horton 2021-02-08 13:50:33 PST
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")
Comment 4 Aditya Keerthi 2021-02-08 13:59:16 PST
(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.
Comment 5 Aditya Keerthi 2021-02-08 14:04:24 PST
(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".
Comment 6 Tim Horton 2021-02-08 14:04:37 PST
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
Comment 7 Aditya Keerthi 2021-02-08 14:07:39 PST
(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.
Comment 8 Sam Weinig 2021-02-08 14:36:27 PST
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.
Comment 9 Aditya Keerthi 2021-02-08 18:03:25 PST
Created attachment 419660 [details]
Patch
Comment 10 Aditya Keerthi 2021-02-08 18:06:06 PST
(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.
Comment 11 Aditya Keerthi 2021-02-08 20:40:31 PST
Created attachment 419672 [details]
Patch
Comment 12 EWS 2021-02-09 11:25:09 PST
Committed r272597: <https://commits.webkit.org/r272597>

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