CSS animations in SVG do not repaint when SVG is zoomed #4727
Conversation
|
EWS run on previous version of this PR (hash 973febc) |
webkit-ews-buildbot
added
the
merging-blocked
Ahmad-S792
force-pushed
the
fix199364
branch
from
973febc
to
2fd947e
Compare
|
EWS run on previous version of this PR (hash 2fd947e) |
Ahmad-S792
removed
the
merging-blocked
webkit-ews-buildbot
added
the
merging-blocked
Ahmad-S792
force-pushed
the
fix199364
branch
from
2fd947e
to
dd59910
Compare
|
EWS run on previous version of this PR (hash dd59910)
|
Ahmad-S792
force-pushed
the
fix199364
branch
from
dd59910
to
fc45d86
Compare
|
EWS run on previous version of this PR (hash fc45d86)
|
Ahmad-S792
force-pushed
the
fix199364
branch
from
fc45d86
to
46cc55f
Compare
|
EWS run on previous version of this PR (hash 46cc55f)
|
Ahmad-S792
removed
the
merging-blocked
Ahmad-S792
requested review from
nikolaszimmermann,
rwlbuis,
smfr and
shallawa
webkit-ews-buildbot
added
the
merging-blocked
CSS animations in SVG do not repaint when SVG is zoomed https://bugs.webkit.org/show_bug.cgi?id=199364 Reviewed by NOBODY (OOPS!). Merge - https://src.chromium.org/viewvc/blink?view=revision&revision=174874 This patch changes the coordinate system used by RenderSVGImage for invalidation rects to account for zoom. When an SVG image issues invalidations to its ImageResource clients, the invalidation rect does not contain zoom. This is due to how SVG images work internally: images are laid out without zoom so that relative sizes are resolved correctly; as a result the invalidation rects do not include zoom. This patch simply removes the zoom from the image size in RenderSVGImage::repaintOrMarkForLayout and clarifies the comment. This change should not affect bitmap image invalidation because bitmaps (even animated gifs) do not use invalidation rects. * Source/WebCore/rendering/svg/RenderSVGImage.cpp: (RenderSVGImage::repaintOrMarkForLayout): Update Comment and simplify logic to remove for 'zoom' factor * LayoutTests/svg/repaint/resources/animate-center.svg: Added Resource for Testcase * LayoutTests/svg/repaint/image-animation-with-zoom.html: Added Test Case * LayoutTests/svg/repaint/image-animation-with-zoom-expected.txt: Added Test Case Expectations
Ahmad-S792
force-pushed
the
fix199364
branch
from
46cc55f
to
d17f0b0
Compare
|
EWS run on current version of this PR (hash d17f0b0) |
| // so map from the bounds of the image to the contentsBox. | ||
| repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), repaintRect))); | ||
| const LayoutSize imageSizeWithoutZoom = m_imageResource->imageSize(1 / style().effectiveZoom()); | ||
| repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageSizeWithoutZoom), repaintRect))); |
There was a problem hiding this comment.
RenderSVGImage is only used for LBSE. Is your plan not to fix it for LegacyRenderSVGImage? Also RenderSVGImage and LegacyRenderSVGImage are created for SVG <image> element https://developer.mozilla.org/en-US/docs/Web/SVG/Element/image. Your test case does not include any SVG <image> element. Instead it includes the HTML <img> element which is rendered through SVGImage::draw().
There was a problem hiding this comment.
Sure - I will add similar to LegacyRenderSVGImage. As for the test case, I am doing merge from Blink patch and it is more repaint / style invalidation fix when CSS animations are applied to SVG image in <img> rather than <image> element while zoomed. If you want me to add <image> case then I can try but I just started dwelling in Webkit Codebase and still novice so I might not be able to do it for sure but worth giving it a try.
There was a problem hiding this comment.
I just had a look into LegacyRenderSVGImage and I think it does not have same codepath as the above and in some point from Legacy to LBSE, we moved this part to LBSE use and I think this Blink merge would be limited to this path. If I am missing anything, happy to be guided.
There was a problem hiding this comment.
The reported test case in Bugzilla https://jstsch.com/misc/2019/7/safari-svg-zoom.html is not fixed with this change. This happens with LBSE on or off.
Also when opening the test case you added in this PR in mini-browser, it is not displayed correctly with this change. A pink rectangle is drawn with smaller green rectangle at its top-left corner. When refreshing the page only a green rectangle is displayed. This happens with LBSE on and off.
So I am not sure what this PR is fixing. I think you need to investigate these two cases and understand the problem more. Feel free to ask questions on slack. Also make sure the added layout test exercises the modified code.
There was a problem hiding this comment.
Closing PR to investigate and documenting stuff on bug as well.
There was a problem hiding this comment.
LBSE + zooming is currently broken anyhow -- a general fix is underways for LBSE that will fix zooming/panning for absolute/relative sized documents with/without viewBox, within subtrees, SVGImages, etc..
-> A huge interop benefit, as Safari currently deviates (impossible to enlarge SVGs with relative sized width/height that have a viewBox set) from all other browsers [funny thought that I. made it like this also to restore inter-op in 2012, where Opera/FF followed these rules).
| setTimeout(function() { | ||
| if (window.testRunner) | ||
| finishRepaintTest(); | ||
| }, 220); |
There was a problem hiding this comment.
Using setTimeout(..., 220) here is a little bit fragile. I do not think there I a guarantee that the CSS animation in the SVG image resource will finish in 200ms.
There was a problem hiding this comment.
Should I increase it? I looked into this test case from Chromium source and it is using 'rAf' but we don't have infrastructure in TestRunner to use "paintUnderInvalidationCheckingEnabled" and those bits so it wouldn't be possible to port latest Chromium test case code here.
There was a problem hiding this comment.
This is guaranteed to lead to flakey tests -- the timing cannot be guaranteed like this.
d17f0b0