Bug 219507 – [iOS][FCR] Add new look for select elements

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/71951874>

[Aditya Keerthi]

Created attachment 415345 [details]
Patch

[Aditya Keerthi]

Created attachment 415349 [details]
Patch

[Wenson Hsieh]

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).

[zalan]

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]

Created attachment 415672 [details]
Patch

[Aditya Keerthi]

(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 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]

(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]

Created attachment 415879 [details]
Patch

[Aditya Keerthi]

(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]

Created attachment 416020 [details]
Patch

[Wenson Hsieh]

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]

(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]

Created attachment 416023 [details]
Patch for landing

[EWS]

Committed r270713: <https://trac.webkit.org/changeset/270713>

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