Last component of Arabic ligature is sometimes duplicated

VERIFIED FIXED in Firefox 10

Status

()

Core
Layout: Text
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Khaled Hosny, Assigned: jfkthame)

Tracking

({regression})

Trunk
mozilla10
regression
Points:
---

Firefox Tracking Flags

(firefox9 affected, firefox10+ verified, firefox11+ verified)

Details

(Whiteboard: [qa!])

Attachments

(5 attachments)

(Reporter)

Description

6 years ago
Created attachment 584747 [details]
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.
(Reporter)

Updated

6 years ago
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
(Reporter)

Comment 3

6 years ago
FWIW, I can't reproduce this with upstream HarfBuzz master.
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 584973 [details] [diff] [review]
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.
Assignee: nobody → jfkthame
Attachment #584973 - Flags: review?(mozilla)

Comment 6

6 years ago
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).
(Assignee)

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
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+
(Assignee)

Comment 9

6 years ago
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.
status-firefox9: --- → affected
tracking-firefox10: --- → ?
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
(Assignee)

Comment 10

6 years ago
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?

Comment 11

6 years ago
Sorry I keep forgetting about the review flags here.
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/mozilla-central/rev/006ab8403475
Status: NEW → RESOLVED
Last Resolved: 6 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?
(Assignee)

Comment 15

6 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/35226ad21950
https://hg.mozilla.org/releases/mozilla-beta/rev/0316db6b614b
I'll leave setting of any flags in this bug as a result of those landings to whoever is more familiar with this bug.
(Assignee)

Comment 20

6 years ago
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.
status-firefox10: --- → fixed
status-firefox11: --- → fixed
tracking-firefox12: ? → ---
Target Milestone: mozilla12 → mozilla10

Updated

6 years ago
tracking-firefox10: ? → +
tracking-firefox11: ? → +

Comment 21

6 years ago
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

6 years ago
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)?
(Assignee)

Comment 23

6 years ago
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
Created attachment 599110 [details]
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.
Created attachment 599111 [details]
mac 10.7
Created attachment 599112 [details]
ubuntu 11.10
(Assignee)

Comment 28

5 years ago
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
status-firefox10: fixed → verified
status-firefox11: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.