Summary: | SVGTextElement.getSubStringLength does not work as expected (affects Acid3 test 77) | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | jwalden+bwo, zimmermann | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 17064 | ||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2008-01-29 14:53:54 PST
We only support kerning for SVG Text (ie. <text kerning=".."..> though we don't support <hkern> / <vkern> for SVG Fonts - this may be the result. SVG Font + kerning should be an easy & fun task. Created attachment 19270 [details]
Patch that makes this test pass
Fix some minor bugs with substrings.
Comment on attachment 19270 [details]
Patch that makes this test pass
first bit could be
if (atCharacter > startPosition + length)
break;
atCharacter++;
Also, this needs plain text testcase -- i won't r+ any non-dumpAsText results :D
Comment on attachment 19270 [details]
Patch that makes this test pass
Clearing review flag. Will leave this up if anyone wants to pick up the bug and finish it. I'm moving on to other bugs.
Comment on attachment 19270 [details]
Patch that makes this test pass
The patch looks totally sane to me. We just need to make a good test case (using the js testing harness) which checks for the getSubStringLength edgecases.
Comment on attachment 19270 [details]
Patch that makes this test pass
I'm going to mark this as r- so it will show up in the forgotten bugs. The patch is fine. It just needs a test case and change log.
(In reply to comment #6) > (From update of attachment 19270 [details] [edit]) > I'm going to mark this as r- so it will show up in the forgotten bugs. The > patch is fine. It just needs a test case and change log. > Agreed... if just my exam on Monday was already finished :-) Greetings, Niko Created attachment 19445 [details]
Fix getSubStringLength range handling.
.../SVGTextElement/getSubStringLength-expected.txt | 19 +++++++++++
.../svg/dom/SVGTextElement/getSubStringLength.html | 13 +++++++
.../svg/dom/SVGTextElement/resources/TEMPLATE.html | 13 +++++++
.../SVGTextElement/resources/getSubStringLength.js | 35 ++++++++++++++++++++
WebCore/svg/SVGTextContentElement.cpp | 15 ++++++--
5 files changed, 91 insertions(+), 4 deletions(-)
Comment on attachment 19445 [details]
Fix getSubStringLength range handling.
Actually, I think this causes a regression in getComputedTextLength() since after this patch we start failing there... which is earlier than getSubStringLength in test order.
(In reply to comment #9) > (From update of attachment 19445 [details] [edit]) > Actually, I think this causes a regression in getComputedTextLength() since > after this patch we start failing there... which is earlier than > getSubStringLength in test order. > At least the test case is useful going forward, even if my patch is wrong. :) Created attachment 19454 [details]
Import test 77 from Acid3
LayoutTests/svg/custom/acid3-test-77.html | 13 ++++
.../svg/custom/resources/Acid3Font-loader.svg | 13 ++++
LayoutTests/svg/custom/resources/Acid3Font.svg | 12 ++++
LayoutTests/svg/custom/resources/acid3-test-77.js | 63 ++++++++++++++++++++
4 files changed, 101 insertions(+), 0 deletions(-)
*** Bug 17332 has been marked as a duplicate of this bug. *** The condition you want for an indexing error is: if (charcount <= charnum || nchars > charcount - charnum) where charcount is the unsigned number of characters in this. The <= changes to a < if text.getSubString(textLength, 0) is to return 0 and not throw, as I mildly think it should. (In reply to comment #11) > Created an attachment (id=19454) [edit] > Import test 77 from Acid3 > > LayoutTests/svg/custom/acid3-test-77.html | 13 ++++ > .../svg/custom/resources/Acid3Font-loader.svg | 13 ++++ > LayoutTests/svg/custom/resources/Acid3Font.svg | 12 ++++ > LayoutTests/svg/custom/resources/acid3-test-77.js | 63 ++++++++++++++++++++ > 4 files changed, 101 insertions(+), 0 deletions(-) > Heya, I'm also trying to fix the bugs right now. I've noticed that in resources/Acid3Font-loader.svg, that you're referencing "font.svg" instead of "Acid3Font.svg", hence that testcase gives wrong metrics, as the font wasn't loaded! Keeping you updated on the progress.... Greetings, Niko (In reply to comment #9) > (From update of attachment 19445 [details] [edit]) > Actually, I think this causes a regression in getComputedTextLength() since > after this patch we start failing there... which is earlier than > getSubStringLength in test order. > I'm not sure about that anymore - the testcase you uploaded never waits that the font is loaded, before quering the metrics. The testcases needs to be modelled just like frame-getSVGDocument.html (startTest, continueTest, completeTest) in order to actually work. I've done that and will upload a testcase soon. Created attachment 19498 [details]
Complete patch
Please check the setTimeout() part, I don't see another way atm.
Comment on attachment 19498 [details] Complete patch You'll need to include the test case that I added (which tests more negative value cases) before landing. I'm not certain this patch would pass all the cases in my test. see http://bugs.webkit.org/attachment.cgi?id=19445&action=prettypatch Created attachment 19509 [details]
Updated patch
Updated patch, integrated Eric's testcase, all PASS now including Acid3. Brings up score to 88/100.
(In reply to comment #13) > The condition you want for an indexing error is: > > if (charcount <= charnum || nchars > charcount - charnum) > > where charcount is the unsigned number of characters in this. The <= changes > to a < if text.getSubString(textLength, 0) is to return 0 and not throw, as I > mildly think it should. Thanks for the suggestion, Jeff. Incorporated in my latest patch. Created attachment 19511 [details]
Updated patch v2
Comment on attachment 19511 [details]
Updated patch v2
r=me
Comment on attachment 19511 [details]
Updated patch v2
As discussed yesterday, the code change is fine. The test is timing dependent (which sucks). If there is a way to fix the test, we should do that before landing. If there is not a way to make the test deterministic, then we should file a bug.
Landed in r30767. |