Summary: | [iOS][FCR] Add new look for select elements | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aditya Keerthi <akeerthi> | ||||||||||||||
Component: | Forms | Assignee: | 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
Aditya Keerthi
2020-12-03 14:07:47 PST
Created attachment 415345 [details]
Patch
Created attachment 415349 [details]
Patch
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 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. Created attachment 415672 [details]
Patch
(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 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) (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. Created attachment 415879 [details]
Patch
(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. Created attachment 416020 [details]
Patch
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. (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`. Created attachment 416023 [details]
Patch for landing
Committed r270713: <https://trac.webkit.org/changeset/270713> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416023 [details]. |