Bug 216997

Created attachment 409747 [details] Patch

<rdar://problem/69597083>

Created attachment 409748 [details] Patch

Created attachment 409749 [details] Patch

Comment on attachment 409749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409749&action=review > Source/WebCore/ChangeLog:8 > + > + * css/CSSComputedStyleDeclaration.cpp: Please provide a link to the specification. Also, why haven't you documented any of the per-file changes? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:612 > + return CSSValuePool::singleton().createIdentifierValue(CSSValueNone); Why are you returning nothing here? This seems like a pretty important thing to support, and not too difficult. > Source/WebCore/css/TransformFunctions.cpp:356 > + // FIXME: what is this used for exactly? > + UNUSED_PARAM(value); > + UNUSED_PARAM(conversionData); > + return nullptr; This converts a transform value into something that makes sense in the render tree (i.e. handles CSS units). > Source/WebCore/css/parser/CSSPropertyParser.cpp:2071 > + y = consumeLengthOrPercent(range, cssParserMode, ValueRangeAll); > + if (!y) > + return list; > + > + range.consumeWhitespace(); > + z = consumeLength(range, cssParserMode, ValueRangeAll); > + > + // If we have a calc() or non-zero y value, we can directly add it to the list. We only > + // want to add a zero y value if a non-zero z value is specified. > + if (is<CSSPrimitiveValue>(y)) { > + auto& yPrimitiveValue = downcast<CSSPrimitiveValue>(*y); > + if (yPrimitiveValue.isCalculated() || !*yPrimitiveValue.isZero()) > + list->append(*y); > + } > + > + if (is<CSSPrimitiveValue>(z)) { > + auto& zPrimitiveValue = downcast<CSSPrimitiveValue>(*z); > + if (!zPrimitiveValue.isCalculated() && *zPrimitiveValue.isZero()) > + return list; > + if (!y) > + list->append(CSSPrimitiveValue::create(0, CSSUnitType::CSS_PX)); > + list->append(*z); > + } I'm a bit confused by the logic here. Firstly, the `if (!y)` inside the `z` block will never happen, since you test `if (!y)` and return at the start. You meant to check the primitive value. But I don't think it matters if we append 0s, since we only get them if they were in the input. Shouldn't it be something like this? y = consume() if (!y) return list; if (is<CSSPV>(y)) { append(y) // doesn't really matter if it is zero } consumeWS() z = consume() if (is<CSSPrimitiveValue(z)) { append(z); // as long as it is a calc. again, 0 doesn't matter } Is there any way to get something back from consumeLength that is NOT a calc value or primitive value? If so, and you do see something you don't expect, then shouldn't the whole parse result in an empty list? i.e. Can you really append and return x if the parsing of y produces a result that is not a primitive value? Lastly, you can define the x, y, z RefPtrs when you first use them. > LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/parsing/translate-parsing-valid-expected.txt:9 > +PASS e.style['translate'] = "100%" should set the property value > +PASS e.style['translate'] = "100px 0px" should set the property value > +PASS e.style['translate'] = "100px 0.1px" should set the property value > +PASS e.style['translate'] = "100px 0%" should set the property value > +PASS e.style['translate'] = "100px calc(10px - 10%)" should set the property value > +PASS e.style['translate'] = "100px 200%" should set the property value Are there any tests for invalid parsing?

Comment on attachment 409749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409749&action=review You need to trigger stacking context too (and test that) - see hasTransformRelatedProperty(). > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:612 > + return CSSValuePool::singleton().createIdentifierValue(CSSValueNone); I think this patch needs to implement enough to pass some WPT tests and therefore implement this. > Source/WebCore/css/TransformFunctions.cpp:356 > + // FIXME: what is this used for exactly? > + UNUSED_PARAM(value); > + UNUSED_PARAM(conversionData); > + return nullptr; Not great to see this in a patch. > Source/WebCore/css/parser/CSSPropertyParser.cpp:2043 > + return list; Shouldn't this return nullptr since it's invalid? > Source/WebCore/rendering/style/RenderStyle.cpp:1403 > + if (TransformOperation* translate = m_rareNonInheritedData->translate.get()) > + translate->apply(transform, boundingBox.size()); > + Is this order right w.r.t. the transform property? I thought the spec described this and it needs to be tested.

Created attachment 410132 [details] Patch

(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 409749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409749&action=review > > You need to trigger stacking context too (and test that) - see > hasTransformRelatedProperty(). > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:612 > > + return CSSValuePool::singleton().createIdentifierValue(CSSValueNone); > > I think this patch needs to implement enough to pass some WPT tests and > therefore implement this. It is now implement and some of the WPT tests pass by virtue of correctly computing the computed style. > > Source/WebCore/css/TransformFunctions.cpp:356 > > + // FIXME: what is this used for exactly? > > + UNUSED_PARAM(value); > > + UNUSED_PARAM(conversionData); > > + return nullptr; > > Not great to see this in a patch. Yeah :) The newer patch implements this method and has new tests to back this up. > > Source/WebCore/rendering/style/RenderStyle.cpp:1403 > > + if (TransformOperation* translate = m_rareNonInheritedData->translate.get()) > > + translate->apply(transform, boundingBox.size()); > > + > > Is this order right w.r.t. the transform property? I thought the spec > described this and it needs to be tested. Yes, the spec does and I've added spec comments and tests. (In reply to Dean Jackson from comment #5) > Comment on attachment 409749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409749&action=review > > > Source/WebCore/ChangeLog:8 > > + > > + * css/CSSComputedStyleDeclaration.cpp: > > Please provide a link to the specification. Done in a newer patch. > Also, why haven't you documented any of the per-file changes? Also done. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:612 > > + return CSSValuePool::singleton().createIdentifierValue(CSSValueNone); > > Why are you returning nothing here? This seems like a pretty important thing > to support, and not too difficult. This is now implemented. > > Source/WebCore/css/TransformFunctions.cpp:356 > > + // FIXME: what is this used for exactly? > > + UNUSED_PARAM(value); > > + UNUSED_PARAM(conversionData); > > + return nullptr; > > This converts a transform value into something that makes sense in the > render tree (i.e. handles CSS units). Right, this is now implemented and tested. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2071 > > + y = consumeLengthOrPercent(range, cssParserMode, ValueRangeAll); > > + if (!y) > > + return list; > > + > > + range.consumeWhitespace(); > > + z = consumeLength(range, cssParserMode, ValueRangeAll); > > + > > + // If we have a calc() or non-zero y value, we can directly add it to the list. We only > > + // want to add a zero y value if a non-zero z value is specified. > > + if (is<CSSPrimitiveValue>(y)) { > > + auto& yPrimitiveValue = downcast<CSSPrimitiveValue>(*y); > > + if (yPrimitiveValue.isCalculated() || !*yPrimitiveValue.isZero()) > > + list->append(*y); > > + } > > + > > + if (is<CSSPrimitiveValue>(z)) { > > + auto& zPrimitiveValue = downcast<CSSPrimitiveValue>(*z); > > + if (!zPrimitiveValue.isCalculated() && *zPrimitiveValue.isZero()) > > + return list; > > + if (!y) > > + list->append(CSSPrimitiveValue::create(0, CSSUnitType::CSS_PX)); > > + list->append(*z); > > + } > > I'm a bit confused by the logic here. > > Firstly, the `if (!y)` inside the `z` block will never happen, since you > test `if (!y)` and return at the start. You meant to check the primitive > value. Correct. I've changed this to check whether there is a single value (x) in which case we must add a 0 y value. > > LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/parsing/translate-parsing-valid-expected.txt:9 > > +PASS e.style['translate'] = "100%" should set the property value > > +PASS e.style['translate'] = "100px 0px" should set the property value > > +PASS e.style['translate'] = "100px 0.1px" should set the property value > > +PASS e.style['translate'] = "100px 0%" should set the property value > > +PASS e.style['translate'] = "100px calc(10px - 10%)" should set the property value > > +PASS e.style['translate'] = "100px 200%" should set the property value > > Are there any tests for invalid parsing? Yes, see imported/w3c/web-platform-tests/css/css-transforms/parsing/translate-parsing-invalid.html. We just happened to already pass those tests since we didn't support that property.

Comment on attachment 410132 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410132&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:621 > + TransformationMatrix transform; > + style.translate()->apply(transform, pixelSnappedRect.size()); Why do you have to go via TransformationMatrix? Translate isn't affected by transform origin, so can't you just serialize the x, y and z values? > Source/WebCore/css/TransformFunctions.cpp:364 > + for (unsigned i = 0; i < valueList.length(); ++i) { Seems weird to use a loop here. Just access the elements by index. > Source/WebCore/css/parser/CSSPropertyParser.cpp:2047 > + if (!y) > + return list; What about "10px junk"? Should that be a valid 1-value transform, or an invalid value? > Source/WebCore/css/parser/CSSPropertyParser.cpp:2049 > + // If we have a calc() or non-zero y value, we can directly add it to the list. We only Is the calc() case tested? > Source/WebCore/css/parser/CSSPropertyParser.cpp:2063 > + if (!zPrimitiveValue.isCalculated() && *zPrimitiveValue.isZero()) Is the calc() case tested? > Source/WebCore/rendering/RenderObject.h:437 > + bool hasTransform() const { return hasTransformRelatedProperty() && (style().hasTransform() || style().translate()); } Change hasTransformRelatedProperty(), rather than checking style().translate().

(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 410132 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410132&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:621 > > + TransformationMatrix transform; > > + style.translate()->apply(transform, pixelSnappedRect.size()); > > Why do you have to go via TransformationMatrix? Translate isn't affected by > transform origin, so can't you just serialize the x, y and z values? I thought it was easier that way so that percentage and calc values get resolved. > > Source/WebCore/css/TransformFunctions.cpp:364 > > + for (unsigned i = 0; i < valueList.length(); ++i) { > > Seems weird to use a loop here. Just access the elements by index. This was so that the lookup and is<CSSPrimitiveValue> check weren't repeated. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2047 > > + if (!y) > > + return list; > > What about "10px junk"? Should that be a valid 1-value transform, or an > invalid value? Will check and add test coverage. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2049 > > + // If we have a calc() or non-zero y value, we can directly add it to the list. We only > > Is the calc() case tested? Will check and add test coverage if necessary. > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2063 > > + if (!zPrimitiveValue.isCalculated() && *zPrimitiveValue.isZero()) > > Is the calc() case tested? Will check and add test coverage if necessary. > > Source/WebCore/rendering/RenderObject.h:437 > > + bool hasTransform() const { return hasTransformRelatedProperty() && (style().hasTransform() || style().translate()); } > > Change hasTransformRelatedProperty(), rather than checking > style().translate(). I thought that RenderObject::hasTransformRelatedProperty() was set to match the value of RenderStyle::hasTransformRelatedProperty() which has been updated to account for CSSPropertyTranslate already.

(In reply to Antoine Quint from comment #10) > (In reply to Simon Fraser (smfr) from comment #9) > > Comment on attachment 410132 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=410132&action=review > > > > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2047 > > > + if (!y) > > > + return list; > > > > What about "10px junk"? Should that be a valid 1-value transform, or an > > invalid value? > > Will check and add test coverage. Invalid. I'll be adding coverage in translate-parsing-invalid.html and submit a WPT PR. > > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2049 > > > + // If we have a calc() or non-zero y value, we can directly add it to the list. We only > > > > Is the calc() case tested? > > Will check and add test coverage if necessary. It is tested by translate-parsing-valid.html. > > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2063 > > > + if (!zPrimitiveValue.isCalculated() && *zPrimitiveValue.isZero()) > > > > Is the calc() case tested? > > Will check and add test coverage if necessary. It is also tested by translate-parsing-valid.html.

Created attachment 410203 [details] Patch

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

I filed a WPT issue for the additional translate invalid value tests at https://github.com/web-platform-tests/wpt/pull/25919.

Created attachment 410225 [details] Patch

I've created a PR for the new SVG tests at https://github.com/web-platform-tests/wpt/pull/25928.

Created attachment 410233 [details] Patch

Comment on attachment 410233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410233&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:613 > + // Inline renderers do not support transforms. > + if (!renderer || is<RenderInline>(*renderer) || !style.translate()) I feel like we must have code somewhere else that answers the question "does this renderer support transforms", but I see you copied this from computedTransform(). You should share more code with computedTransform() so you don't copy-paste for scale and rotate also. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3544 > + case CSSPropertyTranslate: > + return computedTranslate(renderer, style); You're exposing the 'translate' property when cssIndividualTransformProperties is disabled, which is a behavior change. > Source/WebCore/css/parser/CSSPropertyParser.cpp:2047 > + RefPtr<CSSValue> y = consumeLengthOrPercent(range, cssParserMode, ValueRangeAll); > + if (!y) > + return list; How does this detect an invalid value like "0px junk"? > Source/WebCore/platform/graphics/transforms/TranslateTransformOperation.h:59 > + bool apply(TransformationMatrix& transform, const FloatSize& borderBoxSize) const override final > Source/WebCore/platform/graphics/transforms/TranslateTransformOperation.h:61 > + transform.translate3d(x(borderBoxSize), y(borderBoxSize), z(borderBoxSize)); z() doesn't care about box size. Not sure why it's written to take a rect; you could fix. > Source/WebCore/rendering/RenderObject.h:437 > + bool hasTransform() const { return hasTransformRelatedProperty() && (style().hasTransform() || style().translate()); } I see; hasTransformRelatedProperty is the fast bit check. > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:181 > + RefPtr<TranslateTransformOperation> translate; Should this be in StyleTransformData instead? > Source/WebCore/svg/SVGGraphicsElement.cpp:78 > + bool hasTransformStyle = style && style->hasTransform(); Bad name. "transform-style" is its own property.

(In reply to Simon Fraser (smfr) from comment #18) > Comment on attachment 410233 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410233&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:613 > > + // Inline renderers do not support transforms. > > + if (!renderer || is<RenderInline>(*renderer) || !style.translate()) > > I feel like we must have code somewhere else that answers the question "does > this renderer support transforms", but I see you copied this from > computedTransform(). You should share more code with computedTransform() so > you don't copy-paste for scale and rotate also. I'll add this in the commit. static bool rendererCanBeTransformed(RenderObject* renderer) { return renderer && !is<RenderInline>(*renderer); } > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3544 > > + case CSSPropertyTranslate: > > + return computedTranslate(renderer, style); > > You're exposing the 'translate' property when > cssIndividualTransformProperties is disabled, which is a behavior change. Thanks for catching this, I'll add this setting check in the commit: `if (renderer && renderer->settings().cssIndividualTransformPropertiesEnabled())` > > Source/WebCore/css/parser/CSSPropertyParser.cpp:2047 > > + RefPtr<CSSValue> y = consumeLengthOrPercent(range, cssParserMode, ValueRangeAll); > > + if (!y) > > + return list; > > How does this detect an invalid value like "0px junk"? Good question. I debugged it, since somehow it already worked, and we fail this condition in CSSPropertyParser::parseValueStart(): RefPtr<CSSValue> parsedValue = parseSingleValue(propertyID); if (parsedValue && m_range.atEnd()) { // <<< m_range.atEnd() is false because we didn't consume to the end of the specified value. addProperty(propertyID, CSSPropertyInvalid, *parsedValue, important); return true; } > > Source/WebCore/platform/graphics/transforms/TranslateTransformOperation.h:59 > > + bool apply(TransformationMatrix& transform, const FloatSize& borderBoxSize) const override > > final Will fix in commit. > > Source/WebCore/platform/graphics/transforms/TranslateTransformOperation.h:61 > > + transform.translate3d(x(borderBoxSize), y(borderBoxSize), z(borderBoxSize)); > > z() doesn't care about box size. Not sure why it's written to take a rect; > you could fix. Will fix in a followup after I land this. > > Source/WebCore/rendering/RenderObject.h:437 > > + bool hasTransform() const { return hasTransformRelatedProperty() && (style().hasTransform() || style().translate()); } > > I see; hasTransformRelatedProperty is the fast bit check. > > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:181 > > + RefPtr<TranslateTransformOperation> translate; > > Should this be in StyleTransformData instead? I won't add it there right now, I don't see any direct gain and I think conflating "transform" and individual transform properties might do more harm than good. But maybe I'll come to the opposite conclusion when supporting more properties. > > Source/WebCore/svg/SVGGraphicsElement.cpp:78 > > + bool hasTransformStyle = style && style->hasTransform(); > > Bad name. "transform-style" is its own property. Renaming to `hasSpecifiedTransform` in commit.

Committed r267887: <https://trac.webkit.org/changeset/267887>

It looks like the changes in https://trac.webkit.org/changeset/267887/webkit broke fast/css/getComputedStyle/computed-style-without-renderer.html and imported/w3c/web-platform-tests/css/css-pseudo/first-letter-allowed-properties.html Tracking in https://bugs.webkit.org/show_bug.cgi?id=217254

Reverted r267887 for reason: Broke two tests on iOS and Mac Committed r267898: <https://trac.webkit.org/changeset/267898>

Created attachment 410433 [details] Patch

Committed r267937: <https://trac.webkit.org/changeset/267937> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410433 [details].