Bug 17078 – SVGTextElement.getSubStringLength does not work as expected (affects Acid3 test 77)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

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;
    },

[Nikolas Zimmermann]

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. 

[Dave Hyatt]

Created attachment 19270 [details]
Patch that makes this test pass

Fix some minor bugs with substrings.

[Oliver Hunt]

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

[Dave Hyatt]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Nikolas Zimmermann]

(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

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

(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. :)

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

*** Bug 17332 has been marked as a duplicate of this bug. ***

[Jeff Walden (remove +bwo to email)]

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.

[Nikolas Zimmermann]

(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

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Created attachment 19498 [details]
Complete patch

Please check the setTimeout() part, I don't see another way atm.

[Eric Seidel (no email)]

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

[Nikolas Zimmermann]

Created attachment 19509 [details]
Updated patch

Updated patch, integrated Eric's testcase, all PASS now including Acid3. Brings up score to 88/100.

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

Created attachment 19511 [details]
Updated patch v2

[Dave Hyatt]

Comment on attachment 19511 [details]
Updated patch v2

r=me

[Eric Seidel (no email)]

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.

[Nikolas Zimmermann]

Landed in r30767.