Bug 246239 - ComputedStyleExtractor::copyProperties() shouldn't copy shorthands
Summary: ComputedStyleExtractor::copyProperties() shouldn't copy shorthands
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:
 
Reported: 2022-10-07 16:14 PDT by Oriol Brufau
Modified: 2022-10-08 17:33 PDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2022-10-07 16:14:55 PDT
https://searchfox.org/wubkat/source/Source/WebCore/css/ComputedStyleExtractor.cpp#4329-4340

Ref<MutableStyleProperties> ComputedStyleExtractor::copyProperties()
{
    Vector<CSSProperty> list;
    list.reserveInitialCapacity(numCSSProperties);
    for (unsigned i = firstCSSProperty; i < lastCSSProperty; ++i) {
        auto propertyID = convertToCSSPropertyID(i);
        if (auto value = propertyValue(propertyID))
            list.append(CSSProperty(propertyID, WTFMove(value)));
    }
    list.shrinkToFit();
    return MutableStyleProperties::create(WTFMove(list));
}

The loop copies both CSS longhands and shorthands. And the shorthands are not expanded.

This is wrong, e.g. let's say that the returned list contains:
 - margin-top: 1px
 - margin-right: 2px
 - margin-bottom: 3px
 - margin-left: 4px
 - margin: 1px 2px 3px 4px

Then the caller uses setProperty(CSSPropertyMarginTop, "5px"), the list will contain:
 - margin-top: 5px
 - margin-right: 2px
 - margin-bottom: 3px
 - margin-left: 4px
 - margin: 1px 2px 3px 4px

And then getPropertyValue(CSSPropertyMargin) will be the old "1px 2px 3px 4px" instead of "5px 2px 3px 4px".

Also, asText() will produce a string containing both "margin: 1px 2px 3px 4px" twice: one time as a wrong representation of the longhands, and another due to the unexpanded shorthand.

Not sure if these problems are actually webexposed since copyProperties() is basically used internally for editing.

This seems a regression from bug 198680, since previously in only copied computable properties, which are longhands.
Comment 1 Oriol Brufau 2022-10-07 16:26:59 PDT
Pull request: https://github.com/WebKit/WebKit/pull/5157
Comment 2 EWS 2022-10-08 17:32:23 PDT
Committed 255318@main (a97990a34197): <https://commits.webkit.org/255318@main>

Reviewed commits have been landed. Closing PR #5157 and removing active labels.
Comment 3 Radar WebKit Bug Importer 2022-10-08 17:33:20 PDT
<rdar://problem/100939959>