Closed Bug 869781 Opened 7 years ago Closed 7 years ago

Make nsSVGOuterSVGFrame::Reflow update the overflow rects of its children if it has a viewBox

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(4 files, 1 obsolete file)

I'm pretty sure that SVGSVGElement::FlushImageTransformInvalidation needs to call InvalidateTransformNotifyFrame() if the element has a viewBox. A viewBox will result in the SVGSVGElement and its descendants having a different canvasTM if the dimensions that the SVG is painted into are different between paints. For example, if the same SVG resource is used as the background image of two divs in an HTML document that have different background-size.
Attached file testcase
With the patch from bug 869611, this bug is no longer masked.

Note that the second dive does not paint the orange rect, unless the first div is removed from the document.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jwatt
Attachment #746748 - Flags: review?(dholbert)
No separate test, since this bug causes failures in:

reftests/backgrounds/vector/tall--auto-32px--nonpercent-width-omitted-height-viewbox.html
reftests/backgrounds/vector/tall--auto-32px--nonpercent-width-percent-height-viewbox.html
reftests/backgrounds/vector/tall--auto-32px--omitted-width-omitted-height-viewbox.html
reftests/backgrounds/vector/tall--auto-32px--omitted-width-percent-height-viewbox.html
reftests/backgrounds/vector/tall--auto-32px--percent-width-omitted-height-viewbox.html
reftests/backgrounds/vector/tall--auto-32px--percent-width-percent-height-viewbox.html
At first glance, I'm not sure this is quite the right patch.

The place we notify the internal SVG doc that its viewport is changing is in SVGDocumentWrapper::UpdateViewportBounds()'s call to mViewer->SetBounds(), and the subsequent layout flush:
> 128   // If the bounds have changed, we need to do a layout flush.
> 129   if (currentBounds.Size() != aViewportSize) {
> 130     mViewer->SetBounds(nsIntRect(nsIntPoint(0, 0), aViewportSize));
> 131     FlushLayout();
> 132   }
https://mxr.mozilla.org/mozilla-central/source/image/src/SVGDocumentWrapper.cpp#120

Presumably that layout flush is where we were handling this before.

Inside that same clause (maybe after the layout flush), we probably need something like:
   SVGSVGElement* svgElem = GetRootSVGElem();
   if (svgElem && svgElem->HasViewBoxRect()) {
     svgElem->DoSomethingThatTogglesImageNeedsTransformInvalidation();
   }
(and then if that flag is set, we'll invalidate in FlushImageTransformInvalidation, like your existing patch does -- but we'll only do that if we have to.  In particular, we'll only do it if the viewport size has changed.)
(if it's not clear: DoSomethingThatTogglesImageNeedsTransformInvalidation would be a new method, with a better name (maybe "RequestImageTransformInvalidation()?") that toggles mImageNeedsTransformInvalidation to true)

(THOUGH: it'd be much nicer if we could have this happen automatically in the SetBounds or FlushLayout calls... but we could do it in some new method on SVGSVGElement if necessary.)

I'm also somewhat interested to see whether this fixes itself when you fix the other reftest failures (the breaktest in bug 869611).
Attached patch counter-patchSplinter Review
(In reply to Daniel Holbert [:dholbert] from comment #8)
> (THOUGH: it'd be much nicer if we could have this happen automatically in
> the SetBounds or FlushLayout calls... but we could do it in some new method
> on SVGSVGElement if necessary.)

Actually, there is a good place where we can do this, inside of that existing SetBounds call quoted in comment 6.

SetBounds ends up triggering a call to nsSVGOuterSVGFrame::Reflow, which calls svgElem->SetViewportSize if our viewport size has changed. And inside of svgElem->SetViewportSize, we can toggle mImageNeedsTransformInvalidation.

Here's a patch that fixes the bug for me, doing the above. (With this applied on top of bug 869611, I can make it successfully through layout/reftests/backgrounds, and the attached testcase renders as expected.)
Attachment #746772 - Flags: review?(jwatt)
Attachment #746748 - Flags: review?(dholbert)
Attachment #746748 - Attachment is obsolete: true
I did some more digging. So the real problem here is that nsSVGOuterSVGFrame::Reflow isn't updating the overflow rects of its anonymous child or of its real children when the viewport changes size and it has a children-only transform (a viewBox) that affects their overflow rects.
Comment on attachment 746772 [details] [diff] [review]
counter-patch

The only reason that your counter-patch works is because it results in SVGSVGElement::ChildrenOnlyTransformChanged being called _after_ reflow, which allows the nsLayoutUtils::PostRestyleEvent() call in there to actually take place. Really we want to do a separate restyle and reflow though just because nsSVGOuterSVGFrame::Reflow is failing to do it's job. We should fix nsSVGOuterSVGFrame::Reflow.

Counter-counter-patch ;) coming up once I test it.
Attachment #746772 - Flags: review?(jwatt) → review-
s/Really we want/Really we *don't* want/
Yeah, the whole old viewport dimensions <= 0.0f thing is just handling one special case where tests previously failed. It's just wrong.
Attachment #746913 - Flags: review?(dholbert)
Summary: SVGSVGElement::FlushImageTransformInvalidation needs to invalidate transforms for viewBox → Make nsSVGOuterSVGFrame::Reflow update the overflow rects of its children if it has a viewBox
Comment on attachment 746913 [details] [diff] [review]
counter-counter-patch

Looks good!
Attachment #746913 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/7b7eea794c8e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.