Bug 63648 - Severe shadow repaint issues with SVGText elements
Summary: Severe shadow repaint issues with SVGText elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-06-29 12:45 PDT by Tim Horton
Modified: 2011-08-01 17:12 PDT (History)
4 users (show)

See Also:


Attachments
test patch (1.69 KB, patch)
2011-06-29 12:46 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
repro (2.04 KB, text/html)
2011-06-29 12:47 PDT, Tim Horton
no flags Details
Alternative patch which touches fewer codepaths but still fixes ours (2.42 KB, patch)
2011-06-29 13:42 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Even more reasonable patch! (736 bytes, patch)
2011-06-29 14:29 PDT, Tim Horton
zimmermann: review-
Details | Formatted Diff | Diff
override visualOverflowRect() in RenderSVGBlock instead (2.20 KB, patch)
2011-07-01 11:33 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
proper version of previous patch, with test and changelog entries (16.82 KB, patch)
2011-07-01 12:29 PDT, Tim Horton
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.49 MB, application/zip)
2011-07-01 13:17 PDT, WebKit Review Bot
no flags Details
Previous patch, using ahem instead (11.75 KB, patch)
2011-07-01 14:27 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
last patch + <rdar> and rebased (11.92 KB, patch)
2011-07-19 10:54 PDT, Tim Horton
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-06-29 12:45:42 PDT
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!
Comment 1 Tim Horton 2011-06-29 12:46:19 PDT
Created attachment 99128 [details]
test patch
Comment 2 Tim Horton 2011-06-29 12:47:20 PDT
Created attachment 99129 [details]
repro
Comment 3 Tim Horton 2011-06-29 13:42:41 PDT
Created attachment 99141 [details]
Alternative patch which touches fewer codepaths but still fixes ours
Comment 4 Tim Horton 2011-06-29 14:29:20 PDT
Created attachment 99150 [details]
Even more reasonable patch!
Comment 5 Nikolas Zimmermann 2011-06-30 01:33:33 PDT
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?
Comment 6 Tim Horton 2011-06-30 10:04:24 PDT
(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.
Comment 7 Tim Horton 2011-06-30 10:06:04 PDT
(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?
Comment 8 Nikolas Zimmermann 2011-06-30 11:46:42 PDT
(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
Comment 9 Tim Horton 2011-07-01 11:33:31 PDT
Created attachment 99491 [details]
override visualOverflowRect() in RenderSVGBlock instead
Comment 10 Tim Horton 2011-07-01 12:29:39 PDT
Created attachment 99503 [details]
proper version of previous patch, with test and changelog entries
Comment 11 WebKit Review Bot 2011-07-01 13:17:40 PDT
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
Comment 12 WebKit Review Bot 2011-07-01 13:17:46 PDT
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
Comment 13 Tim Horton 2011-07-01 14:27:00 PDT
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...
Comment 14 Tim Horton 2011-07-05 16:43:45 PDT
<rdar://problem/7632269>
Comment 15 Tim Horton 2011-07-19 10:54:15 PDT
Created attachment 101344 [details]
last patch + <rdar> and rebased
Comment 16 WebKit Review Bot 2011-07-19 11:47:24 PDT
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
Comment 17 Nikolas Zimmermann 2011-07-20 00:31:53 PDT
Comment on attachment 101344 [details]
last patch + <rdar> and rebased

Excellent patch, this is just right!
Comment 18 Tim Horton 2011-08-01 16:51:04 PDT
Landed in r92152.
Comment 19 Tim Horton 2011-08-01 17:12:22 PDT
Waiting on bots for new baselines for other platforms.