-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LBSE] Implement support for patterns #22427
[LBSE] Implement support for patterns #22427
Conversation
EWS run on previous version of this PR (hash ada1fd7) |
This mostly follows the legacy pattern code.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Needs to move away from some pre-LBSE idioms that involve SVGRenderingContext.
if (!buildTileImageTransform(renderer, *m_attributes, patternElement(), tileBoundaries, tileImageTransform)) | ||
return nullptr; | ||
|
||
auto absoluteTransform = SVGRenderingContext::calculateTransformationToOutermostCoordinateSystem(renderer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware, this is legacy code. In LBSE this won't give correct answers. You probably need to use SVGLayerTransformComputation directly:
auto transform = SVGLayerTransformComputation(child).computeAccumulatedTransform(&renderer, TransformState::TrackSVGCTMMatrix);
since you do need access to the CTM, to extract the xScale/yScale.
The float calculateScreenFontSizeScalingFactor() const
helper function in SVGLayerTransformComputation has the same purpose (it just returns sqrt((xScale^2+yScale^2)/2
). You could add a helper to return a FloatSize with xScale, yScale, or a std::tuple<float, float>, so one could use structured-binding and use something like "auto [xScale, yScale] = SVGLayerTransformComputation(..).calculateXYCTMScaleForRenderer(..)".
Whatever you prefer :-)
return true; | ||
} | ||
|
||
static inline FloatRect calculatePatternBoundaries(const PatternAttributes& attributes, const FloatRect& objectBoundingBox, const SVGPatternElement& patternElement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be moved into RenderSVGResourcePatternInlines.h
|
||
auto tileSize = roundedUnscaledImageBufferSize(size, scale); | ||
|
||
// FIXME: Use createImageBuffer(rect, scale), delete the above calculations and fix 'tileImageTransform' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that yours?
for (auto& child : childrenOfType<SVGElement>(*attributes.patternContentElement())) { | ||
if (!child.renderer()) | ||
continue; | ||
SVGRenderingContext::renderSubtreeToContext(tileImageContext, *child.renderer(), contentTransformation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is all legacy stuff, SVGRenderingContext does not work for LBSE. Transformations would be broken, rendering at wrong spots -- you need to paint/hit-test via layers, not using the render tree.
Excerpt from downstream RenderSVGResourcePattern:
auto* patternRenderer = static_cast<RenderSVGResourcePattern*>(attributes.patternContentElement()->renderer());
ASSERT(patternRenderer);
ASSERT(patternRenderer->hasLayer());
// Draw the content into the ImageBuffer.
patternRenderer->layer()->paintSVGResourceLayer(tileImageContext, LayoutRect::infiniteRect(), tileImageTransform);
usedContext->setCompositeOperation(CompositeOperator::DestinationIn); | ||
usedContext->beginTransparencyLayer(1); | ||
} else if (usedContext->strokePattern()) { | ||
usedContext->setFillPattern(*usedContext->strokePattern()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also want to extend GraphicsContext :-) a fillRect(const FloatRect&, Pattern&)
variant, would allow you to avoid the state mutation here. (Note: you also need to assure it's reset correctly, the current restore() time is too lat probably)
ada1fd7
to
c09ba71
Compare
EWS run on previous version of this PR (hash c09ba71) |
c09ba71
to
6e3983a
Compare
EWS run on previous version of this PR (hash 6e3983a) |
6e3983a
to
71ef40c
Compare
EWS run on current version of this PR (hash 71ef40c) |
EWS run on previous version of this PR (hash 71ef40c) |
71ef40c
to
cddb8c0
Compare
EWS run on current version of this PR (hash cddb8c0) |
EWS run on previous version of this PR (hash cddb8c0) |
cddb8c0
to
72bb42b
Compare
EWS run on previous version of this PR (hash 72bb42b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great patch, almost r+. New repaint tests would be great too
@@ -2839,6 +2839,8 @@ void RenderLayer::paintSVGResourceLayer(GraphicsContext& context, GraphicsContex | |||
auto localPaintDirtyRect = LayoutRect::infiniteRect(); | |||
|
|||
auto* rootPaintingLayer = enclosingSVGRootLayer(); | |||
if (is<RenderSVGHiddenContainer>(parent()->renderer())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is tricky. It fixes custom/pattern-userSpaceOnUse-userToBaseTransform.xhtml.
The outer svg's have a 1px border which is mistakenly taken into account (before that change) and offsets the pattern contents by 1x1, showing the red rectangle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it deserves a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think I'll leave out this part, it breaks recursive-mask.svg and maybe some others. We'll have to visit this 1px border problem later...
|
||
bool RenderSVGResourcePattern::buildPatternIfNeeded(GraphicsContext& context, const RenderLayerModelObject& targetRenderer) | ||
{ | ||
collectPatternAttributesIfNeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for gradients this returns a bool, false indicating early exit due to empty bbox- why not mirror it 1:1 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made collectPatternAttributesIfNeeded do more checks.
if (gradient) | ||
usedContext->fillRect(textRootBlock->repaintRectInLocalCoordinates(), *gradient, gradientSpaceTransform); | ||
else { | ||
GraphicsContextStateSaver stateSaver(*usedContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would then check if a fillRect(FloatRect,Pattern&) exists to avoid that state swap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to take this in a separated PR?
9d98f15
to
36271bc
Compare
EWS run on previous version of this PR (hash 36271bc) |
36271bc
to
7e5a8d8
Compare
EWS run on previous version of this PR (hash 7e5a8d8) |
7e5a8d8
to
9c241b9
Compare
EWS run on previous version of this PR (hash 9c241b9) |
9c241b9
to
da9ee70
Compare
EWS run on previous version of this PR (hash da9ee70) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, r=me with a few comments left:
|
||
auto patternTransform = m_attributes->patternTransform(); | ||
if (!patternTransform.isIdentity()) | ||
patternSpaceTransform = patternTransform * patternSpaceTransform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually call multiply instead of invoking operators, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have not used these APIs in a long time :-) Fixed.
context.setAlpha(svgStyle.strokeOpacity()); | ||
SVGRenderSupport::applyStrokeStyleToContext(context, style, targetRenderer); | ||
if (svgStyle.vectorEffect() == VectorEffect::NonScalingStroke) { | ||
if (is<RenderSVGShape>(targetRenderer)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two if conditions should be unified into one to make this slightly better readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that does not really work:
if (auto* shape = dynamicDowncast(targetRenderer) ; svgStyle.vectorEffect() == VectorEffect::NonScalingStroke) {
Then we may set shape without using it.
da9ee70
to
f20df52
Compare
EWS run on current version of this PR (hash f20df52) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
https://bugs.webkit.org/show_bug.cgi?id=267041 Reviewed by Nikolas Zimmermann. Implement pattern support using a different approach to legacy including the use of RenderLayer::paintSVGLayer ro paint the pattern contents. Right now there is no caching of pattern tiles (unlike in legacy) at all to keep things simple. Note that there are still various pattern test failures due to problems with hidpi. Adding convenient pattern API for stroke painting in GraphicsContext is left for later (see SVGInlineTextBox.cpp). * LayoutTests/platform/mac-sonoma-wk2-lbse-text/TestExpectations: * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/W3C-SVG-1.1/pservers-grad-03-b-expected.txt: * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/batik/paints/patternRegionA-expected.txt: * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/absolute-root-position-masking-expected.txt: * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/absolute-sized-content-with-resources-expected.png: * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/js-update-pattern-child-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/js-update-pattern-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/oversized-pattern-scale-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-cycle-detection-expected.png: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-cycle-detection-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-excessive-malloc-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-in-defs-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-no-pixelation-expected.png: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-no-pixelation-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-rotate-gaps-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-size-bigger-than-target-size-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-userSpaceOnUse-userToBaseTransform-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-y-offset-expected.png: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pattern-y-offset-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/pending-resource-after-removal-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/custom/recursive-pattern-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/repaint/pattern-object-bounding-box-update-pattern-content-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-lbse-text/svg/repaint/pattern-user-space-on-use-update-pattern-content-expected.txt: Added. * LayoutTests/platform/mac-sonoma-wk2-pixel/svg/custom/pattern-no-pixelation-expected.png: * LayoutTests/platform/mac-sonoma-wk2-pixel/svg/custom/pattern-skew-transformed-expected.png: Removed. * LayoutTests/platform/mac-sonoma-wk2-pixel/svg/custom/pattern-y-offset-expected.png: * LayoutTests/platform/mac-sonoma-wk2-pixel/svg/custom/recursive-pattern-expected.png: * LayoutTests/svg/repaint/pattern-object-bounding-box-expected.txt: Added. * LayoutTests/svg/repaint/pattern-object-bounding-box-transformed-expected.txt: Added. * LayoutTests/svg/repaint/pattern-object-bounding-box-transformed.html: Added. * LayoutTests/svg/repaint/pattern-object-bounding-box-update-pattern-content-expected.txt: Added. * LayoutTests/svg/repaint/pattern-object-bounding-box-update-pattern-content.html: Added. * LayoutTests/svg/repaint/pattern-object-bounding-box.html: Added. * LayoutTests/svg/repaint/pattern-user-space-on-use-expected.txt: Added. * LayoutTests/svg/repaint/pattern-user-space-on-use-transformed-expected.txt: Added. * LayoutTests/svg/repaint/pattern-user-space-on-use-transformed.html: Added. * LayoutTests/svg/repaint/pattern-user-space-on-use-update-pattern-content-expected.txt: Added. * LayoutTests/svg/repaint/pattern-user-space-on-use-update-pattern-content.html: Added. * LayoutTests/svg/repaint/pattern-user-space-on-use.html: Added. * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/rendering/ReferencedSVGResources.cpp: (WebCore::ReferencedSVGResources::referencedSVGResourceIDs): (WebCore::ReferencedSVGResources::referencedPaintServerElement): * Source/WebCore/rendering/RenderLayerModelObject.cpp: (WebCore::RenderLayerModelObject::svgFillPaintServerResourceFromStyle const): (WebCore::RenderLayerModelObject::svgStrokePaintServerResourceFromStyle const): * Source/WebCore/rendering/RenderObject.h: (WebCore::RenderObject::isRenderSVGResourcePaintServer const): (WebCore::RenderObject::isRenderSVGResourcePattern const): * Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp: Added. (WebCore::RenderSVGResourcePattern::RenderSVGResourcePattern): (WebCore::RenderSVGResourcePattern::collectPatternAttributesIfNeeded): (WebCore::clear2DRotation): (WebCore::RenderSVGResourcePattern::buildPattern): (WebCore::RenderSVGResourcePattern::prepareFillOperation): (WebCore::RenderSVGResourcePattern::prepareStrokeOperation): (WebCore::RenderSVGResourcePattern::buildTileImageTransform const): (WebCore::RenderSVGResourcePattern::createTileImage const): * Source/WebCore/rendering/svg/RenderSVGResourcePattern.h: Added. (WebCore::RenderSVGResourcePattern::invalidatePattern): * Source/WebCore/rendering/svg/RenderSVGResourcePatternInlines.h: Added. (WebCore::RenderSVGResourcePattern::patternElement const): (WebCore::calculatePatternBoundaries): * Source/WebCore/rendering/svg/SVGInlineTextBox.cpp: (WebCore::SVGInlineTextBox::acquirePaintingResource): (WebCore::SVGInlineTextBox::releasePaintingResource): (WebCore::SVGInlineTextBox::paintTextWithShadows): * Source/WebCore/rendering/svg/SVGRenderSupport.cpp: (WebCore::SVGHitTestCycleDetectionScope::SVGHitTestCycleDetectionScope): (WebCore::SVGHitTestCycleDetectionScope::~SVGHitTestCycleDetectionScope): * Source/WebCore/rendering/svg/SVGRenderSupport.h: * Source/WebCore/rendering/svg/legacy/LegacyRenderSVGResourcePattern.cpp: (WebCore::LegacyRenderSVGResourcePattern::LegacyRenderSVGResourcePattern): * Source/WebCore/svg/SVGElement.cpp: (WebCore::isSVGLayerAwareElement): * Source/WebCore/svg/SVGPatternElement.cpp: (WebCore::SVGPatternElement::svgAttributeChanged): (WebCore::SVGPatternElement::createElementRenderer): Canonical link: https://commits.webkit.org/273757@main
f20df52
to
db4c995
Compare
Committed 273757@main (db4c995): https://commits.webkit.org/273757@main Reviewed commits have been landed. Closing PR #22427 and removing active labels. |
db4c995
f20df52
π§ͺ wpe-wk2