RESOLVED FIXED 238062
hasExplicitlySetBorderRadius logic is broken
https://bugs.webkit.org/show_bug.cgi?id=238062
Summary hasExplicitlySetBorderRadius logic is broken
Oriol Brufau
Reported 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.
Attachments
Patch (22.83 KB, patch)
2022-03-17 18:36 PDT, Oriol Brufau
no flags
Patch (25.31 KB, patch)
2022-03-17 19:30 PDT, Oriol Brufau
no flags
Oriol Brufau
Comment 1 2022-03-17 18:36:33 PDT
Oriol Brufau
Comment 2 2022-03-17 19:30:17 PDT
Oriol Brufau
Comment 3 2022-03-18 08:01:13 PDT
PTAL.
EWS
Comment 4 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].
Radar WebKit Bug Importer
Comment 5 2022-03-19 19:20:17 PDT
Note You need to log in before you can comment on or make changes to this bug.