Bug 231160 – Add an alternate style for form controls, and implement it for checkboxes and radio buttons

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Add an alternate filled style for form controls on iOS

[Tim Horton]

Created attachment 440043 [details]
Patch

[Aditya Keerthi]

Comment on attachment 440043 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2094
> +        return nil;

`nullptr`?

> Source/WebCore/rendering/RenderThemeIOS.mm:2123
> +    context.setFillGradient(*checkboxRadioBackgroundGradient(roundedRect.rect(), states));

This will result in a null deference in the pressed state.

[Aditya Keerthi]

(In reply to Aditya Keerthi from comment #2)
> Comment on attachment 440043 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440043&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2094
> > +        return nil;
> 
> `nullptr`?
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2123
> > +    context.setFillGradient(*checkboxRadioBackgroundGradient(roundedRect.rect(), states));
> 
> This will result in a null deference in the pressed state.

s/deference/dereference/

[Tim Horton]

Created attachment 440115 [details]
Patch

[Aditya Keerthi]

Comment on attachment 440115 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2120
> +    Path path;

Move inside the `if` directly below, since it's not used otherwise.

[Tim Horton]

Created attachment 440119 [details]
Patch

[Sam Weinig]

Comment on attachment 440119 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.h:117
>  #if ENABLE(IOS_FORM_CONTROL_REFRESH)

Got to rename / remove this soon? Just as soon as we rename "modern-media-controls" right?

> Source/WebCore/rendering/RenderThemeIOS.mm:2070
> +        // FIXME: The disabled state for the alternate appearance is currently unspecified; this is just a guess.

Please make sure this has a bug.

> Source/WebCore/rendering/RenderThemeIOS.mm:2099
> +    gradient->addColorStop({ 1.0f, DisplayP3<float> { 0, 0, 0, 0 }});

Using DisplayP3 for these seem unnecessary (especially for the one that can use Color::transparentBlack), but you do you.

[Tim Horton]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 440119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440119&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.h:117
> >  #if ENABLE(IOS_FORM_CONTROL_REFRESH)
> 
> Got to rename / remove this soon? Just as soon as we rename
> "modern-media-controls" right?

Aditya and I are talking about that; we think both the setting and the ifdef can just go away now.

> > Source/WebCore/rendering/RenderThemeIOS.mm:2070
> > +        // FIXME: The disabled state for the alternate appearance is currently unspecified; this is just a guess.
> 
> Please make sure this has a bug.

Yep, will file a few.

> > Source/WebCore/rendering/RenderThemeIOS.mm:2099
> > +    gradient->addColorStop({ 1.0f, DisplayP3<float> { 0, 0, 0, 0 }});
> 
> Using DisplayP3 for these seem unnecessary (especially for the one that can
> use Color::transparentBlack), but you do you.

True, I was just typing them all straight from a color picker from a P3-space document.

[Tim Horton]

http://trac.webkit.org/changeset/283560/webkit

[Radar WebKit Bug Importer]

<rdar://problem/83895320>