Bug 9753

Summary: SVG with width and height 100% leaves room for scrollbar on the right.
Product: WebKit Reporter: Joost de Valk (AlthA) <joost>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, ddkilzer, mjs, rwlbuis
Priority: P2 Keywords: HasReduction
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
testcase
none
First attempt
none
Improved patch+testcase
none
Improved patch+testcase
none
Improved patch+testcase
adele: review-
Another try mjs: review+

Joost de Valk (AlthA)
Reported 2006-07-06 04:52:24 PDT
Testcase says it all, look on the right side of your window.
Attachments
testcase (108 bytes, application/xml)
2006-07-06 04:52 PDT, Joost de Valk (AlthA) 		no flags
Details
First attempt (9.44 KB, patch)
2006-07-09 07:28 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Improved patch+testcase (10.79 KB, patch)
2006-07-09 12:32 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Improved patch+testcase (10.74 KB, patch)
2006-07-09 13:51 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Improved patch+testcase (14.15 KB, patch)
2006-07-10 13:13 PDT, Rob Buis 		adele: review-
DetailsFormatted DiffDiff
Another try (737.87 KB, patch)
2006-09-06 13:30 PDT, Rob Buis 		mjs: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Joost de Valk (AlthA)
Comment 1 2006-07-06 04:52:50 PDT
Created attachment 9227 [details] testcase
Rob Buis
Comment 2 2006-07-09 07:28:14 PDT
Created attachment 9296 [details] First attempt This patch actually solves a bigger problem, relayouting/resizing of svg elements that depend on percentages. I will add a testcase should the code get okayed. Cheers, Rob.
Eric Seidel (no email)
Comment 3 2006-07-09 07:46:40 PDT
Comment on attachment 9296 [details] First attempt Looks good. There are a couple mis-placed *'s: RenderObject * RenderPath * Also, I've more commonly seen multi-line ifs broken before the binary operator, instead of after, example: if (foo || bar || baz) Not sure if there is an official practice in that regard or not. r=me.
Rob Buis
Comment 4 2006-07-09 12:32:52 PDT
Created attachment 9304 [details] Improved patch+testcase I fixed MacDome's issues, the || we decided is not crucial since it is done inconsistent throughout webkit codebase. I also added a testcase based on Joost's one. Note that all svg tests are changed because all RenderSVGText objects get 15 added to the width because the scrollbar is not taken into account anymore. Cheers, Rob.
Sam Weinig
Comment 5 2006-07-09 13:07:24 PDT
Rob, One other quick style issue is that you have some single line if-statements with curly braces.
Rob Buis
Comment 6 2006-07-09 13:51:59 PDT
Created attachment 9305 [details] Improved patch+testcase Fixing Weinigs issues :) Cheers, Rob.
Maciej Stachowiak
Comment 7 2006-07-10 00:44:40 PDT
Comment on attachment 9305 [details] Improved patch+testcase r=me Really though I think we ultimately want more of percentage handling to be in the render tree, not the DOM.
David Kilzer (:ddkilzer)
Comment 8 2006-07-10 07:12:40 PDT
Commit r15300 (http://paste.lisp.org/display/22342) appears to have caused the following additional SVG test to fail: svg/W3C-SVG-1.1/pservers-grad-08-b.svg
David Kilzer (:ddkilzer)
Comment 9 2006-07-10 07:58:34 PDT
(In reply to comment #8) > Commit r15300 (http://paste.lisp.org/display/22342) appears to have caused the > following additional SVG test to fail: > > svg/W3C-SVG-1.1/pservers-grad-08-b.svg This is not related to this bug.  Filed Bug 9834 for it, though.
Rob Buis
Comment 10 2006-07-10 13:13:55 PDT
Created attachment 9346 [details] Improved patch+testcase This patch has a better testcase, now it also tests resizing of lines, circles and ellipses, which proved to have some logical errors in the way they resize using percentages, the improved testcase should catch this in future. Sorry for having to clear the review bits again :} Cheers, Rob.
David Kilzer (:ddkilzer)
Comment 11 2006-07-12 07:38:35 PDT
Comment on attachment 9296 [details] First attempt Clearing review flag.
David Kilzer (:ddkilzer)
Comment 12 2006-07-12 07:41:29 PDT
Comment on attachment 9305 [details] Improved patch+testcase Clearing review flag.
Maciej Stachowiak
Comment 13 2006-07-13 00:39:32 PDT
Comment on attachment 9346 [details] Improved patch+testcase r=me
Adele Peterson
Comment 14 2006-07-27 21:47:14 PDT
Committed revision 15665.
David Kilzer (:ddkilzer)
Comment 15 2006-07-28 03:27:00 PDT
The patch in Attachment 9346 [details] caused ~190 SVG tests to regress (or need updates to their layout tests). http://build.webkit.org/changes/3041 http://build.webkit.org/results/post-commit-powerpc-mac-os-x/2851/
Joost de Valk (AlthA)
Comment 16 2006-07-28 04:41:41 PDT
I bet all of those need updated results.
Adele Peterson
Comment 17 2006-07-28 11:55:54 PDT
Comment on attachment 9346 [details] Improved patch+testcase R-'ing until we work out the layout test failures
Rob Buis
Comment 18 2006-09-06 13:30:24 PDT
Created attachment 10425 [details] Another try This one compiles against ToT. It contains all changed svg testcases, I think they need careful scrutiny to see whether only some widths changed. I remember something about these changes hiding some errors already in testcase results, is that still the case or was that another bug? Cheers, Rob.
Maciej Stachowiak
Comment 19 2006-09-06 13:36:13 PDT
Comment on attachment 10425 [details] Another try r=me
Rob Buis
Comment 20 2006-09-07 02:30:50 PDT
Landed in r16259.
Note You need to log in before you can comment on or make changes to this bug.