Severe shadow repaint issues with SVGText elements

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

Steps to Reproduce:

1. Open the attached HTML file.
2. Click and drag the "456" element straight down.

Expected outcome:

Both elements and the shadow of both elements remains on the screen.

Actual outcome:

The shadow of the element not being dragged disappears.

Diagnosis:

This occurs because the bounds of the text's shadow are not taken into account when deciding whether or not the "123" element needs to be redrawn.

The attached patch fixes this (obviously incomplete because of tests and changelog), but I have a feeling it's a really bad way to do so, so I'd appreciate any comments!

[Tim Horton]

Created attachment 99128 [details]
test patch

[Tim Horton]

Created attachment 99129 [details]
repro

[Tim Horton]

Created attachment 99141 [details]
Alternative patch which touches fewer codepaths but still fixes ours

[Tim Horton]

Created attachment 99150 [details]
Even more reasonable patch!

[Nikolas Zimmermann]

Comment on attachment 99150 [details]
Even more reasonable patch!

Hm, we're intentionally not using the layout overflow mechanism anywhere in SVG.
We actually consider text shadow in:

FloatRect RenderSVGText::repaintRectInLocalCoordinates() const

    FloatRect repaintRect = strokeBoundingBox();
    SVGRenderSupport::intersectRepaintRectWithResources(this, repaintRect);

    if (const ShadowData* textShadow = style()->textShadow())
        textShadow->adjustRectForShadow(repaintRect);

    return repaintRect;
}

Is this not enough? If so why?

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 99150 [details])
> Hm, we're intentionally not using the layout overflow mechanism anywhere in SVG.
> We actually consider text shadow in:
> ...
> Is this not enough? If so why?

My *understanding* is that this function that you have (repaintRectInLocalCoordinates) is used to decide what part of "us" to redraw if we take damage (on the way up the redraw chain), but that this is not the rect used to determine if we need to redraw when *other* things take damage (on the way back down).

I could be wrong, though.

Look at RenderBlock::paint, which decides if a block needs to redraw on the way back down the chain, and note that the rect it uses to determine if redraw is necessary is visualOverflowRect() (adjusted and inflated slightly, but that's the majority of it).

visualOverflowRect() is a) the bounds of the overflow and element if we have overflow, or b) borderBoxRect() if we don't have any.

Since borderBoxRect() doesn't take into account the shadow, this is where we're failing.

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 99150 [details])
> Hm, we're intentionally not using the layout overflow mechanism anywhere in SVG.

Also, just for understanding's sake, what is the reason?

[Nikolas Zimmermann]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 99150 [details] [details])
> > Hm, we're intentionally not using the layout overflow mechanism anywhere in SVG.
> 
> Also, just for understanding's sake, what is the reason?

painting is handled differently for each svg renderer, that inherits from RenderSVGModelObject (opposed to other renderers like RenderBlock, which end up inheriting from RenderBox -> RenderBoxModelObject). This differentations between a rendering mode that's based upon the CSS box model object, or SVGs rendering model, which historically handled transformations differently like HTML. (CSS transform are applied on new RenderLayer's - for SVG transforms are handled right in the renderers themselves - that dates way back fore CSS transforms were not existant).

The <path> renderer RenderSVGPath is an example for a svg renderer inheriting from RenderSVGModelObject. In its paint() method, we're using the repaintRectInLocalCoordinates() to determine the repaint rect, and do intersection with the damage rect inside paintInfo.

void RenderSVGPath::paint(PaintInfo& paintInfo, const IntPoint&)
{
    if (paintInfo.context->paintingDisabled() || style()->visibility() == HIDDEN || m_path.isEmpty())
        return;

    FloatRect boundingBox = repaintRectInLocalCoordinates();
    if (!SVGRenderSupport::paintInfoIntersectsRepaintRect(boundingBox, m_localTransform, paintInfo))
        return;
...

For RenderBlock::paint it's done using the visualOverflowRect() as you correctly said! RenderSVGText inherits from RenderSVGBlock which in turn inherits from RenderBlock. It's a RenderBoxModelObject derived class, instead - what you probably expect - RenderSVGModelObject. This has numerous reasons (reuse the same code path as HTML text, etc.) that I don't want to comment.

We completly forgot about adjusting the visualOverflowRect() in the case of RenderSVGText (as it's special in the SVG tree, as it does NOT inherit from RenderSVGModelObject, whose repaintRectInLocalCoordinates method is usually used for that).

If you grep for RenderSVGBlock inheritance in rendering/svg:
RenderSVGForeignObject.h:class RenderSVGForeignObject : public RenderSVGBlock {
RenderSVGText.h:class RenderSVGText : public RenderSVGBlock {

You'll see that RenderSVGForeignObject seems potentially affected as well.
How about a generic solution in RenderSVGBlock that takes care of computing the correct visual overflow if the repaintRectInLocalCoordinates() is larger than the borderBoxRect() - the case that we currently fail.

Excellent that you brought this up!
you

[Tim Horton]

Created attachment 99491 [details]
override visualOverflowRect() in RenderSVGBlock instead

[Tim Horton]

Created attachment 99503 [details]
proper version of previous patch, with test and changelog entries

[WebKit Review Bot]

Comment on attachment 99503 [details]
proper version of previous patch, with test and changelog entries

Attachment 99503 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8978065

New failing tests:
svg/custom/repaint-shadow.svg

[WebKit Review Bot]

Created attachment 99514 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tim Horton]

Created attachment 99523 [details]
Previous patch, using ahem instead

Will using ahem make it so I don't have to create results for chromium? Let's try it...

[Tim Horton]

<rdar://problem/7632269>

[Tim Horton]

Created attachment 101344 [details]
last patch + <rdar> and rebased

[WebKit Review Bot]

Comment on attachment 101344 [details]
last patch + <rdar> and rebased

Attachment 101344 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9153259

New failing tests:
svg/custom/repaint-shadow.svg

[Nikolas Zimmermann]

Comment on attachment 101344 [details]
last patch + <rdar> and rebased

Excellent patch, this is just right!

[Tim Horton]

Landed in r92152.

[Tim Horton]

Waiting on bots for new baselines for other platforms.