Closed Bug 615445 Opened 9 years ago Closed 9 years ago

gfxUniscribeShaper should treat halfwidth Katakana sound marks as extending a cluster

Categories

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

x86_64
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: karlt, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
(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.
Blocks: 617869
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.
Assignee: nobody → jfkthame
Attachment #496786 - Flags: review?(karlt)
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.
Attachment #496786 - Attachment is obsolete: true
Attachment #496871 - Flags: review?(karlt)
Attachment #496786 - Flags: review?(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.
Attachment #496871 - Flags: review?(karlt) → review+
Blocks: 618870
(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 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.
Attachment #496871 - Flags: approval2.0?
Updated for Karl's review comments, ready for checkin.
Attachment #496871 - Attachment is obsolete: true
Attachment #499106 - Flags: approval2.0?
Attachment #496871 - Flags: approval2.0?
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.
Attachment #499106 - Flags: approval2.0? → approval2.0+
(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.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/beca3b777fc5 (patch)
http://hg.mozilla.org/mozilla-central/rev/53dc4a2c24ef (mark the reftests that now pass)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.