1248248 - Variation selector doesn't work in certain vertical mode

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Momdo Nakamura]

Attachment: a.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160210153822

Steps to reproduce:

This report is written with reference to the original blog post (in Japanese):
http://we.fnshr.info/2016/02/14/variation-selector/

See attached file for more information about HTML fragments.


Actual results:

Variation selectors don't work in vertical mode (text-orientation `mixed` - it is initial value).


Expected results:

Change each kanji glyph which has variation selector under any text-orientation value in vertical mode.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Don't break glyph run for a variation selector

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8719281 [details] [diff] [review]
Don't break glyph run for a variation selector

Review of attachment 8719281 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Probably worth adding a reftest.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8719281 [details] [diff] [review]
Don't break glyph run for a variation selector

Review of attachment 8719281 [details] [diff] [review]:
-----------------------------------------------------------------

Wait... Should we ever break the glyph run inside a graphme cluster? I suppose not but am not sure.

It seems to me variation selectors are just a single case where we should not break. UAX 29 defines the general rules. In those rules, the one most likely violated here is "Do not break before extending characters". Variation selectors here are included in the "extending characters". However, the range of that concept covers some more characters. [1]

I'm not saying this is the wrong way to fix this issue. I guess it is not necessary to have a completely conforming impl as far as no difference can be observed. But we probably should have some more investigation, and we probably should add comment here to make this situation clearer, and how it could potentially lead to other similiar issues.


[1] http://www.unicode.org/reports/tr29/#Extend

Comment 4

[Jonathan Kew [:jfkthame]]

For the most part, grapheme clusters should be handled at the font-matching stage (we don't want to mix fonts within a cluster); though our support in that area is less than perfect, and bug 543200 is intended to make things better.

Glyph orientation in vertical text is kind of another level on top of that, where we may need to split the glyph run even if the font is the same. Checking UTR#50, it does say that "it does not make sense to use a base character upright and a combining mark attached to it sideways. Implementations should apply the orientation to each grapheme cluster."[1] Which suggests that for combining marks in general, we should ignore the fact that they're listed as "R" in VerticalOrientation.txt, and leave them to adopt the orientation of their base.

In practice, there aren't a lot of combining marks that get applied to upright CJK characters, so the issue will rarely matter; variation selectors are probably the likeliest example of "grapheme extenders" that will actually occur. But you're right, it would be better to extend the check here. We can just use !IsClusterExtender() (from unichar-utils) instead of !IsVarSelector() to do this, I think.

[1] http://unicode.org/reports/tr50/#grapheme_clusters

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Don't break glyph run for orientation mismatch before a cluster-extender

This should be a better fix here (though we still don't address the remark in UTR50 about always making a cluster with an enclosing mark be upright; leaving that for a potential followup).

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for variation selectors in vertical text with mixed (default) orientation

Test that fails with current code; should pass after the patch here.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Don't break glyph run for orientation mismatch before a cluster-extender

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8719287 [details] [diff] [review]
Don't break glyph run for orientation mismatch before a cluster-extender

This seems to be obsolete.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #8)
> Comment on attachment 8719287 [details] [diff] [review]
> Don't break glyph run for orientation mismatch before a cluster-extender
> 
> This seems to be obsolete.

Interesting; I'm virtually sure bzexport used to automatically obsolete the previous version of a patch when attaching a new one, but apparently that didn't happen this time.

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9615b2293d4ac0704004c03587d1f175545b7502
Bug 1248248 - Reftest for variation selectors in vertical text with mixed (default) orientation. r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/891c83f794121721ca124053e7ea83423b11282a
Bug 1248248 - Don't break glyph run for orientation mismatch before a cluster-extender. r=xidorn

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9615b2293d4a
https://hg.mozilla.org/mozilla-central/rev/891c83f79412

Comment 12

[Brendan Dahl [:bdahl]]

It looks like this test and several other CJK font reftests are flaky on android. I'm doing work over in bug 1235869 which is quite unrelated, but I'm now getting perma-oranges on font-face/ivs-1.html, font-face/cjkcisvs-1.html, and the newly added writing-mode/1248248-1-orientation-break-glyphrun.html. There was previously discussion on the test switching from failing to passing on android (for unexplained reasons) in https://bugzilla.mozilla.org/show_bug.cgi?id=552460#c40. Also, locally font-face/ivs-1.html always fails for me in the simulator even without my changes applied. However, if I manually look at the test pages in the simulator they are correct.

Should I disable these tests for android and open a new bug?

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Brendan Dahl [:bdahl] from comment #12)
> Also, locally
> font-face/ivs-1.html always fails for me in the simulator even without my
> changes applied. However, if I manually look at the test pages in the
> simulator they are correct.
>
> Should I disable these tests for android and open a new bug?

We should fix Android tests so that it matches with the manual browsing result. Otherwise we will not notice when the IVS support is silently broken.

Comment 14

[Jonathan Kew [:jfkthame]]

The first thing to do would be to look at the logs for failing runs and see what kind of failure is actually happening. Are we failing to load the font, and getting a fallback? Rendering nothing at all? AFAICS, this was never answered back in https://bugzilla.mozilla.org/show_bug.cgi?id=552460#c43....