WebKit support for Apple Pay Buttons with custom corner radii

• Component: New Bugs
• Status: RESOLVED FIXED

[Nikos Mouchtaris]

WebKit support for Apple Pay Buttons with custom corner radii

[Nikos Mouchtaris]

Created attachment 390563 [details]
Patch

[Andy Estes]

Comment on attachment 390563 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeCocoa.mm:112
> +        PKDrawApplePayButtonWithCornerRadius(paintInfo.context().platformContext(), CGRectMake(paintRect.x(), -paintRect.maxY(), paintRect.width(), paintRect.height()), 1.0, std::max({topLeft.height.value(), topLeft.width.value(), topRight.height.value(), topRight.width.value(), bottomLeft.height.value(), bottomLeft.width.value(), bottomRight.height.value(), bottomRight.width.value()}), toPKPaymentButtonType(renderer.style().applePayButtonType()), toPKPaymentButtonStyle(renderer.style().applePayButtonStyle()), renderer.style().locale());

Pretty hard to read all this as one statement. Can you compute the corner radius separately? Maybe a static helper function that takes a `const RenderStyle&` and returns the (maybe default) corner radius?

> Source/WebCore/rendering/RenderThemeCocoa.mm:116
>      PKDrawApplePayButton(paintInfo.context().platformContext(), CGRectMake(paintRect.x(), -paintRect.maxY(), paintRect.width(), paintRect.height()), 1.0, toPKPaymentButtonType(renderer.style().applePayButtonType()), toPKPaymentButtonStyle(renderer.style().applePayButtonStyle()), renderer.style().locale());

I would call PKDrawApplePayButtonWithCornerRadius whether or not there is a `border-radius`. If there's no border radius, you can use `PKDrawApplePayButtonWithCornerRadius` as a default (see SOFT_LINK_CONSTANT_FOR_{HEADER,SOURCE} for how to load it).

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:39
> +        .borderRadiusLarge{

Missing a space before the {.

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:43
> +            border-top-left-radius: 20px;
> +            border-top-right-radius: 25px;
> +            border-bottom-left-radius: 10px;
> +            border-bottom-right-radius: 5px;

How about cases where the width and height differ?

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:50
> +        for (let style of ["white","white-outline", "black"]) {

Missing a space before "white-outline".

[Nikos Mouchtaris]

Created attachment 390712 [details]
Patch

[Andy Estes]

Comment on attachment 390712 [details]
Patch

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

> Source/WebCore/PAL/pal/cocoa/PassKitSoftLink.h:50
>  SOFT_LINK_FUNCTION_FOR_HEADER(PAL, PassKit, PKDrawApplePayButton, void, (CGContextRef context, CGRect drawRect, CGFloat scale, PKPaymentButtonType type, PKPaymentButtonStyle style, NSString *languageCode), (context, drawRect, scale, type, style, languageCode))

We can remove this now.

> Source/WebCore/PAL/pal/cocoa/PassKitSoftLink.mm:54
>  SOFT_LINK_FUNCTION_FOR_SOURCE(PAL, PassKit, PKDrawApplePayButton, void, (CGContextRef context, CGRect drawRect, CGFloat scale, PKPaymentButtonType type, PKPaymentButtonStyle style, NSString *languageCode), (context, drawRect, scale, type, style, languageCode))

Ditto.

> Source/WebCore/PAL/pal/spi/cocoa/PassKitSPI.h:366
>  extern "C"
>  void PKDrawApplePayButton(_Nonnull CGContextRef, CGRect drawRect, CGFloat scale, PKPaymentButtonType, PKPaymentButtonStyle, NSString * _Nullable languageCode);

Ditto.

> Source/WebCore/rendering/RenderThemeCocoa.mm:103
> +static float getMaxBorderRadius(const RenderStyle& renderStyle)
> +{
> +    return renderStyle.hasBorderRadius() ? std::max({renderStyle.borderTopLeftRadius().height.value(), renderStyle.borderTopLeftRadius().width.value(), renderStyle.borderTopRightRadius().height.value(), renderStyle.borderTopRightRadius().width.value(), renderStyle.borderBottomLeftRadius().height.value(), renderStyle.borderBottomLeftRadius().width.value(), renderStyle.borderBottomRightRadius().height.value(), renderStyle.borderBottomRightRadius().width.value()}) : PAL::get_PassKit_PKApplePayButtonDefaultCornerRadius();
> +}

We should return a CGFloat, and we typically don't prefix functions like this with "get".

A suggestion for even easier reading:

static CGFloat largestCornerRadius(const RenderStyle& style)
{
    if (!renderStyle.hasBorderRadius())
        return PAL::get_PassKit_PKApplePayButtonDefaultCornerRadius();

    return std::max<CGFloat>({
        style.borderTopLeftRadius().height.value(),
        style.borderTopLeftRadius().width.value(),
        style.borderTopRightRadius().height.value(),
        style.borderTopRightRadius().width.value(),
        style.borderBottomLeftRadius().height.value(),
        style.borderBottomLeftRadius().width.value(),
        style.borderBottomRightRadius().height.value(),
        style.borderBottomRightRadius().width.value()
    });
}

> LayoutTests/http/tests/ssl/applepay/ApplePayButtonV4.html:46
> +        for (let style of ["white-outline", "black"]) {

Did you mean to remove the "white" style?

Maybe we should render all the buttons over a neither-white-nor-black background so that it's easy to see the corner radius in all cases.

[Nikos Mouchtaris]

Created attachment 390779 [details]
Patch

[Andy Estes]

Comment on attachment 390779 [details]
Patch

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

One last comment, but otherwise the patch looks great. You can upload a new patch with my named filled in to the "Reviewed by" lines in the ChangeLog. You don't need to set the review bit, but if you set the cq? bit I'll cq+ it. Thanks Nikos!

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:55
> +                if((type == "plain" || type == "set-up")){

Why only these two types?

[Nikos Mouchtaris]

Created attachment 390799 [details]
Patch

[Andy Estes]

Comment on attachment 390799 [details]
Patch

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

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:10
> +            float: left;

Ah, I get why you excluded some types now. Good fix.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390799 [details]:

imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html bug 207790 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390799 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
http/tests/security/contentSecurityPolicy/report-status-code-zero-when-using-https.html bug 197297 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390799 [details]
Patch

Clearing flags on attachment: 390799

Committed r256648: <https://trac.webkit.org/changeset/256648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59474051>

[Simon Fraser (smfr)]

Comment on attachment 390799 [details]
Patch

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

> LayoutTests/ChangeLog:18
> +        * platform/mac-highsierra/http/tests/ssl/applepay/ApplePayButton-expected.png:
> +        * platform/mac-highsierra/http/tests/ssl/applepay/ApplePayButton-expected.txt:
> +        * platform/mac/http/tests/ssl/applepay/ApplePayButton-expected.png:
> +        * platform/mac/http/tests/ssl/applepay/ApplePayButton-expected.txt:
> +        * platform/mac/http/tests/ssl/applepay/ApplePayButtonV4-expected.png:
> +        * platform/mac/http/tests/ssl/applepay/ApplePayButtonV4-expected.txt:

We don't run pixel tests, so these tests are useless. You need to concoct some reference tests with transforms and marks to detect the kind of corner rendering.