RESOLVED FIXED 77903
Repaint issues on changing 'viewBox' of inner SVG
https://bugs.webkit.org/show_bug.cgi?id=77903
Summary Repaint issues on changing 'viewBox' of inner SVG
Dirk Schulze
Reported 2012-02-06 14:31:03 PST
There are repaint issues on inner SVG elements when the 'viewBox' attribute is changed. This just occurs when there is other content behind the SVG element and it's content. I have not figured out what happens, but it seems that previous content gets repainted correctly. It seems to be an issue with painting the SVG content itself. Maybe problems with transformations? This needs more investigation. To see this issue, bug 69459 needs to get fixed first. But it seems to be unrelated to bug 69459.
Attachments
Proposed Patch (6.87 KB, patch)
2012-02-13 20:12 PST, Bear Travis
zimmermann: review-
Updated Patch (8.23 KB, patch)
2012-02-14 13:51 PST, Bear Travis
krit: review-
Updated Patch (13.26 KB, patch)
2012-02-14 17:13 PST, Bear Travis
webkit.review.bot: commit-queue-
Updated Patch (9.92 KB, patch)
2012-02-15 13:00 PST, Bear Travis
no flags
Updated Patch (9.77 KB, patch)
2012-02-15 16:21 PST, Bear Travis
zimmermann: review+
Updated Patch (10.47 KB, patch)
2012-02-15 16:46 PST, Bear Travis
no flags
Bear Travis
Comment 1 2012-02-13 20:12:20 PST
Created attachment 126897 [details] Proposed Patch A patch proposal. The problem occurred because repainting used the new viewbox transform for both the old and new bounding rectangles. The patch updates the transform after the old bounds have been marked as part of layout. Running the bots, then will mark for review.
Nikolas Zimmermann
Comment 2 2012-02-14 00:56:33 PST
Comment on attachment 126897 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126897&action=review r- for the test, and the code changes, which are not a perfect solution yet. > LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:2 > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" > + width="300" height="300" onload="window.setTimeout(resizeViewBox, 0)"> I think it's not a good idea to use reftests here - we really want to check the repainting of inner <svgs>, and thus the repaint.js harness is really helpful, to spot in the pngs that repainting worked correctly. I propose to use: <svg onload="runRepaintTest()"> <script xlink:href="../../fast/repaint/resources/repaint.js"> .. > LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:5 > +if (window.layoutTestController) > + layoutTestController.waitUntilDone(); This can be removed. > LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:7 > +function resizeViewBox() { Rename this to repaintTest(), as it gets called by runRepaintTest for you, after the initial painting finished. > LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:10 > + if (window.layoutTestController) > + layoutTestController.notifyDone(); This can be removed as well. > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:68 > + m_viewbox = svg->currentViewBoxRect(); Slightly worried that we have to store m_viewbox and m_viewport now. I think viewBox changes should be handled from the SVG DOM side, if viewBox attr changes. I'll investigate a bit and come back to your patch soon.
Bear Travis
Comment 3 2012-02-14 13:51:56 PST
Created attachment 127037 [details] Updated Patch Updated the test to use the repaint harness. Updated the code transform invalidation to use the svg dom rather than the renderer, which avoids storing the viewbox rectangle.
Dirk Schulze
Comment 4 2012-02-14 14:49:12 PST
Comment on attachment 127037 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127037&action=review I think this is the right approach. In general a viewBox is nothing else than a transform. r- because of some snippets. > LayoutTests/svg/repaint/inner-svg-change-viewBox-contract.svg:10 > +<!-- blue rect's lower right edges should be visible after viewbox resizing --> It is not. Is that because of the repaint logic? > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:82 > + bool needsUpdate = m_needsTransformUpdate; > + if (needsUpdate) { > + m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > + // If this class were ever given a localTransform(), then the above would read: > + // return viewportTranslation * localTransform() * viewportTransform() > + m_needsTransformUpdate = false; > + } > + return needsUpdate; why not just if (m_needsTransformUpdate) { ... return true; } return false; Also, should it be If this class _was_ ever given? :P I am not sure about that atm :) > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:-85 > - m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > return m_localToParentTransform; > - // If this class were ever given a localTransform(), then the above would read: > - // return viewportTranslation * localTransform() * viewportTransform() Did you run all tests to verify that we don't get a regression? > Source/WebCore/svg/SVGSVGElement.cpp:297 > + if (renderer()) Why not: if (RenderObject* object = renderer()) object->setNeedsTransformUpdate(); saves a renderer() call. Also, don't we just need to call it for view box updates? What about: if (attrName == SVGNames::viewBoxAttr) { if (RenderObject* object = renderer()) object->setNeedsTransformUpdate(); updateRelativeLengths = true; } After the current if clause? Of course it would violate the name updateRelativeLengths. What about renaming it to updateRelativeLengthsOrViewBox? Of course you have to remove the attName check for viewBox in the later if clause.
Bear Travis
Comment 5 2012-02-14 17:13:32 PST
Created attachment 127085 [details] Updated Patch Updated expected .png so background is blue. You should see a blue edge on the lower right when running the test case. Simplified boolean logic in RenderSVGViewPortContainer::calculateLocalTransform (As an aside, _were_ should work, as a present subjunctive, but _was_ sounds like it might fit too. Thank goodness I'm not an English teacher) All the tests have been run. I'm going to run the EWS again before marking this for review. Adding the (RenderObject * object = renderer()) to the conditional at SVGSVGElement.cpp:297. Broke up the logic in SVGSVGElement::svgAttributeChanged. - SVGFitToViewBox::isKnownAttribute (viewBox or aspectRatio) only needs to update the transform and be marked for layout. It doesn't need to update the relative lengths information, so I'm moving it to its own conditional. - Removing the viewBox check for layout marking, as this will be covered by the updateRelativeLengthsOrViewBox == true case.
WebKit Review Bot
Comment 6 2012-02-14 18:24:03 PST
Comment on attachment 127085 [details] Updated Patch Attachment 127085 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11518527
Nikolas Zimmermann
Comment 7 2012-02-15 00:38:39 PST
Comment on attachment 127085 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127085&action=review Adding my 50 cent as well. Approach is much nicer this way. > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:75 > + if (m_needsTransformUpdate) { Early exit should be used here. > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > + // If this class were ever given a localTransform(), then the above would read: > + // return viewportTranslation * localTransform() * viewportTransform() The comment can go away, it's aged, and we won't do this :-) > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:97 > const AffineTransform& RenderSVGViewportContainer::localToParentTransform() const > { > - m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > return m_localToParentTransform; Please inline this function as it has no content anymore. > Source/WebCore/svg/SVGSVGElement.cpp:290 > - bool updateRelativeLengths = false; > + bool updateRelativeLengthsOrViewBox = false; This is unncessary, I'd leave out this change and just do a if (SVGFitToViewBox::isKnownAttribute(attrName)) { if (RenderObject* renderer = this->renderer()) { renderer->setNeedsTransformUpdate(); RenderSVGresources::markForLayoutAndParent...( } return; } This way you don't need to touch the rest of the code at all, except removing the attrName == SVGNames::viewBoxAttr from the if (updateRelativeLengths branch below)
Dirk Schulze
Comment 8 2012-02-15 09:14:03 PST
(In reply to comment #7) > (From update of attachment 127085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127085&action=review > > Adding my 50 cent as well. Approach is much nicer this way. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:75 > > + if (m_needsTransformUpdate) { > > Early exit should be used here. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > + // If this class were ever given a localTransform(), then the above would read: > > + // return viewportTranslation * localTransform() * viewportTransform() > > The comment can go away, it's aged, and we won't do this :-) Be careful with that. The SVG WG decided to support transform on SVGSVGelements for SVG 2.0.
Bear Travis
Comment 9 2012-02-15 10:58:24 PST
(In reply to comment #7) > (From update of attachment 127085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127085&action=review > > Adding my 50 cent as well. Approach is much nicer this way. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:75 > > + if (m_needsTransformUpdate) { > > Early exit should be used here. > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > + // If this class were ever given a localTransform(), then the above would read: > > + // return viewportTranslation * localTransform() * viewportTransform() > > The comment can go away, it's aged, and we won't do this :-) > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:97 > > const AffineTransform& RenderSVGViewportContainer::localToParentTransform() const > > { > > - m_localToParentTransform = AffineTransform::translation(m_viewport.x(), m_viewport.y()) * viewportTransform(); > > return m_localToParentTransform; > > Please inline this function as it has no content anymore. > > > Source/WebCore/svg/SVGSVGElement.cpp:290 > > - bool updateRelativeLengths = false; > > + bool updateRelativeLengthsOrViewBox = false; > > This is unncessary, I'd leave out this change and just do a > if (SVGFitToViewBox::isKnownAttribute(attrName)) { > if (RenderObject* renderer = this->renderer()) { > renderer->setNeedsTransformUpdate(); > RenderSVGresources::markForLayoutAndParent...( > } > return; > } > This way you don't need to touch the rest of the code at all, except removing the attrName == SVGNames::viewBoxAttr from the if (updateRelativeLengths branch below)
Bear Travis
Comment 10 2012-02-15 11:03:56 PST
Please ignore my last comment, my mouse has decided to have a field day. I'll make these adjustments in the next patch. I'll leave the comment per Dirk's concerns. > > Source/WebCore/svg/SVGSVGElement.cpp:290 > > - bool updateRelativeLengths = false; > > + bool updateRelativeLengthsOrViewBox = false; > > This is unncessary, I'd leave out this change and just do a > if (SVGFitToViewBox::isKnownAttribute(attrName)) { > if (RenderObject* renderer = this->renderer()) { > renderer->setNeedsTransformUpdate(); > RenderSVGresources::markForLayoutAndParent...( > } > return; > } > This way you don't need to touch the rest of the code at all, except removing the attrName == SVGNames::viewBoxAttr from the if (updateRelativeLengths branch below) As I understand it, this would require moving the invalidation guard as well. If we can keep the setTransformUpdate and markForLayout calls separate, we won't invalidate when we are not yet attached.
Bear Travis
Comment 11 2012-02-15 13:00:41 PST
Created attachment 127220 [details] Updated Patch Per Nikolas' comments: * Changed to use early exit in RenderSVGViewportContainer.cpp::calculateLocalTransform * Inlined RenderSVGViewportContainer::localToParentTransform() Per Dirk's comments: * Leaving comment at RenderSVGViewportContainer.cpp::calculateLocalTransform intact Leaving SVGSVGElement.cpp::svgAttribute changed as-is for now The code, per Nikolas' comments, would look like: SVGElementInstance::InvalidationGuard invalidationGuard(this); if (SVGFitToViewBox::isKnownAttribute(attrName)) { if (RenderObject* object = renderer()) { object->setNeedsTransformUpdate(); RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer()); } return; } if (SVGTests::handleAttributeChange(this, attrName)) return; if (updateRelativeLengths || SVGLangSpace::isKnownAttribute(attrName) || SVGExternalResourcesRequired::isKnownAttribute(attrName) || SVGZoomAndPan::isKnownAttribute(attrName)) { if (renderer()) RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer()); return; } I can change this if it looks better. There is a chance we'd invalidate when we don't have to (SVGTests::handleAttributeChange returns true).
Nikolas Zimmermann
Comment 12 2012-02-15 14:36:01 PST
(In reply to comment #8) > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > > + // If this class were ever given a localTransform(), then the above would read: > > > + // return viewportTranslation * localTransform() * viewportTransform() > > > > The comment can go away, it's aged, and we won't do this :-) > Be careful with that. The SVG WG decided to support transform on SVGSVGelements for SVG 2.0. Heh, you misunderstood Erics comment it seems. Eric thought about passing localTransform() as _argument_ to localToParentTransform in his early work, hence the FIXME. It has nothing to do with SVGSVGElements being transformable.
Nikolas Zimmermann
Comment 13 2012-02-15 14:37:26 PST
(In reply to comment #11) > I can change this if it looks better. There is a chance we'd invalidate when we don't have to (SVGTests::handleAttributeChange returns true). You're right, I missed the <use> shadow tree invalidation case. Keep it as-is.
Nikolas Zimmermann
Comment 14 2012-02-15 15:29:58 PST
(In reply to comment #13) > (In reply to comment #11) > > I can change this if it looks better. There is a chance we'd invalidate when we don't have to (SVGTests::handleAttributeChange returns true). > You're right, I missed the <use> shadow tree invalidation case. Keep it as-is. Btw, can you look at bug 77535 as well, it seems highly related (not viewBox though, but relative length props and inner svgs).
Dirk Schulze
Comment 15 2012-02-15 16:10:43 PST
(In reply to comment #12) > (In reply to comment #8) > > > > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:78 > > > > + // If this class were ever given a localTransform(), then the above would read: > > > > + // return viewportTranslation * localTransform() * viewportTransform() > > > > > > The comment can go away, it's aged, and we won't do this :-) > > Be careful with that. The SVG WG decided to support transform on SVGSVGelements for SVG 2.0. > Heh, you misunderstood Erics comment it seems. Eric thought about passing localTransform() as _argument_ to localToParentTransform in his early work, hence the FIXME. > It has nothing to do with SVGSVGElements being transformable. Ok yes, I definitely interpreted it differently :)
Bear Travis
Comment 16 2012-02-15 16:21:26 PST
Created attachment 127270 [details] Updated Patch Removing comment from RenderSVGViewportContainer::calculateLocalTransform Kicking off the bots again
Nikolas Zimmermann
Comment 17 2012-02-15 16:25:58 PST
Comment on attachment 127270 [details] Updated Patch Hm seems the EWS bots are not working. Still it looks great, r=me. Please update the chromium test_expectations.txt file in LayoutTest/platform/chromium and upate the ChangeLog to include this file. It should list sth. like: // New test, needs a rebaseline BUGWK77903 : svg/repaint/innert-sg-change-viewBox-contract.svg = IMAGE IMAGE+TEXT Otherwise chromium will turn red.
Bear Travis
Comment 18 2012-02-15 16:46:07 PST
Created attachment 127275 [details] Updated Patch Updated LayoutTests/chromium/test_expectations.txt and LayoutTests/ChangeLog
WebKit Review Bot
Comment 19 2012-02-15 23:09:14 PST
The commit-queue encountered the following flaky tests while processing attachment 127275 [details]: perf/array-reverse.html bug 78593 (author: ojan@chromium.org) The commit-queue is continuing to process your patch.
WebKit Review Bot
Comment 20 2012-02-15 23:16:50 PST
Comment on attachment 127275 [details] Updated Patch Clearing flags on attachment: 127275 Committed r107893: <http://trac.webkit.org/changeset/107893>
WebKit Review Bot
Comment 21 2012-02-15 23:16:57 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.