| Summary: | SVG <text> does not calculate the correct absoluteRepaintRect | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||
| Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | ciccioriccio, gblock, lenricci, mc, mitz, ricciadams, ricksmail03, russgum, wadicyxi, webkit.org, zimmermann | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 420+ | ||||||||||||||
| Hardware: | Mac | ||||||||||||||
| OS: | OS X 10.4 | ||||||||||||||
| URL: | http://bugs.webkit.org/attachment.cgi?id=12032 | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 12185 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Eric Seidel (no email)
2006-12-26 11:22:18 PST
Wildfox has a partial patch to fix this. 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.
Created attachment 12315 [details]
Initial patch
Large SVG dynamic update fix patch.
Comment on attachment 12315 [details]
Initial patch
We discussed a number of changes over IRC, WildFox was going to post a new patch.
*** Bug 10262 has been marked as a duplicate of this bug. *** *** Bug 10263 has been marked as a duplicate of this bug. *** *** Bug 10404 has been marked as a duplicate of this bug. *** *** Bug 10407 has been marked as a duplicate of this bug. *** *** Bug 10806 has been marked as a duplicate of this bug. *** *** Bug 10913 has been marked as a duplicate of this bug. *** *** Bug 10964 has been marked as a duplicate of this bug. *** *** Bug 11680 has been marked as a duplicate of this bug. *** *** Bug 11883 has been marked as a duplicate of this bug. *** *** Bug 11913 has been marked as a duplicate of this bug. *** *** Bug 11979 has been marked as a duplicate of this bug. *** Created attachment 12318 [details]
Updated patch
Created attachment 12319 [details]
Final patch.
Final patch including all ChangeLog's & layout test changes
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.
*** Bug 12156 has been marked as a duplicate of this bug. *** (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 Created attachment 12337 [details]
Updated final patch.
All issues fixed. Ready for review.
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. (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 :-) (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 Landed in r18737. |