615445 - gfxUniscribeShaper should treat halfwidth Katakana sound marks as extending a cluster

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #614468 +++

U+FF84 HALFWIDTH KATAKANA VOICED SOUND MARK and
U+FF9E HALFWIDTH KATAKANA SEMI-VOICED SOUND MARK are listed in
http://www.unicode.org/Public/6.0.0/ucd/auxiliary/GraphemeBreakProperty.txt
with property Extend, and so should join the cluster of the previous
character.
http://www.unicode.org/reports/tr29/tr29-17.html#Default_Grapheme_Cluster_Table

(The normal width Katakana-Hiragana sound marks have independent combining and
non-combining forms U+3029 - U+309C.)

Bug 614468 covers treatment of these characters in gfxPlatform::SetupClusterBoundaries, but gfxUniscribeShaper overwrites the break and joins created there.

Comment 1

[Jonathan Kew [:jfkthame]]

(In reply to comment #0)
> Bug 614468 covers treatment of these characters in
> gfxPlatform::SetupClusterBoundaries, but gfxUniscribeShaper overwrites the
> break and joins created there.

Once gfxPlatform::SetupClusterBoundaries does a better job, I'd like to eliminate that aspect of gfxUniscribeShaper altogether rather than re-do it.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, don't wipe out cluster information when setting glyphs in uniscribe shaper

The problem here is that the Uniscribe shaper overwrites the cluster information that gfxPlatform::SetupClusterBoundaries has already set in the textrun. Modifying SaveGlyphs() so that it preserves the state of the cluster flag should fix the issue.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: patch v2, don't wipe out cluster information when setting glyphs in Uniscribe and GDI shapers

The same issue applies to the GDI shaper, which we use as a fallback with fonts that Uniscribe doesn't like, so we should fix both of them.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 496871 [details] [diff] [review]
patch v2, don't wipe out cluster information when setting glyphs in Uniscribe and GDI shapers

>+            aTextRun->IsClusterStart(offset))

>+                                g.SetComplex(aTextRun->IsClusterStart(offset),

    PRBool atClusterStart = aRun->IsClusterStart(offset);

would reduce the number of IsClusterStart call points in this loop from 2 to 1.

>+++ b/gfx/thebes/gfxUniscribeShaper.cpp

>+                    aRun->IsClusterStart(runOffset))

>+                                    g.SetComplex(aRun->IsClusterStart(runOffset),

and 

    PRBool atClusterStart = aRun->IsClusterStart(runOffset);

would reduce the number of IsClusterStart call points in this loop from 3 to 1.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to comment #4)
>     PRBool atClusterStart = aRun->IsClusterStart(offset);
> 
> would reduce the number of IsClusterStart call points in this loop from 2 to 1.

>     PRBool atClusterStart = aRun->IsClusterStart(runOffset);
> 
> would reduce the number of IsClusterStart call points in this loop from 3 to 1.

So it would. I'll do that, thanks.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 496871 [details] [diff] [review]
patch v2, don't wipe out cluster information when setting glyphs in Uniscribe and GDI shapers

Requesting approval-2.0; this fixes character-cluster errors in first-letter and cursor/editing behavior, and makes our behavior with Uniscribe and GDI shaping more consistent with other platforms/back-ends.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: patch v3 (for checkin) - don't overwrite cluster info when setting glyphs

Updated for Karl's review comments, ready for checkin.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 499106 [details] [diff] [review]
patch v3 (for checkin) - don't overwrite cluster info when setting glyphs

Please add a test if possible.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to comment #8)
> Please add a test if possible.

A couple of the first-letter reftests (329069-2.html and 329069-3.html) already test this; they've been failing on WinXP, but I'll be able to mark them as passing when this lands.

Comment 10

[Jonathan Kew [:jfkthame]]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/beca3b777fc5 (patch)
http://hg.mozilla.org/mozilla-central/rev/53dc4a2c24ef (mark the reftests that now pass)