Bug 230389 - if border-radius includes a var(), the value is not readable from .style
Summary: if border-radius includes a var(), the value is not readable from .style
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikos Mouchtaris
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-16 23:36 PDT by Tuomas Karkkainen
Modified: 2021-10-28 19:20 PDT (History)
11 users (show)

See Also:


Attachments
html file demonstrating the issue (555 bytes, text/html)
2021-09-16 23:36 PDT, Tuomas Karkkainen
no flags Details
Patch (4.80 KB, patch)
2021-10-05 19:31 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (11.92 KB, patch)
2021-10-06 14:39 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2021-10-28 15:43 PDT, Nikos Mouchtaris
mmaxfield: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
fix shorthand property set to var bug (13.65 KB, patch)
2021-10-28 16:12 PDT, Nikos Mouchtaris
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
fix shorthand property set to var bug (13.65 KB, patch)
2021-10-28 17:01 PDT, Nikos Mouchtaris
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tuomas Karkkainen 2021-09-16 23:36:34 PDT
Created attachment 438447 [details]
html file demonstrating the issue

this css:

--v0: 50px;
border-radius: var(--v0);

is rendered correctly, but in JavaScript the value of [style].style.borderRadius is "".
Comment 1 Tuomas Karkkainen 2021-09-17 05:42:05 PDT
these properties exhibit the same quirk:

border-block-width
border-inline-width
perspective-origin
Comment 2 Radar WebKit Bug Importer 2021-09-23 23:37:17 PDT
<rdar://problem/83482301>
Comment 3 Nikos Mouchtaris 2021-10-05 19:31:00 PDT
Created attachment 440324 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-10-05 19:41:28 PDT
Comment on attachment 440324 [details]
Patch

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

> Source/WebCore/css/StyleProperties.cpp:149
>          if (shorthand.length() && is<CSSPendingSubstitutionValue>(getPropertyCSSValue(shorthand.properties()[0])))
> -            return String();
> +            return downcast<CSSPendingSubstitutionValue>(*getPropertyCSSValue(shorthand.properties()[0])).shorthandValue().cssText();

Can we avoid repeating getPropertyCSSValue(shorthand.properties()[0])?
Comment 5 Nikos Mouchtaris 2021-10-06 14:39:07 PDT
Created attachment 440439 [details]
Patch
Comment 6 Myles C. Maxfield 2021-10-28 14:15:07 PDT
Comment on attachment 440439 [details]
Patch

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

> Source/WebCore/css/StyleProperties.cpp:162
> +                return downcast<CSSPendingSubstitutionValue>(*getPropertyCSSValue(shorthand.properties()[0])).shorthandValue().cssText();

I don't understand the [0] here. What if you have something like

element {
border-top-left-radius: var(--a);
border-top-right-radius: var(--b);
border-bottom-right-radius: var(--c);
border-bottom-left-radius: var(--d);
}
element.style.getPropertyValue("border-radius")

This code would appear to return var(--a), but that doesn't seem right. Am I misunderstanding the code? Or maybe the spec says that [0] is the correct behavior?
Comment 7 Nikos Mouchtaris 2021-10-28 14:29:33 PDT
> This code would appear to return var(--a), but that doesn't seem right. Am I
> misunderstanding the code? Or maybe the spec says that [0] is the correct
> behavior?

So this return is only used if all longhand are pending substitution from a variable set by the shorthand (ie. border-radius: var(--a)), so using [0] doesn't matter as all longhand values are the same.
Comment 8 Nikos Mouchtaris 2021-10-28 15:43:34 PDT
Created attachment 442756 [details]
Patch
Comment 9 Darin Adler 2021-10-28 15:48:39 PDT
Comment on attachment 442756 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Return correct string for shorthand CSS values set by var. Add extra checks for if all 
> +        longhand properties are pending values and if the requested shorthand property was set
> +        to a variable. 

Can we put this check into a separate function rather than making StyleProperties::getPropertyValue so long?
Comment 10 Nikos Mouchtaris 2021-10-28 16:12:11 PDT
Created attachment 442760 [details]
fix shorthand property set to var bug
Comment 11 Nikos Mouchtaris 2021-10-28 17:01:27 PDT
Created attachment 442767 [details]
fix shorthand property set to var bug
Comment 12 EWS 2021-10-28 19:19:58 PDT
Committed r285015 (243659@main): <https://commits.webkit.org/243659@main>

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