Bug 238062 - hasExplicitlySetBorderRadius logic is broken
Summary: hasExplicitlySetBorderRadius logic is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on:
Blocks: 212476 236199
  Show dependency treegraph
 
Reported: 2022-03-17 18:08 PDT by Oriol Brufau
Modified: 2022-03-19 19:20 PDT (History)
15 users (show)

See Also:


Attachments
Patch (22.83 KB, patch)
2022-03-17 18:36 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (25.31 KB, patch)
2022-03-17 19:30 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2022-03-17 18:08:24 PDT
Bug 212476 added a hasExplicitlySetBorderRadius flag that tracks whether the radius is a non initial value.
It doesn't work well, because border-radius is a shorthand with multiple longhands.

    <style>
        .apple-pay-button {
            -webkit-appearance: -apple-pay-button;
        }
        .square-corners {
            border-radius: 0px;
        }
    </style>
    <div class=container>
        <button class='apple-pay-button square-corners' style='border-bottom-left-radius: inherit'></button>
        <button class='apple-pay-button square-corners' style='border-bottom-right-radius: inherit'></button>
        <button class='apple-pay-button square-corners' style='border-top-left-radius: inherit'></button>
        <button class='apple-pay-button square-corners' style='border-top-right-radius: inherit'></button>
    </div>

Result: all buttons have square corners, except the last one.
Why? Because all the border-*-radius share the same flag, and the properties are applied in alphabetical order.

So e.g. for the first button:
 1. Applying border-bottom-left-radius:inherit sets the flag to the parent value, which is false.
 2. Applying border-bottom-right-radius:0px sets the flag to true.
 3. Applying border-top-left-radius:0px sets the flag to true.
 4. Applying border-top-right-radius:0px sets the flag to true.
The flag ends up being true.

So e.g. for the first button:
 1. Applying border-bottom-left-radius:0px sets the flag to true.
 2. Applying border-bottom-right-radius:0px sets the flag to true.
 3. Applying border-top-left-radius:0px sets the flag to true.
 4. Applying border-top-right-radius:inherit sets the flag to the parent value, which is false.
The flag ends up being false!

Also, only BuilderCustom::applyInheritBorder*Radius and BuilderCustom::applyValueBorder*Radius set the flag.
But BuilderFunctions::applyInitialBorder*Radius doesn't set it.
This makes the test appearance-apple-pay-button-border-radius.html fail with my patch for bug 236199.

IMO this flag doesn't make much sense, as mentioned in bug 212476 it should probably be about cascade origins.
Or rather than being a separate flag, it would probably be better to store it as an auto radius Length?
But given the current state, and since I don't want to refactor the entire thing, as a band-aid I think it makes more sense to:
 - Have one flag for each longhand.
 - Let 'inherit' set it to true.
Comment 1 Oriol Brufau 2022-03-17 18:36:33 PDT
Created attachment 455055 [details]
Patch
Comment 2 Oriol Brufau 2022-03-17 19:30:17 PDT
Created attachment 455059 [details]
Patch
Comment 3 Oriol Brufau 2022-03-18 08:01:13 PDT
PTAL.
Comment 4 EWS 2022-03-19 19:19:13 PDT
Committed r291536 (248642@main): <https://commits.webkit.org/248642@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455059 [details].
Comment 5 Radar WebKit Bug Importer 2022-03-19 19:20:17 PDT
<rdar://problem/90531307>