Bug 19128 – SVG fonts don't work with medial Arabic characters

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

[Jonathan Haas]

To repro, go to LayoutTests/svg/W3C-SVG-1.1/fonts-glyphs-02-t.svg. Note big ugly blank rectangles where none should exist.

Simplified repro case is:

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="http://www.w3.org/2000/svg" >
    <defs>
      <font horiz-adv-x="500">
        <font-face font-family="SVGFont" />
        <missing-glyph d="M0 500L250 750L500 500"/>
        <glyph unicode="&#x069A;" arabic-form="initial" d="M0 500L250 0L500 500"/>
        <glyph unicode="&#x069A;" arabic-form="medial" d="M0 500L250 0L500 500"/>
        <glyph unicode="&#x069A;" arabic-form="terminal" d="M0 500L250 0L500 500"/>
      </font>
    </defs>
    <g font-family="SVGFont" font-size="80">
      <text font-face="SVGFont" font-size="80" x="100" y="100">&#x069A;&#x069A;&#x069A;</text>
    </g>
</svg>

This should output three downward-pointing triangles (one for each character in the <text> element) but it outputs two downward triangles with an upward triangle between them. Glyphs with arabic-form "medial" don't work. Here's the problem.

SVGGlyphElement.h:

    struct SVGGlyphIdentifier {
...
        // SVG Font depends on exactly this order.
        enum ArabicForm {
            None = 0,
            Isolated,
            Terminal,
            Initial,
            Medial
        };
...
        ArabicForm arabicForm : 3;

ArabicForm has values 0 to 4 inclusive, so it looks like a 3-bit bitfield should be more than sufficient. But enumerations are signed types, so the bitfield is also signed, so rather than ranging from 0..7, it ranges from -4..3. You can verify this:



void main() {
  enum foo { ZERO, ONE, TWO, THREE, FOUR };
  struct bar { foo f : 3; } b;

  b.f = FOUR;
  cout << "FOUR = " << FOUR << endl;
  cout << "b.f = " << b.f << endl;
  cout << "FOUR == b.f = " << (FOUR==b.f) << endl;
}

...which outputs:

FOUR = 4
b.f = -4
FOUR == b.f = 0

The simplest solution is to bump the size of the bitfield up by one. This should have zero impact on performance, since the next data member should be byte-aligned (at least) anyway.

Sure would be nice if the compiler warned of the potential integer overflow.

[Jonathan Haas]

Created attachment 21233 [details]
Dirt-simple patch

[Eric Seidel (no email)]

Comment on attachment 21233 [details]
Dirt-simple patch

Needs a test case.  Otherwise this looks great!

[Eric Seidel (no email)]

Comment on attachment 21233 [details]
Dirt-simple patch

Oh, and actually, this is the wrong fix.  There is a way to force unsigned enums.  We have to do that for windows compilers.  there is an rtl flag on RenderStyle which we had to do this for iirc.  (Random Trivia: When the very very first versions of WebKit ran on windows the first time all the text was RTL due to this bug.)

[Jonathan Haas]

Huh... learn something new every day. There is a way to force an enum to be unsigned, and the syntax is:

enum foo : unsigned
{
  one,
  two, 
  three
};

I had no idea. Will update patch. Will also start thinking of a test case, but the current SVG tree dumper doesn't dump anything wrong with the way things are, so it'll require changes (and hence changes to a lot of expected output.)

[Jonathan Haas]

Alas, while enum something : unsigned { } is kind of neat, it appears to be a nonstandard language extension. Other places in the code just use unsigned as the bitfield type, guess that's what I'll do here.

[Jonathan Haas]

Created attachment 21243 [details]
Updated patch

Rather than increasing the size of the bitfield, declared it as unsigned instead of ArabicForm. This matches how other enumerations in WebKit are placed in bitfields.

As for a testcase, well, you can verify the bug's existence and fix manually by visiting LayoutTests/svg/W3C-SVG-1.1/fonts-glyph-03-t.svg. A testcase that fails automated tests without the patch would be a lot tougher... the SVG render tree dumper doesn't dump nearly enough information to cause a test case to fail, and would need to be changed substantially to make it do so.

[Eric Seidel (no email)]

Comment on attachment 21243 [details]
Updated patch

Ok.  r=me.

[Darin Adler]

Patch fails to compile as-is with a warning about signed/unsigned comparison. I'll fix it as I check it in.

[Darin Adler]

Committed revision 34445.