Closed Bug 1108177 Opened 5 years ago Closed 5 years ago

Mark positioning in Arabic broken in Firefox 34 with some fonts

Categories

(Core :: Graphics: Text, defect)

Product:
Core
Component:
Graphics: Text
Version:
34 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: mozilla, Assigned: jfkthame)

References

Details

Attachments

(4 files)

Screenshot from 2014-12-15 09:59:21.png
92.74 KB, image/png
Details
Implement harfbuzz glyph-extents callback, so that fallback mark positioning works in legacy truetype fonts
13.65 KB, patch
jtd
: review+
Details | Diff | Splinter Review
Reftest to check for fallback mark stacking
2.83 KB, patch
jtd
: review+
Details | Diff | Splinter Review
screenshot, patch vs. chrome vs. safari with different fonts
185.62 KB, image/png
Details 
Not sure what the characteristics are.  Or if it's new in Firefox 34.  But on my Linux box, this page is very broken:

http://jsfiddle.net/ikopylov/0yeavgmk/

It works fine in Chrome.
No issue here, what version of Times New Roman do you have? I have 6.83 here.
Component: General → Graphics: Text
Product: Firefox → Core
Version: 17 Branch → 34 Branch
Humm.  I'm looking at version 3.0 if I understand corrrectly.  That said, HarfBuzz renders it correctly.  My version has no GPOS, so the question is, if Firefox is indeed using the font I think it's using, why doesn't the interal HarfBuzz mark stacking not working for me.
(In reply to Behdad Esfahbod from comment #2)
> Humm.  I'm looking at version 3.0 if I understand corrrectly.  That said,
> HarfBuzz renders it correctly.  My version has no GPOS, so the question is,
> if Firefox is indeed using the font I think it's using, why doesn't the
> interal HarfBuzz mark stacking not working for me.

Could you post a screenshot to show the brokenness you're seeing, please?
Flags: needinfo?(mozilla)
Attached.
Flags: needinfo?(mozilla)
The issue is that harfbuzz's fallback positioning doesn't work in Firefox, because we don't provide a get_glyph_extents callback. We should fix that.
Ah, good to know.  Yes, please fix.
Having implemented basic loca/glyf-loading to support proper vertical metrics, we can refactor this slightly and also provide a glyph-extents callback, so that harfbuzz's fallback mark positioning works properly. This fixes the rendering of Behdad's example here, and similar cases with old (GPOS-less) fonts.
Attachment #8553023 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
And here's a simple reftest to check that it's working.
Attachment #8553028 - Flags: review?(jdaggett)
Comment on attachment 8553023 [details] [diff] [review]
Implement harfbuzz glyph-extents callback, so that fallback mark positioning works in legacy truetype fonts

So this codepath only gets hit for marks from fonts lacking a GPOS table? So it's not really important to worry about how to deal with this issue for CFF fonts, the assumption being that there's a properly constructed GPOS table?
(In reply to John Daggett (:jtd) from comment #9)
> Comment on attachment 8553023 [details] [diff] [review]
> Implement harfbuzz glyph-extents callback, so that fallback mark positioning
> works in legacy truetype fonts
> 
> So this codepath only gets hit for marks from fonts lacking a GPOS table? So

Correct.

> it's not really important to worry about how to deal with this issue for CFF
> fonts, the assumption being that there's a properly constructed GPOS table?

We can even argue that CFF fonts must have GPOS.  This is mostly for TrueType-but-not-OpenType fonts.
Comment on attachment 8553028 [details] [diff] [review]
Reftest to check for fallback mark stacking

>+++ b/layout/reftests/text/fallback-mark-stacking-1-notref.html
>@@ -0,0 +1,23 @@

>+<body>
>+These examples should NOT look the same:
>+<div class=test>x&#x303;&#x302; x&#x302;&#x303;</span>
>+</body>

This is kind of strange structure, was it intentional? Maybe </span> should be </div>?
From left to right, trunk with patch, Chrome 41.0.2272.3 dev, Safari 6.2.2, all running on OSX.

Gentium - test font
Fira Sans - otf version 3.002
Constantia - TrueType Microsoft cfont
Candara - TrueType Microsoft cfont
Minion Pro - otf, Adobe standard serif
Cambria - TrueType Microsoft cfont
Calibri - TrueType Microsoft cfont

Fira Sans and Minion Pro lack support for U+302, U+303 but all the Microsoft cfont's do. Not sure why Constantia and Candara fail here.
(In reply to John Daggett (:jtd) from comment #11)
> Comment on attachment 8553028 [details] [diff] [review]
> Reftest to check for fallback mark stacking
> 
> >+++ b/layout/reftests/text/fallback-mark-stacking-1-notref.html
> >@@ -0,0 +1,23 @@
> 
> >+<body>
> >+These examples should NOT look the same:
> >+<div class=test>x&#x303;&#x302; x&#x302;&#x303;</span>
> >+</body>
> 
> This is kind of strange structure, was it intentional? Maybe </span> should
> be </div>?

Oops. Yes, indeed.
(In reply to John Daggett (:jtd) from comment #12)
> Created attachment 8554384 [details]
> screenshot, patch vs. chrome vs. safari with different fonts
> 
> From left to right, trunk with patch, Chrome 41.0.2272.3 dev, Safari 6.2.2,
> all running on OSX.
> 
> Gentium - test font
> Fira Sans - otf version 3.002
> Constantia - TrueType Microsoft cfont
> Candara - TrueType Microsoft cfont
> Minion Pro - otf, Adobe standard serif
> Cambria - TrueType Microsoft cfont
> Calibri - TrueType Microsoft cfont
> 
> Fira Sans and Minion Pro lack support for U+302, U+303 but all the Microsoft
> cfont's do. Not sure why Constantia and Candara fail here.

Because they have GPOS tables (and so the fallback positioning logic is not used) but don't include mark positioning features.
(In reply to Jonathan Kew (:jfkthame) from comment #14)

> Because they have GPOS tables (and so the fallback positioning logic is not
> used) but don't include mark positioning features.

Sort of unfortunate that we can deal with these fonts better. Safari displays marks correctly for both. But IE11 shows the same problem, so I guess we can deal with that later.
Comment on attachment 8553028 [details] [diff] [review]
Reftest to check for fallback mark stacking

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

r+ with s/span/div/
Attachment #8553028 - Flags: review?(jdaggett) → review+
Attachment #8553023 - Flags: review?(jdaggett) → review+
https://hg.mozilla.org/mozilla-central/rev/02601ab9f9af
https://hg.mozilla.org/mozilla-central/rev/0edb544e9ebc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 1127935
You need to log in before you can comment on or make changes to this bug.