Closed Bug 714067 Opened 8 years ago Closed 8 years ago

Last component of Arabic ligature is sometimes duplicated

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla10
Tracking Status
firefox9 --- affected
firefox10 + verified
firefox11 + verified

People

(Reporter: dr.khaled.hosny, Assigned: jfkthame)

References

Details

(Keywords: regression, Whiteboard: [qa!])

Attachments

(5 files)

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.
Attachment #584747 - Attachment mime type: text/plain → text/html
Regression range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c8147998124&tochange=de483d897af4

Maybe bug 701637?
Blocks: 701637
Status: UNCONFIRMED → NEW
Ever confirmed: true
... by which I mean http://hg.mozilla.org/mozilla-central/rev/de483d897af4 -- the bug number in the checkin comment seems to be something else
FWIW, I can't reproduce this with upstream HarfBuzz master.
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.
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.
Assignee: nobody → jfkthame
Attachment #584973 - Flags: review?(mozilla)
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).
Landed on inbound, taking comment #6 as r=behdad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/006ab8403475
Keywords: regression
Target Milestone: --- → mozilla12
Version: 9 Branch → Trunk
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.
Attachment #584973 - Flags: review?(mozilla) → review+
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 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.)
Attachment #584973 - Flags: approval-mozilla-beta?
Attachment #584973 - Flags: approval-mozilla-aurora?
Sorry I keep forgetting about the review flags here.
https://hg.mozilla.org/mozilla-central/rev/006ab8403475
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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?
Additionally - is this bug meant to be marked as Linux only?
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.
OS: Linux → All
Hardware: x86 → All
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.
Attachment #584973 - Flags: approval-mozilla-beta?
Attachment #584973 - Flags: approval-mozilla-beta+
Attachment #584973 - Flags: approval-mozilla-aurora?
Attachment #584973 - Flags: approval-mozilla-aurora+
I'll leave setting of any flags in this bug as a result of those landings to whoever is more familiar with this bug.
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.
Target Milestone: mozilla12 → mozilla10
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)?
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)?
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.)
Whiteboard: [qa+]
Duplicate of this bug: 722696
Attached image 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.
Attached image mac 10.7
Attached image ubuntu 11.10
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.
Based on comment 28, marking the bug as verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.