Closed
Bug 1458158
Opened 6 years ago
Closed 6 years ago
Bold font not rendering for Arabic characters when the style calls for Arial with font-weight:900
Categories
(Core :: Layout: Text and Fonts, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | verified |
People
(Reporter: over68, Assigned: jfkthame, NeedInfo)
References
Details
(Keywords: regression)
Attachments
(4 files)
3.39 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
2.44 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
390.04 KB,
image/png
|
Details | |
89.02 KB,
image/png
|
Details |
Bold font not rendering for Arabic characters when the style calls for Arial with font-weight:900. Testcase https://bugzilla.mozilla.org/attachment.cgi?id=598489
Flags: needinfo?(jfkthame)
Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=30ef2759d59356ab199342927eb6e450199cb024&tochange=2e9eda40f4770cc1fd2a2aa2263c1020445ef636 Regressed by: bug 1454598.
Blocks: 1454598
Keywords: regression
This similar to bug 728518.
Assignee | ||
Comment 3•6 years ago
|
||
What result do you get for the testcase? (Could you attach a screenshot, please.)
Flags: needinfo?(jfkthame) → needinfo?(over68)
Assignee | ||
Comment 4•6 years ago
|
||
Ah, never mind about a screenshot, I could reproduce this on a Win10 machine. So yes, the updated font code has regressed bug 728518. Will fix; thanks for the report!
Flags: needinfo?(over68)
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae1b1cf7a7da1e2b77d5b4a114bc5f2de8d906d9
Assignee | ||
Comment 6•6 years ago
|
||
Here's a reftest form of the testcase; this currently fails on trunk due to the bug here.
Attachment #8972303 -
Flags: review?(jwatt)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 7•6 years ago
|
||
Fortunately the fix is simple: in the update to CalcStyleMatch, I inadvertently reversed the sense of the weight factor, so that it prefers further weights instead of closer ones. Fixing that restores the desired behavior. (Actually, I think as a followup, we should consider replacing CalcStyleMatch entirely, and instead relying on the same WeightStyleStretch distance function as the main font selection code. But let's deal with that separately, as it could change behavior -- for the better, I think -- as opposed to simply fixing the regression here.)
Attachment #8972304 -
Flags: review?(jwatt)
Updated•6 years ago
|
Severity: normal → critical
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
status-firefox-esr60:
--- → unaffected
tracking-firefox61:
--- → +
Priority: -- → P1
Comment 8•6 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #7) > (Actually, I think as a followup, we should consider replacing > CalcStyleMatch entirely, and instead relying on the same WeightStyleStretch > distance function as the main font selection code. But let's deal with that > separately, as it could change behavior -- for the better, I think -- as > opposed to simply fixing the regression here.) Can you file a bug for that and mention the bug number here?
Comment 9•6 years ago
|
||
Comment on attachment 8972303 [details] [diff] [review] Testcase for rendering Arabic characters when styled with an extra-bold weight of Arial Review of attachment 8972303 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/font-matching/1458158-1-ref.html @@ +14,5 @@ > + h1 { > + font-family: arial_bold; > + } > + b { > + font-family: arial_black, arial_bold; The arial_bold here can go, right?
Attachment #8972303 -
Flags: review?(jwatt) → review+
Updated•6 years ago
|
Attachment #8972304 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #9) > Comment on attachment 8972303 [details] [diff] [review] > Testcase for rendering Arabic characters when styled with an extra-bold > weight of Arial > > Review of attachment 8972303 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/reftests/font-matching/1458158-1-ref.html > @@ +14,5 @@ > > + h1 { > > + font-family: arial_bold; > > + } > > + b { > > + font-family: arial_black, arial_bold; > > The arial_bold here can go, right? No, it's there because Arial Black does not actually include any Arabic characters - it has a reduced character set. And so at font-weight:900, although we initially choose the Black face, we want them to fall back to Arial Bold (and not to Regular, as they did thanks to this bug, or to something else entirely). So it's explicitly here to provide a controlled fallback for those characters.
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #8) > (In reply to Jonathan Kew (:jfkthame) from comment #7) > > (Actually, I think as a followup, we should consider replacing > > CalcStyleMatch entirely, and instead relying on the same WeightStyleStretch > > distance function as the main font selection code. But let's deal with that > > separately, as it could change behavior -- for the better, I think -- as > > opposed to simply fixing the regression here.) > > Can you file a bug for that and mention the bug number here? Filed as bug 1458301.
Comment 12•6 years ago
|
||
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/74b436c29aa1 Testcase for rendering Arabic characters when styled with an extra-bold weight of Arial. r=jwatt https://hg.mozilla.org/integration/mozilla-inbound/rev/7faed86e2815 Correct the weight-difference factor in CalcStyleMatch to favor closer weights, not further ones. r=jwatt
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74b436c29aa1 https://hg.mozilla.org/mozilla-central/rev/7faed86e2815
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 14•6 years ago
|
||
Thanks for landing an automated test for this. Is this something we should have QE do some extra exploratory testing around or are you reasonably confident that the new test will catch any issues?
Flags: qe-verify?
Flags: needinfo?(jfkthame)
Flags: in-testsuite+
Comment 15•6 years ago
|
||
Actually, I'm going to say that the answer to that question is yes since the new test only runs on Windows.
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 16•6 years ago
|
||
Additional testing wouldn't hurt, but the issue is one that we've only ever seen on Windows; it arises because of an oddity of the particular collection of Arial font faces that Windows ships, and the way those are organized by the DirectWrite font subsystem. In theory a similar issue could occur on other platforms, but we've never seen an actual case AFAIK. (It would require creating a deliberately anomalous font family with multiple weights and differing character repertoires across the weights.)
Comment 17•6 years ago
|
||
I reproduced this issue on Windows 10 x64, Windows 7 x64 and Mac OS X 10.12 using Nightly 61.0a1(2018-04-30). I could not reproduce it on Ubuntu 17.04 x64 using Nightly 61.0a1(2018-04-30) and the latest Nightly 61.0a1(2018-05-03). The issue is verified fixed on Windows 10 x64, Windows 7 x64 and Mac OS X 10.12a using the latest Nightly 61.0a1(2018-05-03). Also, I tried to reproduce the initial issue on Windows 10 x64 and Windows 7 with an affected Nightly 61.0a1 build (2018-04-30) using several websites with Arabic characters and Arial bold fonts, without luck. Is there any testing that I should do in order to verify this bug?
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 18•6 years ago
|
||
The problem occurs in affected builds when Arabic characters are used with Arial weight 900 (extra-bold/black), not with the normal Bold face. So for example if a site has a heading <h1> which is bold by default, and then uses <b> within the heading, the resolved font-weight will be 900 and the bug will occur. The older report bug 728518 mentioned in comment 2 had a link to https://www.adslgate.com/dsl/showpost.php?p=1063716613&postcount=1 where the issue occurs: the Arabic text of the blue heading should appear in Arial Bold, but this bug caused it to appear in Arial (regular) instead. Verifying with the attached testcase should be OK, however.
Flags: needinfo?(jfkthame)
Comment 19•6 years ago
|
||
I could not reproduce the bug using the test case from comment 18 on Windows 10 x64, Mac OS X 10.12 and Ubuntu 17.04 x64 with an older Nightly 61.0a1 (2018-04-30) and with the latest Nightly 61.0a1 (2018-05-06). Am I missing something?
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 20•6 years ago
|
||
Your screenshot (comment 19) does actually show the issue, but it's difficult to see at such a small font size. If we put the two lines of text directly above each other, as here, it becomes clearer: the top line (from Nightly 04-30) does not use the same font face for the Arabic script as the lower line (from 05-06). What we're seeing here is that the bottom line correctly uses Arial Bold for the Arabic script text, but the upper line is (I think) using the Arial Regular face with a synthetic-bold effect. The difference is particularly visible on the two- and three-dot combinations above some of the letters such as in the right-most word شركة.
Flags: needinfo?(jfkthame)
Comment 21•6 years ago
|
||
Based on comment 17 and comment 20 I will mark this bug as Verified fixed.
Comment hidden (spam) |
Comment hidden (spam) |
You need to log in
before you can comment on or make changes to this bug.
Description
•