Last Comment Bug 769475 - incorrect font used for italicized Arabic text when font-family is Arial or Times New Roman
: incorrect font used for italicized Arabic text when font-family is Arial or T...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks: 764005
  Show dependency treegraph
 
Reported: 2012-06-28 15:17 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-11-03 09:51 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch, don't forget to check other styles of the first font in the group (3.28 KB, patch)
2012-06-28 15:17 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
patch v2, don't forget to check other styles of the first font in the group (5.36 KB, patch)
2012-07-03 07:02 PDT, Jonathan Kew (:jfkthame)
smontagu: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
reftest (2.84 KB, patch)
2012-07-03 07:03 PDT, Jonathan Kew (:jfkthame)
smontagu: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2012-06-28 15:17:59 PDT
Created attachment 637700 [details] [diff] [review]
patch, don't forget to check other styles of the first font in the group

The optimization of gfxFontGroup::FindFontForChar in bug 764005 regressed the behavior of italicized Arabic text in font families such as Times New Roman and Arial on Windows, where the regular face includes Arabic characters but the italic face does not.

This was previously fixed in bug 668813, but the optimization in 764005 bypassed that fix in the case where the "problem" family is the first in the font-group's list.

An example that shows the problem is attachment 594315 [details] (from bug 668813); on Windows, this should render using Arial with synthetic italicization (oblique), but in current Nightly it falls back to a different font such as Microsoft Uighur.

The patch here fixes the problem, while still allowing the vast majority of calls to take the optimized early-return path.
Comment 1 Jonathan Kew (:jfkthame) 2012-07-03 07:02:23 PDT
Created attachment 638699 [details] [diff] [review]
patch v2, don't forget to check other styles of the first font in the group

Here's a better version of the patch, factoring out a TryOtherFamilyMembers helper method.
Comment 2 Jonathan Kew (:jfkthame) 2012-07-03 07:03:07 PDT
Created attachment 638700 [details] [diff] [review]
reftest

And a reftest (for OS X and Windows) to go with it.
Comment 3 Simon Montagu :smontagu 2012-07-22 05:17:36 PDT
Comment on attachment 638699 [details] [diff] [review]
patch v2, don't forget to check other styles of the first font in the group

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

Thanks to bletch on IRC for drawing this to my attention
Comment 4 Jonathan Kew (:jfkthame) 2012-07-23 01:02:14 PDT
Oops, it looks like I failed to attach a name to the r? request - sorry! Thanks for picking this up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9be67e1a7b4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cd3d70434c
Comment 5 Ed Morley [:emorley] 2012-07-23 03:04:44 PDT
Sorry, had to back out for failures in arial-arabic.html on Windows:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=07cd3d70434c

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8425b8e1cd
Comment 6 Jonathan Kew (:jfkthame) 2012-07-23 03:45:35 PDT
Sigh - looks like a single-pixel line height discrepancy, for some reason - probably something about how we round metrics (there's an open bug on that, iirc).

I've added an explicit line-height to the test/reference, and pushed to tryserver to see if that makes it happy. https://tbpl.mozilla.org/?tree=Try&rev=dd2dba88c9fb
Comment 7 Jonathan Kew (:jfkthame) 2012-07-23 06:22:10 PDT
...and that tryserver job burned on windows, apparently due to bug 776503.

I've re-pushed a try job based on a slightly earlier inbound changeset, in hopes that might avoid the build issue and let us confirm that the new test passes. https://tbpl.mozilla.org/?tree=Try&rev=fcda56bb2f1c
Comment 8 Jonathan Kew (:jfkthame) 2012-07-24 00:28:19 PDT
Re-landed the patch (unchanged) and test (fixed).
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f86dfdcae06
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8987e78bad

According to https://tbpl.mozilla.org/?tree=Try&rev=ccf74222523c, this version passes on Windows (using an explicit line height in the test, to avoid issues with rounding metrics from the font).
Comment 10 Jonathan Kew (:jfkthame) 2012-08-16 01:27:19 PDT
Comment on attachment 638699 [details] [diff] [review]
patch v2, don't forget to check other styles of the first font in the group

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 764005

User impact if declined: incorrect font selection for italic-styled Arabic text when font family is Arial or Times New Roman - we fall back to a different font family

Testing completed (on m-c, etc.): patch has been in Nightly for several weeks; also has a reftest

Risk to taking this patch (and alternatives if risky): minimal risk - just reinstates the fix from bug 668813 that was improperly bypassed by one of the optimized codepaths in 764005

String or UUID changes made by this patch: none
Comment 11 Jonathan Kew (:jfkthame) 2012-08-21 03:53:32 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7b344615437

Note You need to log in before you can comment on or make changes to this bug.