Closed Bug 624359 Opened 9 years ago Closed 9 years ago

mozTextAlongPath should iterate by character clusters, not individual codepoints

Categories

(Core :: Canvas: 2D, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(4 files, 1 obsolete file)

This is a followup to bug 621918, which was originally about SVG <textPath> but also had some relevance to <canvas>.

When placing glyphs along a path using mozTextAlongPath, we should iterate by character clusters so that base+diacritic combinations and Indic syllables are not broken up. This is analogous to how letter-spacing works, for example.

The attached testcase places a number of letter+diacritic combinations along a path. On OS X, at least (behavior may vary depending on font support for the combinations used), the a+dieresis and e+dieresis combinations stay together, because the font has specific composite glyphs that are used for these, but the other letter+dieresis combinations break up when they occur on the peaks of the path, because the letter and dieresis glyphs are placed separately in relation to different positions along the path. We should be treating these grapheme clusters as indivisible units for the purpose of glyph placement here.
This shows the rendering of the above testcase on OS X 10.6. The expected result would be for the dieresis to remain directly above the base letter in all cases, not only for the vowels where the font happens to have a composite glyph.
This modifies the <canvas> mozTextAlongPath code to iterate by character clusters, so that we don't break up grapheme clusters when placing the glyphs.

A simple gfxTextRun::ClusterIterator class is used to support this; currently, this provides only the basic functionality needed by the <canvas> code, but we might want to enhance it further eventually if other textRun clients want to use it as well. (E.g. the SVG <textPath> code might be revised to use this instead of its own private iterator.)
Assignee: nobody → jfkthame
Attachment #502800 - Flags: review?(vladimir)
Reftesting this seems a bit tricky, but this works well for me locally, at least. Pushed to tryserver to check behavior there.
Attachment #502803 - Attachment is patch: true
Attachment #502803 - Attachment mime type: application/octet-stream → text/plain
Reftest for character clusters in mozTextAlongPath. This fails with current trunk code, because the diacritic gets placed separately from its base character; the patch makes us respect grapheme clusters when placing text on the path, and so the diacritic stays where it belongs.
Attachment #502803 - Attachment is obsolete: true
Attachment #503178 - Flags: review?(vladimir)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment on attachment 502800 [details] [diff] [review]
patch, v1 - make mozTextAlongPath iterate by clusters

Requesting approval-2.0 for this fix; this will ensure that we don't inappropriately break up grapheme clusters when displaying <canvas> text-along-path; it also makes the behavior more consistent with the SVG version. In addition to Latin-script clusters of base letter + diacritic, this is significant for Indic scripts where consonant + vowel clusters need to be kept together for proper rendering.

The patch should be low risk, as it affects only mozTextAlongPath, and does not change its behavior at all for "simple" text (without combining marks, Indic syllables & conjuncts, etc).
Attachment #502800 - Flags: approval2.0?
Comment on attachment 502800 [details] [diff] [review]
patch, v1 - make mozTextAlongPath iterate by clusters

I love the fact that we have tests, but I'd much rather this go into a 4.0.x release. If you feel strongly that this needs to be in 4.0.0, please renom.
Attachment #502800 - Flags: approval2.0? → approval2.0-
http://hg.mozilla.org/mozilla-central/rev/92bc57f478c1
http://hg.mozilla.org/mozilla-central/rev/8f679b9e32eb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.