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

Aditya Keerthi
Reported 2020-12-03 14:07:47 PST
...
Attachments
Patch (33.50 KB, patch)
2020-12-03 14:24 PST, Aditya Keerthi 		no flags
DetailsFormatted DiffDiff
Patch (35.27 KB, patch)
2020-12-03 14:37 PST, Aditya Keerthi 		no flags
DetailsFormatted DiffDiff
Patch (35.42 KB, patch)
2020-12-08 13:48 PST, Aditya Keerthi 		no flags
DetailsFormatted DiffDiff
Patch (35.69 KB, patch)
2020-12-10 08:49 PST, Aditya Keerthi 		no flags
DetailsFormatted DiffDiff
Patch (35.48 KB, patch)
2020-12-11 11:38 PST, Aditya Keerthi 		wenson_hsieh: review+
DetailsFormatted DiffDiff
Patch for landing (35.49 KB, patch)
2020-12-11 11:57 PST, Aditya Keerthi 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Aditya Keerthi
Comment 1 2020-12-03 14:08:04 PST
<rdar://problem/71951874>
Aditya Keerthi
Comment 2 2020-12-03 14:24:00 PST
Created attachment 415345 [details] Patch
Aditya Keerthi
Comment 3 2020-12-03 14:37:18 PST
Created attachment 415349 [details] Patch
Wenson Hsieh
Comment 4 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).
alan
Comment 5 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.
Aditya Keerthi
Comment 6 2020-12-08 13:48:07 PST
Created attachment 415672 [details] Patch
Aditya Keerthi
Comment 7 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.
Wenson Hsieh
Comment 8 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)
Aditya Keerthi
Comment 9 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.
Aditya Keerthi
Comment 10 2020-12-10 08:49:59 PST
Created attachment 415879 [details] Patch
Aditya Keerthi
Comment 11 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.
Aditya Keerthi
Comment 12 2020-12-11 11:38:07 PST
Created attachment 416020 [details] Patch
Wenson Hsieh
Comment 13 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.
Aditya Keerthi
Comment 14 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`.
Aditya Keerthi
Comment 15 2020-12-11 11:57:43 PST
Created attachment 416023 [details] Patch for landing
EWS
Comment 16 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].
Note You need to log in before you can comment on or make changes to this bug.