Summary: | [iOS][FCR] Use UIColorPickerViewController for color inputs | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aditya Keerthi <akeerthi> | ||||||||
Component: | Forms | Assignee: | Aditya Keerthi <akeerthi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, mifenton, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Aditya Keerthi
2021-02-08 13:20:43 PST
Created attachment 419620 [details]
Patch
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") (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. (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 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 (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 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. Created attachment 419660 [details]
Patch
(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. Created attachment 419672 [details]
Patch
Committed r272597: <https://commits.webkit.org/r272597> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419672 [details]. |