Skip to content

Commit

Permalink
Sum & Product calculation node children should only get sorted for se…
Browse files Browse the repository at this point in the history 
…rialization

https://bugs.webkit.org/show_bug.cgi?id=248925

Reviewed by Antti Koivisto.

Sum & Product calculation node children should only get sorted for
serialization, as per:
- https://drafts.csswg.org/css-values-4/#serialize-a-calculation-tree
- https://drafts.csswg.org/css-values-4/#sort-a-calculations-children

However, we were doing this eagerly after parsing the calculation value, during
the simplification step:
- https://w3c.github.io/csswg-drafts/css-values/#calc-simplification

This is observable with CSS Typed OM since the following:
`CSSStyleValue.parse("width", "calc(1px + 1em)")` would get parsed as
`new CSSMathSum(CSS.em(1), CSS.px(1))` instead of
`new CSSMathSum(CSS.px(1), CSS.em(1))`.

This was causing some CSS typed OM tests to fail.

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-normalization/normalize-numeric.tentative-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-normalization/transformvalue-normalization.tentative-expected.txt:
Rebaseline some WPT tests now that more checks are passing.

* Source/WebCore/css/calc/CSSCalcOperationNode.cpp:
(WebCore::sortChildren):
Factor the logic to sort sum / product children out of combineChildren() and
into a separate function. This is to avoid code duplication now that the logic
is needed in two places.

