590114 - hwid/twid/qwid not working for Hiragino Mincho Pro

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

Attachment: testcase, compare hwid/twid/qwid treatment for Kozuka vs. Hiragino

To enable the use of half-width, third-width, quarter-width numbers, Japanese fonts use the hwid/twid/qwid OpenType features.  But for some reason, harfbuzz doesn't seem to support these for Hiragino Mincho Pro but *does* support it for Kozuka Mincho Pro (Adobe).

Comment 1

[Jonathan Kew [:jfkthame]]

If you try running text that includes numbers, you'll find that the features do work as expected. They only fail in your testcase because the numbers are isolated in table cells.

The issue is that the Hiragino font doesn't have an entry for the "default" script in its script table, only specific scripts (CJK, of course, plus Latin, Cyrillic, Greek). But when the script itemizer gets a run of purely numerals and other "shared" characters, it can't assign a specific script code but returns the HB_SCRIPT_COMMON value, which would lead to the shaper using the default script table.

I think it'd probably be worth catching such runs and overriding the script code to Latin; this may not always be the best choice but it is impossible to be sure. If a font implemented significantly different features for the numerals, punctuation, etc depending on script, the only reliable way to access and control them all would be something like a -moz-font-script-override property - but I'm not keen to clutter things in that way for such edge cases.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1 - treat unresolved "common" script runs as Latin

Comment 3

[John Daggett (:jtd)]

>      PRUint32 runStart = 0, runLimit = aLength;
>      PRInt32 runScript = HB_SCRIPT_LATIN;
>      while (scriptRuns.Next(runStart, runLimit, runScript)) {
> +        if (scriptCode <= HB_SCRIPT_INHERITED) {
> +            // For unresolved "common" or "inherited" runs, default to Latin
> +            // for now.
> +            // (Should we somehow use the language or locale to try and infer
> +            // a better default?)
> +            scriptCode = HB_SCRIPT_LATIN;
> +        }
>          InitTextRun(aContext, aTextRun, aString, aLength,
>                      runStart, runLimit, runScript);
>      }

Hmmm, is this code in the right place?  I don't see a scriptCode
variable defined anywhere in gfxFont.cpp.  Did you mean runScript?

Comment 4

[Jonathan Kew [:jfkthame]]

Oops, silly error. Yes, I meant runScript - as the compiler just told me when I actually tried to build!

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1.1 - fixed patch to use the right variable name!

Comment 6

[John Daggett (:jtd)]

>      while (scriptRuns.Next(runStart, runLimit, runScript)) {
> +        if (runScript <= HB_SCRIPT_INHERITED) {
> +            // For unresolved "common" or "inherited" runs, default to Latin
> +            // for now.
> +            // (Should we somehow use the language or locale to try and infer
> +            // a better default?)
> +            runScript = HB_SCRIPT_LATIN;
> +        }

This seems reasonable but aren't combining diacritics classified as
"common"?  Would it make more sense to use the script code preceding the
"common" case so that font selection will occur the same way?

Comment 7

[Jonathan Kew [:jfkthame]]

Diacritics are "inherited" rather than "common", but the issue would be similar.

However, if there's anything "non-common/inherited" preceding (or following) in the textrun, the script itemizer will handle "inheriting" the actual script to the common characters so that they become part of the same script run. The problem only arises in cases (like yours) where the textrun contains ONLY such script-agnostic characters, so there's no useful context available.

Comment 8

[John Daggett (:jtd)]

Comment on attachment 479343 [details] [diff] [review]
patch, v1.1 - fixed patch to use the right variable name!

Looks fine then.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 479343 [details] [diff] [review]
patch, v1.1 - fixed patch to use the right variable name!

Requesting approval to land this on trunk for Gecko 2.0 once we open up after beta7. It's a well-contained, low-risk patch that gives us more consistent and predictable behavior for our new font feature support. Without this, users may be unable to activate features on text runs containing only "script-agnostic" characters such as digits/punctuation with some fonts.

Comment 10

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/d12bb59deb69

Comment 11

[Karl Tomlinson (:karlt)]

Could switching from default to latin disable some feature for languages with non-Latin scripts?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #9)
> Without this, users may
> be unable to activate features on text runs containing only "script-agnostic"
> characters such as digits/punctuation with some fonts.

Or is this implying that there is no such thing as a "default" script table entry?