Closed Bug 739117 Opened 10 years ago Closed 6 years ago

additional space in text with arabic ligature

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: heycam, Assigned: jfkthame)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached file test (obsolete) —
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.
Attached image screen shot on mac
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.
Does this still reproduce?
Flags: needinfo?(cam)
It does.
Flags: needinfo?(cam)
Attached file 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)
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)
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.
Reftest that exhibits the bug here (on OS X; should pass harmlessly elsewhere).
Attachment #8697699 - Flags: review?(jdaggett)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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 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+
Attachment #8697699 - Flags: review?(jdaggett) → review+
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/1246e8a495ead61d9aeae2c999d1340127a9f33e
Backed out changeset a7c9fc3f1701 (bug 739117) for unexpected Core Text crashes.
(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......
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
https://hg.mozilla.org/mozilla-central/rev/7b567b63d13c
https://hg.mozilla.org/mozilla-central/rev/e75e82f7f068
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Duplicate of this bug: 1250409
You need to log in before you can comment on or make changes to this bug.