(WebCore::CSSCalcOperationNode::combineChildren):
- Stop sorting sum and product children during simplification, as this is
  observable by CSS Typed OM and not as per spec
  (https://w3c.github.io/csswg-drafts/css-values/#calc-simplification).
- Update the logic to simplify sum and product nodes to not rely on the
  fact that the children were previously sorted (since this is not the
  case anymore).

(WebCore::CSSCalcOperationNode::buildCSSTextRecursive):
- During serialization, if the node is a sum or a product, make sure to
  sort the children before serializing them, as per the spec
  (https://drafts.csswg.org/css-values-4/#serialize-a-calculation-tree).

* Source/WebCore/css/typedom/CSSNumericValue.cpp:
(WebCore::reifyMathExpression):
- Add a FIXME for a bug that I found in our math reification logic while
  investigating this, so I remember to fix it later.

Canonical link: https://commits.webkit.org/257573@main
  • Loading branch information
@cdumez
cdumez committed Dec 8, 2022
1 parent 56d7f89 commit e06fc20
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 59 deletions.
2 changes: 1 addition & 1 deletion ...-tests/css/css-typed-om/stylevalue-normalization/normalize-numeric.tentative-expected.txt
View file
Expand Up @@ -3,6 +3,6 @@ PASS Normalizing a <number> returns a number CSSUnitValue
PASS Normalizing a <percentage> returns a percent CSSUnitValue
PASS Normalizing a <dimension> returns a CSSUnitValue with the correct unit
PASS Normalizing a <number> with a unitless zero returns 0
FAIL Normalizing a <calc> returns simplified expression assert_approx_equals: expected 4 +/- 0.000001 but got 1
PASS Normalizing a <calc> returns simplified expression
PASS Normalizing a <dimension> with a unitless zero returns 0px

2 changes: 1 addition & 1 deletion ...css-typed-om/stylevalue-normalization/transformvalue-normalization.tentative-expected.txt
View file
Expand Up @@ -26,5 +26,5 @@ PASS Normalizing a skewY() returns a CSSSkewY
PASS Normalizing a perspective() returns a CSSPerspective
PASS Normalizing a perspective(none) returns a CSSPerspective
PASS Normalizing a <transform-list> returns a CSSTransformValue containing all the transforms
FAIL Normalizing transforms with calc values contains CSSMathValues assert_equals: expected "px" but got "em"
PASS Normalizing transforms with calc values contains CSSMathValues

120 changes: 63 additions & 57 deletions Source/WebCore/css/calc/CSSCalcOperationNode.cpp
View file
Expand Up @@ -36,6 +36,7 @@
#include "CalcExpressionOperation.h"
#include "Logging.h"
#include <wtf/Algorithms.h>
#include <wtf/ListHashSet.h>
#include <wtf/text/TextStream.h>

namespace WebCore {
Expand Down Expand Up @@ -315,6 +316,25 @@ static std::optional<CalculationCategory> commonCategory(const Vector<Ref<CSSCal
return expectedCategory;
}

// https://drafts.csswg.org/css-values-4/#sort-a-calculations-children
static void sortChildren(Vector<Ref<CSSCalcExpressionNode>>& children)
{
std::stable_sort(children.begin(), children.end(), [](auto& first, auto& second) {
// Sort order: number, percentage, dimension, other.
SortingCategory firstCategory = sortingCategory(first.get());
SortingCategory secondCategory = sortingCategory(second.get());

if (firstCategory == SortingCategory::Dimension && secondCategory == SortingCategory::Dimension) {
// If nodes contains any dimensions, remove them from nodes, sort them by their units, and append them to ret.
auto firstUnitString = CSSPrimitiveValue::unitTypeString(first->primitiveType());
auto secondUnitString = CSSPrimitiveValue::unitTypeString(second->primitiveType());
return codePointCompareLessThan(firstUnitString, secondUnitString);
}

return static_cast<unsigned>(firstCategory) < static_cast<unsigned>(secondCategory);
});
}

RefPtr<CSSCalcOperationNode> CSSCalcOperationNode::create(CalcOperator op, RefPtr<CSSCalcExpressionNode>&& leftSide, RefPtr<CSSCalcExpressionNode>&& rightSide)
{
if (!leftSide || !rightSide)
Expand Down Expand Up @@ -620,6 +640,7 @@ bool CSSCalcOperationNode::canCombineAllChildren() const
return true;
}

// https://w3c.github.io/csswg-drafts/css-values/#calc-simplification
void CSSCalcOperationNode::combineChildren()
{
if (isIdentity() || !m_children.size())
Expand Down Expand Up @@ -664,48 +685,31 @@ void CSSCalcOperationNode::combineChildren()
return;
}

if (shouldSortChildren()) {
// <https://drafts.csswg.org/css-values-4/#sort-a-calculations-children>
std::stable_sort(m_children.begin(), m_children.end(), [](const auto& first, const auto& second) {
// Sort order: number, percentage, dimension, other.
SortingCategory firstCategory = sortingCategory(first.get());
SortingCategory secondCategory = sortingCategory(second.get());

if (firstCategory == SortingCategory::Dimension && secondCategory == SortingCategory::Dimension) {
// If nodes contains any dimensions, remove them from nodes, sort them by their units, and append them to ret.
auto firstUnitString = CSSPrimitiveValue::unitTypeString(first->primitiveType());
auto secondUnitString = CSSPrimitiveValue::unitTypeString(second->primitiveType());
return codePointCompareLessThan(firstUnitString, secondUnitString);
}

return static_cast<unsigned>(firstCategory) < static_cast<unsigned>(secondCategory);
});

LOG_WITH_STREAM(Calc, stream << "post-sort: " << *this);
}

if (calcOperator() == CalcOperator::Add) {
// For each set of root’s children that are numeric values with identical units,
// remove those children and replace them with a single numeric value containing
// the sum of the removed nodes, and with the same unit.
Vector<Ref<CSSCalcExpressionNode>> newChildren;
newChildren.reserveInitialCapacity(m_children.size());
newChildren.uncheckedAppend(m_children[0].copyRef());

CSSUnitType previousType = primitiveTypeForCombination(newChildren[0].get());

for (unsigned i = 1; i < m_children.size(); ++i) {
auto& currentNode = m_children[i];
CSSUnitType currentType = primitiveTypeForCombination(currentNode.get());

auto conversionType = conversionToAddValuesWithTypes(previousType, currentType);
if (conversionType != CSSCalcPrimitiveValueNode::UnitConversion::Invalid) {
downcast<CSSCalcPrimitiveValueNode>(newChildren.last().get()).add(downcast<CSSCalcPrimitiveValueNode>(currentNode.get()), conversionType);
continue;
ListHashSet<CSSCalcExpressionNode*> remainingChildren;
for (auto& child : m_children)
remainingChildren.add(child.ptr());

while (!remainingChildren.isEmpty()) {
newChildren.uncheckedAppend(Ref { *remainingChildren.takeFirst() });
CSSUnitType previousType = primitiveTypeForCombination(newChildren.last());
for (auto it = remainingChildren.begin(); it != remainingChildren.end();) {
auto currentIterator = it;
++it;
auto& currentNode = **currentIterator;
CSSUnitType currentType = primitiveTypeForCombination(currentNode);
auto conversionType = conversionToAddValuesWithTypes(previousType, currentType);
if (conversionType == CSSCalcPrimitiveValueNode::UnitConversion::Invalid)
continue;
downcast<CSSCalcPrimitiveValueNode>(newChildren.last().get()).add(downcast<CSSCalcPrimitiveValueNode>(currentNode), conversionType);
remainingChildren.remove(currentIterator);
}

previousType = primitiveTypeForCombination(currentNode);
newChildren.uncheckedAppend(currentNode.copyRef());
}

newChildren.shrinkToFit();
Expand All @@ -717,16 +721,16 @@ void CSSCalcOperationNode::combineChildren()
// If root has multiple children that are numbers (not percentages or dimensions),
// remove them and replace them with a single number containing the product of the removed nodes.
double multiplier = 1;
size_t numberNodeCount = 0;

// Sorting will have put the number nodes first.
unsigned leadingNumberNodeCount = 0;
for (auto& node : m_children) {
auto nodeType = primitiveTypeForCombination(node.get());
if (nodeType != CSSUnitType::CSS_NUMBER)
break;

multiplier *= node->doubleValue(CSSUnitType::CSS_NUMBER);
++leadingNumberNodeCount;
CSSCalcExpressionNode* lastNonNumberNode = nullptr;
for (auto& child : m_children) {
if (primitiveTypeForCombination(child) != CSSUnitType::CSS_NUMBER) {
lastNonNumberNode = child.ptr();
continue;
}
multiplier *= downcast<CSSCalcPrimitiveValueNode>(child.get()).doubleValue(CSSUnitType::CSS_NUMBER);
++numberNodeCount;
}

Vector<Ref<CSSCalcExpressionNode>> newChildren;
Expand All @@ -737,26 +741,26 @@ void CSSCalcOperationNode::combineChildren()
// return the Sum.
// The Sum's children simplification will have happened already.
bool didMultiply = false;
if (leadingNumberNodeCount && m_children.size() - leadingNumberNodeCount == 1) {
auto multiplicandCategory = calcUnitCategory(primitiveTypeForCombination(m_children.last().get()));
if (numberNodeCount && (m_children.size() - numberNodeCount) == 1) {
ASSERT(lastNonNumberNode);
auto multiplicandCategory = calcUnitCategory(primitiveTypeForCombination(*lastNonNumberNode));
if (multiplicandCategory != CalculationCategory::Other) {
newChildren.uncheckedAppend(m_children.last().copyRef());
newChildren.uncheckedAppend(Ref { *lastNonNumberNode });
downcast<CSSCalcPrimitiveValueNode>(newChildren[0].get()).multiply(multiplier);
didMultiply = true;
} else if (is<CSSCalcOperationNode>(m_children.last()) && downcast<CSSCalcOperationNode>(m_children.last().get()).calcOperator() == CalcOperator::Add) {
} else if (auto* sumNode = dynamicDowncast<CSSCalcOperationNode>(*lastNonNumberNode); sumNode && sumNode->calcOperator() == CalcOperator::Add) {
// If we're multiplying with another operation that is an addition and all the added children
// are percentages or dimensions, we should multiply each child and make this expression an
// addition.
auto allChildrenArePrimitiveValues = [](const Vector<Ref<CSSCalcExpressionNode>>& children) -> bool
{
auto allChildrenArePrimitiveValues = [](const Vector<Ref<CSSCalcExpressionNode>>& children) -> bool {
for (auto& child : children) {
if (!is<CSSCalcPrimitiveValueNode>(child))
return false;
}
return true;
};

auto& children = downcast<CSSCalcOperationNode>(m_children.last().get()).children();
auto& children = sumNode->children();
if (allChildrenArePrimitiveValues(children)) {
for (auto& child : children) {
newChildren.append(child.copyRef());
Expand All @@ -769,13 +773,15 @@ void CSSCalcOperationNode::combineChildren()
}

if (!didMultiply) {
if (leadingNumberNodeCount) {
if (numberNodeCount) {
auto multiplierNode = CSSCalcPrimitiveValueNode::create(CSSPrimitiveValue::create(multiplier, CSSUnitType::CSS_NUMBER));
newChildren.uncheckedAppend(WTFMove(multiplierNode));
}

for (unsigned i = leadingNumberNodeCount; i < m_children.size(); ++i)
newChildren.uncheckedAppend(m_children[i].copyRef());
for (auto& child : m_children) {
if (primitiveTypeForCombination(child) != CSSUnitType::CSS_NUMBER)
newChildren.uncheckedAppend(child.copyRef());
}
}

newChildren.shrinkToFit();
Expand Down Expand Up @@ -1160,8 +1166,8 @@ void CSSCalcOperationNode::buildCSSTextRecursive(const CSSCalcExpressionNode& no
if (parens == GroupingParens::Include)
builder.append('(');

// Simplification already sorted children.
auto& children = operationNode.children();
auto children = operationNode.children();
sortChildren(children);
ASSERT(children.size());
// Serialize root’s first child, and append it to s.
buildCSSTextRecursive(children.first(), builder);
Expand Down Expand Up @@ -1203,8 +1209,8 @@ void CSSCalcOperationNode::buildCSSTextRecursive(const CSSCalcExpressionNode& no
if (parens == GroupingParens::Include)
builder.append('(');

// Simplification already sorted children.
auto& children = operationNode.children();
auto children = operationNode.children();
sortChildren(children);
ASSERT(children.size());
// Serialize root’s first child, and append it to s.
buildCSSTextRecursive(children.first(), builder);
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/css/typedom/CSSNumericValue.cpp
View file
Expand Up @@ -148,6 +148,7 @@ static ExceptionOr<Ref<CSSNumericValue>> reifyMathExpression(const CSSCalcOperat
const CSSCalcExpressionNode* currentNode = &root;
do {
auto* operationNode = downcast<CSSCalcOperationNode>(currentNode);
// FIXME: This is incorrect when there are more than 2 children.
if (operationNode->children().size() == 2) {
CSS_NUMERIC_RETURN_IF_EXCEPTION(value, CSSNumericValue::reifyMathExpression(operationNode->children()[1].get()));
values.append(negateOrInvertIfRequired(operationNode->calcOperator(), WTFMove(value)));
Expand Down

0 comments on commit e06fc20

Please sign in to comment.