If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

additional space in text with arabic ligature

RESOLVED FIXED in Firefox 46

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: heycam, Assigned: jfkthame)

Tracking

({regression})

Trunk
mozilla46
x86
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Created attachment 609180 [details]
test

On my machine (a Mac) an ALEF LAM ligature is rendered with too much space.  It also seems that the gfxTextRun is reporting true for IsLigatureGroupStart and IsClusterStart for all of the characters, which doesn't seem right.  I tried this with a couple of different Arabic fonts.
Created attachment 609181 [details]
screen shot on mac
(Assignee)

Comment 2

6 years ago
Hmm.... this only seems to occur when the ligature occurs before a direction-run boundary; and only when using an AAT font (such as Geeza Pro), not an OpenType font (such as Times New Roman).

Your testcase didn't exhibit the problem for me at first; I had to explicitly select the Geeza Pro font to make it happen.

This is presumably a regression, but from quite a while ago.... the problem is present in FF10 as well as current trunk.
Keywords: regressionwindow-wanted
Does this still reproduce?
Flags: needinfo?(cam)
It does.
Flags: needinfo?(cam)
Created attachment 8675460 [details]
test (using Geeza Pro)
Attachment #609180 - Attachment is obsolete: true
Paul, one more Mac-only bug to try hunting down :)
Flags: needinfo?(paul.oiegas)
Hi Ryan,

So I dug, and after I dug some more :) Found that this fossil issue started to appear somewhere between FF 3.0 and 4.0. Tried to use mozregression but I got some errors that I think are related to the builds that I wanted to test. I had to download and test Minefield builds. So I applied the same sorting algorithm manually as mozregression and found out that the issue first appeared in 2009-09-27 Nightly build for Mac OS.
Strangely if I used mozregression after to point at the exact day the build was downloaded and I could get the push log (http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-09-26&enddate=2009-09-27).
Flags: needinfo?(paul.oiegas)
Thanks for digging, Paul! That said, I don't think that pushlog range is quite right. Nothing stands out in the range you gave that looks like an obvious culprit. So I downloaded the nightly builds and grabbed the SourceStamp from application.ini to get the exact revs they were built from.

That gave me a range of:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=149c3820e8a8&tochange=61620a8558c4

Conveniently, that yields a very good-looking candidate: http://hg.mozilla.org/mozilla-central/rev/086b132ab779 (Bug 517877 - Enable Core Text text back-end by default). That said, I'm not sure what if anything can be done here in that case. Any ideas, Jonathan?
Blocks: 517877
Flags: needinfo?(jfkthame)
Keywords: regressionwindow-wanted → regression
(Assignee)

Comment 9

2 years ago
I guess we just need to debug gfxCoreTextShaper.cpp and figure out where it's getting things wrong. (FWIW, I notice that if I select the excess space in the testcase, and copy/paste it somewhere, what actually gets copied is the ALEF that is part of the ligature.)

It's pretty clear that we're doing something wrong in the case where an RTL text fragment ends with a ligature, and in theory we should be able to fix it. I think we must be setting up the character/glyph associations incorrectly somehow.

I'm afraid it's not at the top of my priority stack at the moment, though, given the limited scope of the bug (only Mac OS X, only AAT fonts, only when a ligature occurs at the end of an RTL text fragment).

cc'ing jdaggett, in case he has time to look into this.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 10

2 years ago
I have a patch that I think fixes this, but it depends on a new Core Text API that is not supported until OS X 10.8. In theory, though, we should be able to check at runtime and use the new codepath on current systems, so that the bug will be fixed there and only continue to affect older OS versions.
(Assignee)

Comment 11

2 years ago
Created attachment 8697699 [details] [diff] [review]
Reftest for Arabic ligature in an AAT font

Reftest that exhibits the bug here (on OS X; should pass harmlessly elsewhere).
Attachment #8697699 - Flags: review?(jdaggett)
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 12

2 years ago
Created attachment 8697700 [details] [diff] [review]
Avoid bidi-wrapping the text to be shaped if Core Text direction override API is available

On 10.8+, we can avoid the extra bidi controls we currently wrap around the text, which fixes the spacing issue here.
Attachment #8697700 - Flags: review?(jdaggett)

Comment 13

2 years ago
Comment on attachment 8697700 [details] [diff] [review]
Avoid bidi-wrapping the text to be shaped if Core Text direction override API is available

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

r+ with trailing space trim

::: gfx/thebes/gfxCoreTextShaper.cpp
@@ +32,5 @@
> +};
> +
> +// Helper to create a CFDictionary with the right attributes for shaping our
> +// text, including imposing the given directionality.
> +// This will only be called if we're on 10.8 or later. 

Trailing space
Attachment #8697700 - Flags: review?(jdaggett) → review+

Updated

2 years ago
Attachment #8697699 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad9316f0b509191a91b076912a66053935e71c3
Bug 739117 - Reftest for Arabic ligature in an AAT font. r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c9fc3f1701afdbf5d0a8d951e2c66db43b2025
Bug 739117 - Avoid bidi-wrapping the text to be shaped if Core Text direction override API is available. r=jdaggett
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1246e8a495ead61d9aeae2c999d1340127a9f33e
Backed out changeset a7c9fc3f1701 (bug 739117) for unexpected Core Text crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/99983442ee3371dcce892f9104397f623c03a596
Backed out changeset cad9316f0b50 (bug 739117) for R14 bustage on Android
(Assignee)

Comment 17

2 years ago
(In reply to Nigel Babu [:nigelb] from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 99983442ee3371dcce892f9104397f623c03a596
> Backed out changeset cad9316f0b50 (bug 739117) for R14 bustage on Android

That's bizarre; the failure there was in layout/reftests/svg/svg-integration/clipPath-html-06.xhtml and layout/reftests/svg/svg-integration/clipPath-html-06-extref.xhtml, which has no relation at all to the new test landed that here. And there was no actual code change whatsoever.

The only "connection" I can think of is that adding the new test (in an entirely different directory) shifted the Android reftest chunk boundaries by one testcase, and somehow the different chunk contents triggered the failure on these two clipPath tests (even though they're in the middle of a chunk).

I notice that these tests are already annotated as fuzzy-if(Android&&AndroidVersion==18,255,30), and the failure that occurred here has that same pixel difference. So I propose to simply adjust the annotation there (removing the version dependency).

Before re-landing, though, I also need to figure out the 10.10-specific Core Text crashes in the actual patch......
(Assignee)

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b567b63d13ccef2c20a7cd5820b1143acc5fde3
Bug 739117 - Reftest for Arabic ligature in an AAT font. r=jdaggett

https://hg.mozilla.org/integration/mozilla-inbound/rev/e75e82f7f0686bc67e4f059d54974fb67d6a940d
Bug 739117 - Avoid bidi-wrapping the text to be shaped if Core Text direction override API is available. r=jdaggett

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7b567b63d13c
https://hg.mozilla.org/mozilla-central/rev/e75e82f7f068
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1250409
You need to log in before you can comment on or make changes to this bug.