WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Updated Patch
(8.23 KB, patch)
2012-02-14 13:51 PST
,
Bear Travis
krit
: review-
Details
Formatted Diff
Diff
Updated Patch
(13.26 KB, patch)
2012-02-14 17:13 PST
,
Bear Travis
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Updated Patch
(9.92 KB, patch)
2012-02-15 13:00 PST
,
Bear Travis
no flags
Details
Formatted Diff
Diff
Updated Patch
(9.77 KB, patch)
2012-02-15 16:21 PST
,
Bear Travis
zimmermann
: review+
Details
Formatted Diff
Diff
Updated Patch
(10.47 KB, patch)
2012-02-15 16:46 PST
,
Bear Travis
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug