714067 - Last component of Arabic ligature is sometimes duplicated

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Khaled Hosny]

Attachment: Test file showing the issue

If there is a combining mark on one of ligature components (except the last) the non-ligated form of the last component would be rendered after the ligature and the combining mark would not be rendered.

This can be seen with e.g. لا ligature (almost any Arabic font).

The attached file shows the issue (Arabic Typesetting is specified to show it is reproducible with any ligature).

This seems to be a regression from 8 which didn't suffer from this.

Comment 1

[Simon Montagu :smontagu]

Regression range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c8147998124&tochange=de483d897af4

Maybe bug 701637?

Comment 2

[Simon Montagu :smontagu]

... by which I mean http://hg.mozilla.org/mozilla-central/rev/de483d897af4 -- the bug number in the checkin comment seems to be something else

Comment 3

[Khaled Hosny]

FWIW, I can't reproduce this with upstream HarfBuzz master.

Comment 4

[Jonathan Kew [:jfkthame]]

Yes, almost certainly a regression from bug 701637; I'll try to look into it. Though in view of comment 3, perhaps taking a complete harfbuzz update (as per bug 695857, awaiting review....) would resolve the problem.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch, adjust ligature-composition in the light of changed loop indexing

This is definitely a regression from bug 701637. The restructuring of the first loop in Ligature::apply means that we now exit that loop with j one less than previously, and so a corresponding adjustment is needed in the condition that checks whether any marks were present.

The problem doesn't occur with current harfbuzz trunk because the patch from bug 701637 hasn't been applied there yet. So Behdad, I think it's time to take that patch, but be sure to include this correction as well.

Comment 6

[Behdad Esfahbod]

The logic looks right to me.  May make sense to make "i" local to the loop now.  (here and in other places.  but as a separate change).

Comment 7

[Jonathan Kew [:jfkthame]]

Landed on inbound, taking comment #6 as r=behdad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/006ab8403475

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 584973 [details] [diff] [review]
patch, adjust ligature-composition in the light of changed loop indexing

Just FTR, setting r+ flag on the basis of Behdad's comment.

Comment 9

[Jonathan Kew [:jfkthame]]

Nominating for tracking-firefox10 and following versions: this is a regression in FF9 that results in incorrect rendering of ligatures (e.g. Arabic, but would affect other languages too) where combining marks (diacritics) are present.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 584973 [details] [diff] [review]
patch, adjust ligature-composition in the light of changed loop indexing

[Approval Request Comment]
Regression caused by (bug #): 701637

User impact if declined:
Incorrect rendering (garbled text) in cases where a diacritic occurs within a ligature; noted in Arabic-script examples but could also affect other scripts, depending on font support for ligatures and presence of diacritics.

Testing completed (on m-c, etc.):
Confirmed that the testcase renders correctly with the patch, and checked by tracing that the if/else branches here are being taken in the expected situations.

Risk to taking this patch (and alternatives if risky):
No significant risk - the change simply corrects the condition for taking a "no-diacritics-were-present" shortcut in the code, which is currently taken in the wrong situation and leads to the wrong glyphs being displayed. (The alternative fix for the regression would be to back out the loop-indexing changes from bug 701637, but that would reintroduce a potential crash due to an out-of-bounds glyph buffer access.)

Comment 11

[Behdad Esfahbod]

Sorry I keep forgetting about the review flags here.

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/006ab8403475

Comment 13

[Alex Keybl [:akeybl]]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> User impact if declined:
> Incorrect rendering (garbled text) in cases where a diacritic occurs within
> a ligature; noted in Arabic-script examples but could also affect other
> scripts, depending on font support for ligatures and presence of diacritics.

Hi Jonathan - before making a decision on whether to take this patch, a backout of bug 701637, or do nothing at all for FF10, could you clarify whether the rendered text is unreadable and how common this incorrect rendering is?

Comment 14

[Alex Keybl [:akeybl]]

Additionally - is this bug meant to be marked as Linux only?

Comment 15

[Jonathan Kew [:jfkthame]]

No, this affects all platforms, it just got noticed/filed first on Linux.

The incorrectly-rendered text is "garbled" in that where the bug occurs, an extra and contextually-inappropriate letterform appears in the word (instead of an intended diacritic) - imagine English text where there@s an occasio&nal unexpected character th%rown in to the se?quence of let!ters to get some sense of how it would seem. Users would generally be able to figure out what the text was meant to say, I think, but it would be glaringly obvious that something's wrong with it, and readers would probably hesitate and stumble over the glitches.

I'd guess that the problem is not particularly common, mainly because Arabic (where the issue was noted, although it could affect other scripts) is often written without marking the vowel diacritics. However, in text genres (such as some educational materials, religious texts, and perhaps other specialized cases) where all the vowels are written, it would be fairly common.

IMO, the choice for FF10 should be between taking this patch (which I believe to be very safe) or doing nothing, and allowing the regression to persist until FF11 (though I think that'd be regrettable); backing out bug 701637 and risking the reappearance of the crashes there would be a worse situation.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 584973 [details] [diff] [review]
patch, adjust ligature-composition in the light of changed loop indexing

[Triage Comment]
Given the affect on Arabic readability, approving for aurora and beta 5.

Comment 17

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35226ad21950

Comment 18

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0316db6b614b

Comment 19

[Timothy Nikkel (:tnikkel)]

I'll leave setting of any flags in this bug as a result of those landings to whoever is more familiar with this bug.

Comment 20

[Jonathan Kew [:jfkthame]]

Timothy, many thanks for pushing this (while I was asleep!).

Marking as Fixed for FF10 and FF11, as a result of the beta and aurora landings.

Comment 21

[Abdellah Chelli]

I confirm that this bug affect firefox mobile (firefox/9.0 fennec/9.0).
Does firefox share same source with fennec, so if it is fixed in firefox10 then it is so for firefox mobile (fennec/10.0)?

Comment 22

[Abdellah Chelli]

I confirm that this bug affects firefox mobile (firefox/9.0 fennec/9.0).
Does firefox share same source with fennec, so if it is fixed in firefox10 then it is so for firefox mobile (fennec/10.0)?

Comment 23

[Jonathan Kew [:jfkthame]]

Yes, the same code is used on both mobile and desktop. (You should be able to check that it's fixed using current Beta, Aurora or Nightly versions.)

Comment 25

[Paul Silaghi, QA [:pauly]]

Attachment: win 7

I verified this on Firefox 10.0.2 and 11b3 on Win 7/64, Ubuntu 11.10 and Mac OS X 10.7. I got different results on each OS. Please see the screen shots attached.

Comment 26

[Paul Silaghi, QA [:pauly]]

Attachment: mac 10.7

Comment 27

[Paul Silaghi, QA [:pauly]]

Attachment: ubuntu 11.10

Comment 28

[Jonathan Kew [:jfkthame]]

You're getting different results because you've got quite different fonts on the various systems - in particular, the fonts being used on Ubuntu have poor support for the Arabic diacritics. However, none of them exhibit the bug here, they're all rendering as well as can be expected given the fonts involved.

Comment 29

[Paul Silaghi, QA [:pauly]]

Based on comment 28, marking the bug as verified fixed.