983985 - Italic text is not displayed correctly if the text contains hexbox and if the italic style is synthesized

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Set the default sans-serif font to MS PGothic and default family to sans-serif (it is the default of Japanese localization.)
2. Open http://glyphwiki.org/wiki/Special:Recentchanges?view=50&hidebots=1&hideauto=1&listtype=document&offset=1262040 and see the text "(resolved 

Comment 1

[Masatoshi Kimura [:emk]]

Oops, I forgot bugzilla dislikes non-BMP characters.

2. Open http://glyphwiki.org/wiki/Special:Recentchanges?view=50&hidebots=1&hideauto=1&listtype=document&offset=1262040 and see the text "(resolved [0F7AB4] vs [0F7823])" and "(resolved [0F7AB0] vs [0F781C])"
(Actually [0F7xxx] is a hexbox)

Actual result:
Text display is incorrect.

Expected result:
Text display should be correct.

If I set the default sans-serif font to Meiryo, it displayed correctly (Meiryo has an italic variant).

Comment 2

[Jonathan Kew [:jfkthame]]

Could you please attach a screenshot of the problem? Thanks.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: with MS PGothic

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: with Meiryo

Comment 5

[Masatoshi Kimura [:emk]]

Although I took this capture on a 192dpi monitor, it is also reproducible on a normal 96dpi monitor.

Comment 7

[Jonathan Kew [:jfkthame]]

Also reported on OS X, see bug 1060749.  => platform:all.

Marking as a regression, as I believe this didn't happen until we enabled Azure content.

Comment 8

[Jonathan Kew [:jfkthame]]

Reduced testcase that shows the bug on OS X, using an unassigned codepoint to try and ensure we get a hexbox even if extra fonts are installed:

  data:text/html,<span style="font: italic 24px monaco">Note: `&%23x0530;` is U+0530

The Monaco font is specified because it does *not* have a separate italic face, and so a synthetic-italic skew transform will be applied. That, together with rendering a hexbox, appears to be what triggers the bug.

Comment 9

[Jonathan Kew [:jfkthame]]

The main problem here occurs because gfxFontMissingGlyphs::DrawMissingGlyph calls

    aContext->Save();
    ...
    aContext->Restore();

on the gfxContext, and this (even with no code in between) modifies the matrix of the DrawTarget. This interferes with the font code's use of a skew matrix on the DrawTarget to implement fake-italics for fonts that don't have a real italic face.

I don't really know anything about how the interaction between gfxContext and DrawTarget is supposed to work, but it seems intuitively wrong that a simple Save/Restore pair should have this side-effect. I'll file a separate Graphics bug about this.

In any case, however, I think we want to remove the skew transform from the DrawTarget when rendering missing-glyph hexboxes, as they will not stand up to skewing well (and we don't skew them when they occur within a run of true italic text). We can do this by saving, modifying and restoring the matrix around the call to DrawMissingGlyph in the case where we're using a fake-italic skew for the current run.

Comment 10

[Jonathan Kew [:jfkthame]]

Filed bug 1060762 for the gfxContext::Save/Restore problem. Marking it as blocking this to indicate the relationship, as it is the immediate cause of the garbled rendering, although I'm actually intending to work around the issue here by removing the fake-italic skew before calling DrawMissingGlyph.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: remove fake-italic skew from the drawTarget's matrix before rendering a hexbox.

This appears to resolve the problem on OS X; not yet tested on Windows. I still think the behavior of Save/Restore is broken (see separate bug), but with this patch, at least it no longer breaks text rendering in this situation.

Comment 12

[Jonathan Kew [:jfkthame]]

Try builds with this patch: https://tbpl.mozilla.org/?tree=Try&rev=130b10075785. Kimura-san, could you confirm whether this fixes the problems you were seeing on Windows? Thanks.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Try builds with this patch:
> https://tbpl.mozilla.org/?tree=Try&rev=130b10075785. Kimura-san, could you
> confirm whether this fixes the problems you were seeing on Windows? Thanks.

Yes, this patch fixed the problems for me.

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 8481776 [details] [diff] [review]
remove fake-italic skew from the drawTarget's matrix before rendering a hexbox.

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

Makes sense to me.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: reftests for hexbox in fake-italic text.

Here are a couple of reftests that would have detected the problem here.

Comment 16

[Florian Bender]

Awesome! Patch looks innocuous enough so once this lands & sticks, this can be uplifted to 33, I think. I guess the issue does not warrant uplifting for a possible 32.0.1 (or 32.0RC2 if there'll be one) or 31ESR … would be nice, though.

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> Try builds with this patch:
> https://tbpl.mozilla.org/?tree=Try&rev=130b10075785. Kimura-san, could you
> confirm whether this fixes the problems you were seeing on Windows? Thanks.

Fixes the issue from Bug 1060749 for me as well!

Comment 17

[Jonathan Kew [:jfkthame]]

Landed the patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e542dad3c1

Reftests not yet landed, pending review.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8481776 [details] [diff] [review]
remove fake-italic skew from the drawTarget's matrix before rendering a hexbox.

Requesting uplift to Aurora (mozilla-33), but if this isn't processed before the next merge, then please consider this as a request for Beta instead.

Approval Request Comment
[Feature/regressing bug #]:
Azure content rendering (enabled by a variety of separate bugs on different platforms).

[User impact if declined]:
Garbled text in certain (fairly rare) cases: when a synthetic-italic font is used and the text contains unsupported characters.

[Describe test coverage new/current, TBPL]:
Patch has landed safely on inbound, passes all current text-rendering tests.
Reftests specific to this issue checked locally and on tryserver, will land when reviewed.

[Risks and why]: 
Low risk - only affects the missing-character hexbox codepath; restores the transform it has modified immediately after drawing.

[String/UUID change made/needed]:
n/a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68e542dad3c1

Comment 20

[Jonathan Kew [:jfkthame]]

Rebased and pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b5b25981de3d

Comment 21

[Jonathan Kew [:jfkthame]]

Bas: review ping? (reftest patch)

Comment 22

[Florian Bender]

(In reply to Jonathan Kew (:jfkthame) from comment #21)
> Bas: review ping? (reftest patch)

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 8481793 [details] [diff] [review]
reftests for hexbox in fake-italic text.

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

Sorry for missing this!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3be186980023

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3be186980023