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
: WebKit
SVG
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 17064
  Show dependency treegraph
 
Reported: 2008-01-29 14:53 PST by
Modified: 2008-03-04 15:23 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2008-02-21 19:19:29 PST -------
Created an attachment (id=19270) [details]
Patch that makes this test pass

Fix some minor bugs with substrings.
------- Comment #3 From 2008-02-21 22:46:45 PST -------
(From update of attachment 19270 [details])
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 From 2008-02-21 23:18:42 PST -------
(From update of attachment 19270 [details])
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 From 2008-02-22 00:36:22 PST -------
(From update of attachment 19270 [details])
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 From 2008-02-22 00:37:00 PST -------
(From update of attachment 19270 [details])
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 From 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 From 2008-02-29 00:26:38 PST -------
Created an attachment (id=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 From 2008-02-29 00:39:30 PST -------
(From update of attachment 19445 [details])
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 From 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 From 2008-02-29 11:36:18 PST -------
Created an attachment (id=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 From 2008-02-29 14:17:09 PST -------
*** Bug 17332 has been marked as a duplicate of this bug. ***
------- Comment #13 From 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 From 2008-03-02 03:47:05 PST -------
(In reply to comment #11)
> Created an attachment (id=19454) [edit] [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(-)
> 

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 From 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 From 2008-03-03 11:20:46 PST -------
Created an attachment (id=19498) [details]
Complete patch

Please check the setTimeout() part, I don't see another way atm.
------- Comment #17 From 2008-03-03 15:42:44 PST -------
(From update of attachment 19498 [details])
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 From 2008-03-03 17:25:27 PST -------
Created an attachment (id=19509) [details]
Updated patch

Updated patch, integrated Eric's testcase, all PASS now including Acid3. Brings up score to 88/100.
------- Comment #19 From 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 From 2008-03-03 17:39:29 PST -------
Created an attachment (id=19511) [details]
Updated patch v2
------- Comment #21 From 2008-03-04 13:47:12 PST -------
(From update of attachment 19511 [details])
r=me
------- Comment #22 From 2008-03-04 13:51:04 PST -------
(From update of attachment 19511 [details])
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 From 2008-03-04 15:23:17 PST -------
Landed in r30767.