Bug 11984 – SVG <text> does not calculate the correct absoluteRepaintRect

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVG <text> does not calculate the correct absoluteRepaintRect

I'm splitting this out from:
http://bugs.webkit.org/show_bug.cgi?id=11979

specifically:
http://bugs.webkit.org/attachment.cgi?id=12032
is the test case which this bug is all about fixing.

[Eric Seidel (no email)]

Wildfox has a partial patch to fix this.

[Eric Seidel (no email)]

Created attachment 12039 [details]
Wildfox's half-patch

This is a stab in the right direction, but really doesn't solve the problem.  With this patch applied way more than enough area is being invalidated.  I assume because RenderFlow has some logic in it to invalidate the entire line when any part of that line changes.  So this isn't the right fix, but it's a start.

[Nikolas Zimmermann]

Created attachment 12315 [details]
Initial patch

Large SVG dynamic update fix patch.

[Eric Seidel (no email)]

Comment on attachment 12315 [details]
Initial patch

We discussed a number of changes over IRC, WildFox was going to post a new patch.

[Eric Seidel (no email)]

*** Bug 10262 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 10263 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 10404 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 10407 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 10806 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 10913 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 10964 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 11680 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 11883 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 11913 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 11979 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 12318 [details]
Updated patch

[Nikolas Zimmermann]

Created attachment 12319 [details]
Final patch.

Final patch including all ChangeLog's & layout test changes

[Eric Seidel (no email)]

Comment on attachment 12319 [details]
Final patch.

This patch has some typos: "layouting" and "childre".

I'm not sure the removal of the hasPercentagValues() is wise.  We'll need to add something like that back again.  It may be simpler to just fix the existing optimization.  I'm still not sure what was wrong with it.

I'm confused why there are all these changes:
-      RenderSVGContainer {g} at (7,26) size 420x221
+      RenderSVGContainer {g} at (14,26) size 413x221

I assume they're related to text now having the correct bboxes?  Yet the SVGText and SVGInlineTExt objects haven't chagned?

         RenderSVGText {text} at (225,40) size 480x17
           RenderSVGInlineText {#text} at (-197,-14) size 394x17


Also it looks like SVG text objects dump relative bboxes (relative to what?) instead of absolute bboxes.  That seems wrong.

This is a big patch.  A good patch.  But being big, makes it a bit unwieldy... I'm certain this patch would cause a performance regression for resizing the window with SVGs (suddenly now SVGs will repaint themselves on any window resize, even if not necessary).

Perhaps we can land a smaller piece of this patch first?  A piece where it's easier to understand the layout test results changes?  Even something as simple as just the RenderSVGImage changes might make an easy sub-patch to land first.

[Nikolas Zimmermann]

*** Bug 12156 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

(In reply to comment #18)
> (From update of attachment 12319 [details] [edit])
> This patch has some typos: "layouting" and "childre".
Fixed.
 
> I'm not sure the removal of the hasPercentagValues() is wise.  We'll need to
> add something like that back again.  It may be simpler to just fix the existing
> optimization.  I'm still not sure what was wrong with it.
It wasn't wise. I added a new "hasRelativeValues()" function, also checking
for emx/exs who depend on RenderStyle for their calculation. Added isRelative()
helper to SVGLength - to avoid code duplication everywhere.

After all it was worth gold to investigate there, it showed the real bug:
we _never_ repainted SVG containers. RenderContainer::layout() was used,
which called setNeedsLayout(false) at the end, and three lines below in
RenderSVGContainer we checked if (selfNeedsLayout()...
I discussed this with olliej, proper fix now in.

Thanks again for kicking me to check that :-)
 
> I'm confused why there are all these changes:
> -      RenderSVGContainer {g} at (7,26) size 420x221
> +      RenderSVGContainer {g} at (14,26) size 413x221
>
> I assume they're related to text now having the correct bboxes?  Yet the
> SVGText and SVGInlineTExt objects haven't chagned?
This is correct. See below for explaination.

> 
>          RenderSVGText {text} at (225,40) size 480x17
>            RenderSVGInlineText {#text} at (-197,-14) size 394x17
> 
> 
> Also it looks like SVG text objects dump relative bboxes (relative to what?)
> instead of absolute bboxes.  That seems wrong.
This has always been the case, we never dump absolute values. This should be
a seperated patch fixing those issues. I'll file a bug about this later!
 
> This is a big patch.  A good patch.  But being big, makes it a bit unwieldy...
> I'm certain this patch would cause a performance regression for resizing the
> window with SVGs (suddenly now SVGs will repaint themselves on any window
> resize, even if not necessary).

This is fixed now, even SVG printing works now (!).

> Perhaps we can land a smaller piece of this patch first?  A piece where it's
> easier to understand the layout test results changes?  Even something as simple
> as just the RenderSVGImage changes might make an easy sub-patch to land first.

As all issues are worked out, I don't think I need to split up the patch - it's really
good reviewed now, and well you'd just need to check the last fixes regarding
the issues you mentioned :-)

Greeetings,
Niko

[Nikolas Zimmermann]

Created attachment 12337 [details]
Updated final patch.

All issues fixed. Ready for review.

[Eric Seidel (no email)]

Comment on attachment 12337 [details]
Updated final patch.

http://bugs.webkit.org/show_bug.cgi?id=12064

isn't fixed by this...

http://bugs.webkit.org/show_bug.cgi?id=12156

isn't a real bug, is it?  Printing already worked...

This looks great.

We really should have some additional tests for setSlice invalidation issues, as well as setFontSize causing a re-layout of a containers kids.

This looks great.  We should either add those tests or file bugs for them though.

[Nikolas Zimmermann]

(In reply to comment #22)
> (From update of attachment 12337 [details] [edit])
> http://bugs.webkit.org/show_bug.cgi?id=12064
> 
> isn't fixed by this...
right, leftover in the ChangeLog. Removed.
 
> http://bugs.webkit.org/show_bug.cgi?id=12156
> 
> isn't a real bug, is it?  Printing already worked...
Hm, I removed it - as it worked before already, it just
got better because of this patch. Anyway leaving it out here.

 
> This looks great.
> 
> We really should have some additional tests for setSlice invalidation issues,
> as well as setFontSize causing a re-layout of a containers kids.
> 
> This looks great.  We should either add those tests or file bugs for them
> though.
> 
I'll come up with some tests now.
Yay, finally we got it :-)

[Nikolas Zimmermann]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 12337 [details] [edit] [edit])
> > http://bugs.webkit.org/show_bug.cgi?id=12064
> > 
> > isn't fixed by this...
> right, leftover in the ChangeLog. Removed.
> 
> > http://bugs.webkit.org/show_bug.cgi?id=12156
> > 
> > isn't a real bug, is it?  Printing already worked...
> Hm, I removed it - as it worked before already, it just
> got better because of this patch. Anyway leaving it out here.
> 
> 
> > This looks great.
> > 
> > We really should have some additional tests for setSlice invalidation issues,
> > as well as setFontSize causing a re-layout of a containers kids.
> > 
> > This looks great.  We should either add those tests or file bugs for them
> > though.
> > 
> I'll come up with some tests now.
> Yay, finally we got it :-)
> 
Okay, I changed my mind - the tests need to be done in a seperated patch,
as I already found some problems with it (ie. SVGPreserveAspectRatio JS constructor
not available etc..). I think there is even a bug for it, we should use that one to
fix these issues.

Landing the patch now, without new LayoutTests.

Niko

[Sam Weinig]

Landed in r18737.