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
SVG clip-path is sometimes broken on stevejobsarchive.com #12845
SVG clip-path is sometimes broken on stevejobsarchive.com #12845
Conversation
EWS run on previous version of this PR (hash aadf5e6) |
@@ -167,7 +167,7 @@ bool RenderSVGResourceClipper::applyClippingToContext(GraphicsContext& context, | |||
if (!clipperData.imageBuffer && pathOnlyClipping(context, animatedLocalTransform, objectBoundingBox, effectiveZoom)) | |||
return true; | |||
|
|||
if (clipperData.invalidate(computeInputs(renderer, objectBoundingBox, clippedContentBounds, effectiveZoom))) { | |||
if (clipperData.invalidate(computeInputs(renderer, context, objectBoundingBox, clippedContentBounds, effectiveZoom))) { |
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 not computeInputs()
and applyClippingToContext()
have the arguments: RenderElement and GraphicsContext in the same order: (const RenderElement&, const GraphicsContext& ...)
or (const GraphicsContext&, const RenderElement& ...)
?
@@ -45,6 +45,7 @@ struct ClipperData { | |||
FloatRect clippedContentBounds; | |||
FloatSize scale; | |||
float effectiveZoom = 1; | |||
bool paintingDisabled { false }; |
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.
Alternatively ClipperData::invalidate()
can check
context.hasPlatformContext() == imageBuffer->context().hasPlatformContext()
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 won't work; both the NullGraphicsContext and the GPUP-based GraphicsContext have no platform contexts.
bool operator==(const Inputs& other) const | ||
{ | ||
return std::tie(objectBoundingBox, clippedContentBounds, scale, effectiveZoom) == std::tie(other.objectBoundingBox, other.clippedContentBounds, other.scale, other.effectiveZoom); | ||
return std::tie(objectBoundingBox, clippedContentBounds, scale, effectiveZoom, paintingDisabled) == std::tie(other.objectBoundingBox, other.clippedContentBounds, other.scale, other.effectiveZoom, other.paintingDisabled); | ||
} |
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 this just be
bool operator==(const Inputs&) const = default;
aadf5e6
to
0ad7712
Compare
EWS run on current version of this PR (hash 0ad7712) |
https://bugs.webkit.org/show_bug.cgi?id=255577 rdar://107885344 Reviewed by Said Abou-Hallawa. http://book.stevejobsarchive.com/ uses CSS clip-path with a reference to an SVG <clipPath> element which contains text. In this configuration, RenderSVGResourceClipper::applyClippingToContext() falls back to a code path that uses an ImageBuffer as a mask, and it caches the ImageBuffer between calls. This caused a problem when DOM Rendering in the GPU Process was enabled; this code is first hit for a "fake" paint with a NullGraphicsContext which is updating EventRegions, called out of `RenderLayerBacking::updateEventRegion()`. The NullGraphicsContext will make a local ImageBuffer. If we then hit this same code for actual painting with a painting GraphicsContext, we'll use that cached ImageBuffer, rather than creating a new one with appropriate GPU Process backing. Fix this by adding `isPaintingDisabled` to the criteria used to decide if the cached buffer can be re-used. * LayoutTests/svg/masking/masking-with-event-region-expected.html: Added. * LayoutTests/svg/masking/masking-with-event-region.html: Added. * Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp: (WebCore::RenderSVGResourceClipper::computeInputs): (WebCore::RenderSVGResourceClipper::applyClippingToContext): * Source/WebCore/rendering/svg/RenderSVGResourceClipper.h: (WebCore::ClipperData::Inputs::operator== const): Canonical link: https://commits.webkit.org/263087@main
0ad7712
to
e4d2a74
Compare
Committed 263087@main (e4d2a74): https://commits.webkit.org/263087@main Reviewed commits have been landed. Closing PR #12845 and removing active labels. |
e4d2a74
0ad7712
π§ͺ wpe-wk2π§ͺ ios-wk2π§ͺ ios-wk2-wptπ§ͺ gtk-wk2π§ͺ api-iosπ§ͺ mac-AS-debug-wk2