739117 - additional space in text with arabic ligature

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: screen shot on mac

Comment 2

[Jonathan Kew [:jfkthame]]

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.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Does this still reproduce?

Comment 4

[Cameron McCormack (:heycam)]

It does.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: test (using Geeza Pro)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Paul, one more Mac-only bug to try hunting down :)

Comment 7

[Paul Oiegas [:poiegas], PM for (Ecosystem QA, Services QA, WebCompat QA, Hubs QA teams)]

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).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 9

[Jonathan Kew [: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.

Comment 10

[Jonathan Kew [: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.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for Arabic ligature in an AAT font

Reftest that exhibits the bug here (on OS X; should pass harmlessly elsewhere).

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 13

[John Daggett (:jtd)]

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

Comment 14

[Jonathan Kew [:jfkthame]]

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

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1246e8a495ead61d9aeae2c999d1340127a9f33e
Backed out changeset a7c9fc3f1701 (bug 739117) for unexpected Core Text crashes.

Comment 16

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99983442ee3371dcce892f9104397f623c03a596
Backed out changeset cad9316f0b50 (bug 739117) for R14 bustage on Android

Comment 17

[Jonathan Kew [:jfkthame]]

(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......

Comment 18

[Jonathan Kew [:jfkthame]]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b567b63d13c
https://hg.mozilla.org/mozilla-central/rev/e75e82f7f068