Bug 11984 - SVG <text> does not calculate the correct absoluteRepaintRect
Summary: SVG <text> does not calculate the correct absoluteRepaintRect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL: http://bugs.webkit.org/attachment.cgi...
Keywords:
: 10262 10263 10404 10407 10806 10913 10964 11680 11883 11913 11979 12156 (view as bug list)
Depends on:
Blocks: 12185
  Show dependency treegraph
 
Reported: 2006-12-26 11:22 PST by Eric Seidel (no email)
Modified: 2019-03-29 16:37 PDT (History)
10 users (show)

See Also:


Attachments
Wildfox's half-patch (889 bytes, patch)
2006-12-26 11:44 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Initial patch (58.81 KB, patch)
2007-01-08 15:47 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated patch (48.34 KB, patch)
2007-01-08 16:42 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch. (346.87 KB, patch)
2007-01-08 17:41 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated final patch. (350.23 KB, patch)
2007-01-09 15:39 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-12-26 11:22:18 PST
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.
Comment 1 Eric Seidel (no email) 2006-12-26 11:22:42 PST
Wildfox has a partial patch to fix this.
Comment 2 Eric Seidel (no email) 2006-12-26 11:44:15 PST
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.
Comment 3 Nikolas Zimmermann 2007-01-08 15:47:32 PST
Created attachment 12315 [details]
Initial patch

Large SVG dynamic update fix patch.
Comment 4 Eric Seidel (no email) 2007-01-08 16:36:21 PST
Comment on attachment 12315 [details]
Initial patch

We discussed a number of changes over IRC, WildFox was going to post a new patch.
Comment 5 Eric Seidel (no email) 2007-01-08 16:39:34 PST
*** Bug 10262 has been marked as a duplicate of this bug. ***
Comment 6 Eric Seidel (no email) 2007-01-08 16:39:45 PST
*** Bug 10263 has been marked as a duplicate of this bug. ***
Comment 7 Eric Seidel (no email) 2007-01-08 16:39:53 PST
*** Bug 10404 has been marked as a duplicate of this bug. ***
Comment 8 Eric Seidel (no email) 2007-01-08 16:40:05 PST
*** Bug 10407 has been marked as a duplicate of this bug. ***
Comment 9 Eric Seidel (no email) 2007-01-08 16:40:13 PST
*** Bug 10806 has been marked as a duplicate of this bug. ***
Comment 10 Eric Seidel (no email) 2007-01-08 16:40:22 PST
*** Bug 10913 has been marked as a duplicate of this bug. ***
Comment 11 Eric Seidel (no email) 2007-01-08 16:40:30 PST
*** Bug 10964 has been marked as a duplicate of this bug. ***
Comment 12 Eric Seidel (no email) 2007-01-08 16:40:37 PST
*** Bug 11680 has been marked as a duplicate of this bug. ***
Comment 13 Eric Seidel (no email) 2007-01-08 16:40:48 PST
*** Bug 11883 has been marked as a duplicate of this bug. ***
Comment 14 Eric Seidel (no email) 2007-01-08 16:40:59 PST
*** Bug 11913 has been marked as a duplicate of this bug. ***
Comment 15 Eric Seidel (no email) 2007-01-08 16:41:10 PST
*** Bug 11979 has been marked as a duplicate of this bug. ***
Comment 16 Nikolas Zimmermann 2007-01-08 16:42:01 PST
Created attachment 12318 [details]
Updated patch
Comment 17 Nikolas Zimmermann 2007-01-08 17:41:21 PST
Created attachment 12319 [details]
Final patch.

Final patch including all ChangeLog's & layout test changes
Comment 18 Eric Seidel (no email) 2007-01-08 18:15:59 PST
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.
Comment 19 Nikolas Zimmermann 2007-01-09 15:24:55 PST
*** Bug 12156 has been marked as a duplicate of this bug. ***
Comment 20 Nikolas Zimmermann 2007-01-09 15:32:10 PST
(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
Comment 21 Nikolas Zimmermann 2007-01-09 15:39:38 PST
Created attachment 12337 [details]
Updated final patch.

All issues fixed. Ready for review.
Comment 22 Eric Seidel (no email) 2007-01-09 17:31:59 PST
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.
Comment 23 Nikolas Zimmermann 2007-01-10 03:23:17 PST
(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 :-)
Comment 24 Nikolas Zimmermann 2007-01-10 03:53:07 PST
(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
Comment 25 Sam Weinig 2007-01-11 13:52:59 PST
Landed in r18737.