Bug 17078 - SVGTextElement.getSubStringLength does not work as expected (affects Acid3 test 77)
: SVGTextElement.getSubStringLength does not work as expected (affects Acid3 te...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Nikolas Zimmermann
:
Depends on:
Blocks: Acid3
  Show dependency treegraph
 
Reported: 2008-01-29 14:53 PST by Eric Seidel
Modified: 2008-03-04 15:23 PST (History)
2 users (show)

See Also:


Attachments
Patch that makes this test pass (991 bytes, patch)
2008-02-21 19:19 PST, Dave Hyatt
eric: review-
Details | Formatted Diff | Diff
Fix getSubStringLength range handling. (6.30 KB, patch)
2008-02-29 00:26 PST, Eric Seidel
eric: review-
Details | Formatted Diff | Diff
Import test 77 from Acid3 (5.15 KB, patch)
2008-02-29 11:36 PST, Eric Seidel
no flags Details | Formatted Diff | Diff
Complete patch (184.44 KB, patch)
2008-03-03 11:20 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated patch (16.53 KB, patch)
2008-03-03 17:25 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch v2 (17.63 KB, patch)
2008-03-03 17:39 PST, Nikolas Zimmermann
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2008-01-29 14:53:54 PST
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;
    },
Comment 1 Nikolas Zimmermann 2008-02-10 04:18:14 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. 
Comment 2 Dave Hyatt 2008-02-21 19:19:29 PST
Created attachment 19270 [details]
Patch that makes this test pass

Fix some minor bugs with substrings.
Comment 3 Oliver Hunt 2008-02-21 22:46:45 PST
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 4 Dave Hyatt 2008-02-21 23:18:42 PST
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 5 Eric Seidel 2008-02-22 00:36:22 PST
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 6 Eric Seidel 2008-02-22 00:37:00 PST
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.
Comment 7 Nikolas Zimmermann 2008-02-22 05:30:15 PST
(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
Comment 8 Eric Seidel 2008-02-29 00:26:38 PST
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 9 Eric Seidel 2008-02-29 00:39:30 PST
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.
Comment 10 Eric Seidel 2008-02-29 00:40:07 PST
(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. :)
Comment 11 Eric Seidel 2008-02-29 11:36:18 PST
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(-)
Comment 12 Eric Seidel 2008-02-29 14:17:09 PST
*** Bug 17332 has been marked as a duplicate of this bug. ***
Comment 13 Jeff Walden (remove +bwo to email) 2008-03-01 04:42:05 PST
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.
Comment 14 Nikolas Zimmermann 2008-03-02 03:47:05 PST
(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
Comment 15 Nikolas Zimmermann 2008-03-03 03:38:49 PST
(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.
Comment 16 Nikolas Zimmermann 2008-03-03 11:20:46 PST
Created attachment 19498 [details]
Complete patch

Please check the setTimeout() part, I don't see another way atm.
Comment 17 Eric Seidel 2008-03-03 15:42:44 PST
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
Comment 18 Nikolas Zimmermann 2008-03-03 17:25:27 PST
Created attachment 19509 [details]
Updated patch

Updated patch, integrated Eric's testcase, all PASS now including Acid3. Brings up score to 88/100.
Comment 19 Nikolas Zimmermann 2008-03-03 17:26:19 PST
(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.

Comment 20 Nikolas Zimmermann 2008-03-03 17:39:29 PST
Created attachment 19511 [details]
Updated patch v2
Comment 21 Dave Hyatt 2008-03-04 13:47:12 PST
Comment on attachment 19511 [details]
Updated patch v2

r=me
Comment 22 Eric Seidel 2008-03-04 13:51:04 PST
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.
Comment 23 Nikolas Zimmermann 2008-03-04 15:23:17 PST
Landed in r30767.