Bug 219507

Summary: [iOS][FCR] Add new look for select elements
Product: WebKit Reporter: Aditya Keerthi <akeerthi>
Component: FormsAssignee: Aditya Keerthi <akeerthi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, kondapallykalyan, macpherson, menard, mifenton, pdr, thorton, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
wenson_hsieh: review+
Patch for landing none

Description Aditya Keerthi 2020-12-03 14:07:47 PST 
...
Comment 1 Aditya Keerthi 2020-12-03 14:08:04 PST 
<rdar://problem/71951874>
Comment 2 Aditya Keerthi 2020-12-03 14:24:00 PST 
Created attachment 415345 [details]
Patch
Comment 3 Aditya Keerthi 2020-12-03 14:37:18 PST 
Created attachment 415349 [details]
Patch
Comment 4 Wenson Hsieh 2020-12-08 12:56:30 PST 
Comment on attachment 415349 [details]
Patch

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

> Source/WebCore/rendering/RenderMenuList.cpp:672
> +    RenderThemeIOS::adjustRoundBorderRadius(mutableStyle(), *this);

It seems a bit strange to be adjusting style during layout, but I do see that this is already done in a similar way in RenderButton and RenderTextControlSingleLine. Perhaps other folks might have some more input on whether this is fine (and what alternatives we should consider otherwise).
Comment 5 zalan 2020-12-08 13:17:44 PST 
Comment on attachment 415349 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:553
> +LengthBox RenderThemeIOS::popupInternalPaddingBox(const RenderStyle& style, const Element& element) const

Please do not pass in the Element just to consult with the iOSFormControlRefreshEnabled bit.
Comment 6 Aditya Keerthi 2020-12-08 13:48:07 PST 
Created attachment 415672 [details]
Patch
Comment 7 Aditya Keerthi 2020-12-08 13:51:05 PST 
(In reply to zalan from comment #5)
> Comment on attachment 415349 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415349&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:553
> > +LengthBox RenderThemeIOS::popupInternalPaddingBox(const RenderStyle& style, const Element& element) const
> 
> Please do not pass in the Element just to consult with the
> iOSFormControlRefreshEnabled bit.

Changed to pass in the Settings.
Comment 8 Wenson Hsieh 2020-12-09 16:53:18 PST 
Comment on attachment 415672 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2219
> +void RenderThemeIOS::paintMenuListButtonDecorationsFCR(const RenderBox& box, const PaintInfo& paintInfo, const FloatRect& rect)

(Same comment here about method naming and acronyms/abbreviations)
Comment 9 Aditya Keerthi 2020-12-09 19:25:28 PST 
(In reply to Wenson Hsieh from comment #8)
> Comment on attachment 415672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415672&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2219
> > +void RenderThemeIOS::paintMenuListButtonDecorationsFCR(const RenderBox& box, const PaintInfo& paintInfo, const FloatRect& rect)
> 
> (Same comment here about method naming and acronyms/abbreviations)

Will update after landing https://bugs.webkit.org/show_bug.cgi?id=219698, since this patch will need to be rebased.
Comment 10 Aditya Keerthi 2020-12-10 08:49:59 PST 
Created attachment 415879 [details]
Patch
Comment 11 Aditya Keerthi 2020-12-10 08:51:09 PST 
(In reply to Wenson Hsieh from comment #8)
> Comment on attachment 415672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415672&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2219
> > +void RenderThemeIOS::paintMenuListButtonDecorationsFCR(const RenderBox& box, const PaintInfo& paintInfo, const FloatRect& rect)
> 
> (Same comment here about method naming and acronyms/abbreviations)

Renamed method to paintMenuListButtonDecorationsWithFormControlRefresh.
Comment 12 Aditya Keerthi 2020-12-11 11:38:07 PST 
Created attachment 416020 [details]
Patch
Comment 13 Wenson Hsieh 2020-12-11 11:50:33 PST 
Comment on attachment 416020 [details]
Patch

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

r=mews

> Source/WebCore/rendering/RenderThemeIOS.mm:2388
> +        constexpr int size = 18;

Perhaps `length` or `sideLength` instead of `size`? Size more commonly refers to a { width, height }, especially in this area of code.
Comment 14 Aditya Keerthi 2020-12-11 11:56:35 PST 
(In reply to Wenson Hsieh from comment #13)
> Comment on attachment 416020 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416020&action=review
> 
> r=mews
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2388
> > +        constexpr int size = 18;
> 
> Perhaps `length` or `sideLength` instead of `size`? Size more commonly
> refers to a { width, height }, especially in this area of code.

Thanks for the review!

Will update to `length`.
Comment 15 Aditya Keerthi 2020-12-11 11:57:43 PST 
Created attachment 416023 [details]
Patch for landing
Comment 16 EWS 2020-12-11 15:28:01 PST 
Committed r270713: <https://trac.webkit.org/changeset/270713>

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