19128 2008-05-19 13:16:44 -0700 SVG fonts don't work with medial Arabic characters 2008-06-08 11:40:45 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 1 myrdred webkit-unassigned myrdred oldest_to_newest 80913 0 myrdred 2008-05-19 13:16:44 -0700 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="ښ" arabic-form="initial" d="M0 500L250 0L500 500"/> <glyph unicode="ښ" arabic-form="medial" d="M0 500L250 0L500 500"/> <glyph unicode="ښ" 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">ښښښ</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. 80915 1 21233 myrdred 2008-05-19 13:23:37 -0700 Created attachment 21233 Dirt-simple patch 80916 2 21233 eric 2008-05-19 13:27:32 -0700 Comment on attachment 21233 Dirt-simple patch Needs a test case. Otherwise this looks great! 80917 3 21233 eric 2008-05-19 13:29:22 -0700 Comment on attachment 21233 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.) 80918 4 myrdred 2008-05-19 13:40:56 -0700 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.) 80927 5 myrdred 2008-05-19 16:06:53 -0700 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. 80937 6 21243 myrdred 2008-05-19 18:27:46 -0700 Created attachment 21243 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. 82423 7 21243 eric 2008-06-06 08:21:31 -0700 Comment on attachment 21243 Updated patch Ok. r=me. 82622 8 darin 2008-06-08 11:34:25 -0700 Patch fails to compile as-is with a warning about signed/unsigned comparison. I'll fix it as I check it in. 82623 9 darin 2008-06-08 11:40:45 -0700 Committed revision 34445. 21233 2008-05-19 13:23:37 -0700 2008-05-19 18:28:19 -0700 Dirt-simple patch patch text/plain 1029 myrdred SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzMzU3OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDgtMDUtMTkgIEpvbmF0aGFuIEhhYXMgPG15cmRyZWRAZ21haWwu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IChUQlIpLgorCisgICAgICAgIGh0dHA6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5MTI4CisgICAgICAgIEJ1bXBlZCB1cCB0aGUg c2l6ZSBvZiBhIGJpdGZpZWxkIHRvIGZpeCBhbiBvdmVyZmxvdworCisgICAgICAgICogc3ZnL1NW R0dseXBoRWxlbWVudC5oOgorCiAyMDA4LTA1LTE5ICBBZGEgQ2hhbiAgPGFkYWNoYW5AYXBwbGUu Y29tPgogCiAgICAgICAgIEFkZGVkIENvb2tpZVN0b3JhZ2VXaW4uaC9jcHAsIGludHJvZHVjaW5n IG1ldGhvZHMgdG8gZ2V0L3NldCB0aGUgY3VycmVudCBDRkhUVFBDb29raWVTdG9yYWdlUmVmLgpJ bmRleDogV2ViQ29yZS9zdmcvU1ZHR2x5cGhFbGVtZW50LmgKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29y ZS9zdmcvU1ZHR2x5cGhFbGVtZW50LmgJKHJldmlzaW9uIDMzNTc4KQorKysgV2ViQ29yZS9zdmcv U1ZHR2x5cGhFbGVtZW50LmgJKHdvcmtpbmcgY29weSkKQEAgLTg2LDcgKzg2LDcgQEAgbmFtZXNw YWNlIFdlYkNvcmUgewogICAgICAgICBib29sIGlzVmFsaWQgOiAxOwogCiAgICAgICAgIE9yaWVu dGF0aW9uIG9yaWVudGF0aW9uIDogMjsKLSAgICAgICAgQXJhYmljRm9ybSBhcmFiaWNGb3JtIDog MzsKKyAgICAgICAgQXJhYmljRm9ybSBhcmFiaWNGb3JtIDogNDsKICAgICAgICAgaW50IHByaW9y aXR5OwogICAgICAgICBzaXplX3QgbmFtZUxlbmd0aDsKICAgICAgICAgU3RyaW5nIGdseXBoTmFt ZTsK 21243 2008-05-19 18:27:46 -0700 2008-06-06 08:21:31 -0700 Updated patch patch text/plain 1102 myrdred SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzMzU4MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDgtMDUtMTkgIEpvbmF0aGFuIEhhYXMgPG15cmRyZWRAZ21haWwu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IChUQlIpLgorCisgICAgICAgIGh0dHA6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5MTI4CisgICAgICAgIE1hZGUgYXJhYmljRm9y bSB1bnNpZ25lZCBzbyBpdCBmaXRzIGludG8gaXRzIDMtYml0IGJpdGZpZWxkCisKKyAgICAgICAg KiBzdmcvU1ZHR2x5cGhFbGVtZW50Lmg6CisKIDIwMDgtMDUtMTkgIEFkYSBDaGFuICA8YWRhY2hh bkBhcHBsZS5jb20+CiAKICAgICAgICAgQWRkZWQgQ29va2llU3RvcmFnZVdpbi5oL2NwcCwgaW50 cm9kdWNpbmcgbWV0aG9kcyB0byBnZXQvc2V0IHRoZSBjdXJyZW50IENGSFRUUENvb2tpZVN0b3Jh Z2VSZWYuCkluZGV4OiBXZWJDb3JlL3N2Zy9TVkdHbHlwaEVsZW1lbnQuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBXZWJDb3JlL3N2Zy9TVkdHbHlwaEVsZW1lbnQuaAkocmV2aXNpb24gMzM1ODApCisrKyBXZWJD b3JlL3N2Zy9TVkdHbHlwaEVsZW1lbnQuaAkod29ya2luZyBjb3B5KQpAQCAtODUsOCArODUsOCBA QCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKICAgICAgICAgYm9vbCBpc1ZhbGlkIDogMTsKIAotICAg ICAgICBPcmllbnRhdGlvbiBvcmllbnRhdGlvbiA6IDI7Ci0gICAgICAgIEFyYWJpY0Zvcm0gYXJh YmljRm9ybSA6IDM7CisgICAgICAgIHVuc2lnbmVkIG9yaWVudGF0aW9uIDogMjsgLy8gT3JpZW50 YXRpb24KKyAgICAgICAgdW5zaWduZWQgYXJhYmljRm9ybSA6IDM7ICAvLyBBcmFiaWNGb3JtCiAg ICAgICAgIGludCBwcmlvcml0eTsKICAgICAgICAgc2l6ZV90IG5hbWVMZW5ndGg7CiAgICAgICAg IFN0cmluZyBnbHlwaE5hbWU7Cg==