SVGTextElement.getComputedTextLength does not work as expected (Acid3 bug) Test 78: expected: 4776, got: 5550 - getComputedTextLength failed. I bet we're not respecting kerning or some advanced text feature as specified by the Acid3SVGFont in question. function () { // test 78: external SVG fonts, from Erik Dahlstrom // // SVGFonts are described here[3], and the relevant DOM methods // used in the test are defined here[4]. // // Note that in order to be more predictable the svg should be // visible, so that clause "For non-rendering environments, the // user agent shall make reasonable assumptions about glyph // metrics." doesn't influence the results. We use 'opacity:0' // to hide the SVG, but arguably it's still a "rendering // environment". // // The font-size 4000 was chosen because that matches the // unitsPerEm value in the svgfont, which makes it easy to check // the glyph advances since they will then be exactly what was // specified in the svgfont. // // [3] http://www.w3.org/TR/SVG11/fonts.html // [4] http://www.w3.org/TR/SVG11/text.html#InterfaceSVGTextContentElement var svgns = "http://www.w3.org/2000/svg"; var xlinkns = "http://www.w3.org/1999/xlink"; var svgdoc = kungFuDeathGrip.firstChild.contentDocument; assert(svgdoc, "contentDocument failed on <object> for svg document."); var svg = svgdoc.documentElement; var text = svgdoc.createElementNS(svgns, "text"); text.setAttribute("y", "1em"); text.setAttribute("font-size", "4000"); text.setAttribute("font-family", "ACID3svgfont"); var textContent = svgdoc.createTextNode("abc"); text.appendChild(textContent); svg.appendChild(text); // The font-size 4000 was chosen because that matches the unitsPerEm value in the svgfont, // which makes it easy to check the glyph advances since they will then be exactly what was specified in the svgfont. assert(text.getNumberOfChars, "SVGTextContentElement.getNumberOfChars() not supported."); assertEquals(text.getNumberOfChars(), 3, "getNumberOfChars returned incorrect string length."); assertEquals(text.getComputedTextLength(), 4711+42+23, "getComputedTextLength failed."); assertEquals(text.getSubStringLength(0,1), 42, "getSubStringLength #1 failed."); assertEquals(text.getSubStringLength(0,2), 42+23, "getSubStringLength #2 failed."); assertEquals(text.getSubStringLength(1,1), 23, "getSubStringLength #3 failed."); assertEquals(text.getSubStringLength(1,0), 0, "getSubStringLength #4 failed."); var code = -1000; try { var sl = text.getSubStringLength(1,3); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getSubStringLength #1 didn't throw exception."); code = -1000; try { var sl = text.getSubStringLength(0,3); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getSubStringLength #2 didn't throw exception."); code = -1000; try { var sl = text.getSubStringLength(1,2); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getSubStringLength #3 didn't throw exception."); code = -1000; try { var sl = text.getSubStringLength(-17,20); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getSubStringLength #4 didn't throw exception."); code = -1000; try { var sl = text.getSubStringLength(1,2); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getSubStringLength #5 didn't throw exception."); assertEquals(text.getStartPositionOfChar(0).x, 0, "getStartPositionOfChar(0).x returned invalid value."); assertEquals(text.getStartPositionOfChar(1).x, 42, "getStartPositionOfChar(1).x returned invalid value."); assertEquals(text.getStartPositionOfChar(2).x, 42+23, "getStartPositionOfChar(2).x returned invalid value."); assertEquals(text.getStartPositionOfChar(0).y, 4000, "getStartPositionOfChar(0).y returned invalid value."); code = -1000; try { var val = text.getStartPositionOfChar(-1); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getStartPositionOfChar #1 exception failed."); code = -1000; try { var val = text.getStartPositionOfChar(4); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getStartPositionOfChar #2 exception failed."); assertEquals(text.getEndPositionOfChar(0).x, 42, "getEndPositionOfChar(0).x returned invalid value."); assertEquals(text.getEndPositionOfChar(1).x, 42+23, "getEndPositionOfChar(1).x returned invalid value."); assertEquals(text.getEndPositionOfChar(2).x, 42+23+4711, "getEndPositionOfChar(2).x returned invalid value."); code = -1000; try { var val = text.getEndPositionOfChar(-17); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getEndPositionOfChar #1 exception failed."); code = -1000; try { var val = text.getEndPositionOfChar(4); } catch(e) { code = e.code; } assertEquals(code, DOMException.INDEX_SIZE_ERR, "getEndPositionOfChar #2 exception failed."); return 5; },
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.