Bug 223208 – [iOS][FCR] Add pressed state for button-like controls

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/72399087>

[Aditya Keerthi]

Created attachment 423220 [details]
Patch

[EWS]

Committed r274473: <https://commits.webkit.org/r274473>

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

[Simon Fraser (smfr)]

Comment on attachment 423220 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:350
> +        style.setOpacity(0.75f);

You can't just set opacity in the style without calling Adjuster::adjust() - opacity has side effects, like creating CSS stacking context.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 223287

[Aditya Keerthi]

Created attachment 423746 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 423746 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2124
> +        if (pressed)
> +            context.fillRoundedRect(checkboxRect, systemColor(CSSValueAppleSystemBlue, styleColorOptions).colorWithAlphaMultipliedBy(pressedStateOpacity));
> +        else if (enabled)
>              context.fillRoundedRect(checkboxRect, systemColor(CSSValueAppleSystemBlue, styleColorOptions));
>          else
>              context.fillRoundedRect(checkboxRect, systemColor(CSSValueAppleSystemSecondaryFillDisabled, styleColorOptions));

With code like this, it's much better to factor it so that you only call the actual function once. Prepare the inputs based on state, but just have one call to context.fillRoundedRect()

[Aditya Keerthi]

Created attachment 423759 [details]
Patch

[Aditya Keerthi]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 423746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423746&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2124
> > +        if (pressed)
> > +            context.fillRoundedRect(checkboxRect, systemColor(CSSValueAppleSystemBlue, styleColorOptions).colorWithAlphaMultipliedBy(pressedStateOpacity));
> > +        else if (enabled)
> >              context.fillRoundedRect(checkboxRect, systemColor(CSSValueAppleSystemBlue, styleColorOptions));
> >          else
> >              context.fillRoundedRect(checkboxRect, systemColor(CSSValueAppleSystemSecondaryFillDisabled, styleColorOptions));
> 
> With code like this, it's much better to factor it so that you only call the
> actual function once. Prepare the inputs based on state, but just have one
> call to context.fillRoundedRect()

Done. I moved the color determination into separate methods, so that they can be used for both checkboxes and radio buttons.

[Simon Fraser (smfr)]

Comment on attachment 423759 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2100
> +    if (!(states & ControlStates::EnabledState))

ControlStates::States should be an enum class, and be used OptionSet<>

[Aditya Keerthi]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 423759 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423759&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2100
> > +    if (!(states & ControlStates::EnabledState))
> 
> ControlStates::States should be an enum class, and be used OptionSet<>

Tracking this refactoring in https://bugs.webkit.org/show_bug.cgi?id=223647.

[EWS]

Committed r274886: <https://commits.webkit.org/r274886>

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