Last Comment Bug 714067 - Last component of Arabic ligature is sometimes duplicated
: Last component of Arabic ligature is sometimes duplicated
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
: 722696 (view as bug list)
Depends on:
Blocks: 701637
  Show dependency treegraph
 
Reported: 2011-12-29 07:43 PST by Khaled Hosny
Modified: 2012-02-23 07:42 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
verified
+
verified


Attachments
Test file showing the issue (138 bytes, text/html)
2011-12-29 07:43 PST, Khaled Hosny
no flags Details
patch, adjust ligature-composition in the light of changed loop indexing (1.20 KB, patch)
2011-12-30 09:11 PST, Jonathan Kew (:jfkthame)
jfkthame: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
win 7 (173.25 KB, image/jpeg)
2012-02-21 04:44 PST, Paul Silaghi, QA [:pauly]
no flags Details
mac 10.7 (337.53 KB, image/jpeg)
2012-02-21 04:44 PST, Paul Silaghi, QA [:pauly]
no flags Details
ubuntu 11.10 (96.99 KB, image/jpeg)
2012-02-21 04:45 PST, Paul Silaghi, QA [:pauly]
no flags Details

Description Khaled Hosny 2011-12-29 07:43:02 PST
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.
Comment 2 Simon Montagu :smontagu 2011-12-29 08:23:30 PST
... 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 2011-12-29 09:10:51 PST
FWIW, I can't reproduce this with upstream HarfBuzz master.
Comment 4 Jonathan Kew (:jfkthame) 2011-12-29 10:05:50 PST
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) 2011-12-30 09:11:50 PST
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.
Comment 6 Behdad Esfahbod 2012-01-14 15:56:41 PST
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) 2012-01-15 00:55:13 PST
Landed on inbound, taking comment #6 as r=behdad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/006ab8403475
Comment 8 Jonathan Kew (:jfkthame) 2012-01-15 00:57:05 PST
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) 2012-01-15 01:01:53 PST
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) 2012-01-15 05:07:20 PST
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 2012-01-15 10:34:45 PST
Sorry I keep forgetting about the review flags here.
Comment 12 Jonathan Kew (:jfkthame) 2012-01-16 04:33:32 PST
https://hg.mozilla.org/mozilla-central/rev/006ab8403475
Comment 13 Alex Keybl [:akeybl] 2012-01-17 12:39:14 PST
(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] 2012-01-17 14:42:05 PST
Additionally - is this bug meant to be marked as Linux only?
Comment 15 Jonathan Kew (:jfkthame) 2012-01-17 15:25:41 PST
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] 2012-01-17 16:47:53 PST
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) 2012-01-17 20:22:12 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/35226ad21950
Comment 18 Timothy Nikkel (:tnikkel) 2012-01-17 20:35:32 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/0316db6b614b
Comment 19 Timothy Nikkel (:tnikkel) 2012-01-17 20:36:42 PST
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) 2012-01-18 00:20:56 PST
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 2012-01-23 01:32:38 PST
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 2012-01-23 01:33:35 PST
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) 2012-01-23 03:02:53 PST
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 24 Simon Montagu :smontagu 2012-02-01 23:39:38 PST
*** Bug 722696 has been marked as a duplicate of this bug. ***
Comment 25 Paul Silaghi, QA [:pauly] 2012-02-21 04:44:09 PST
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.
Comment 26 Paul Silaghi, QA [:pauly] 2012-02-21 04:44:51 PST
Created attachment 599111 [details]
mac 10.7
Comment 27 Paul Silaghi, QA [:pauly] 2012-02-21 04:45:16 PST
Created attachment 599112 [details]
ubuntu 11.10
Comment 28 Jonathan Kew (:jfkthame) 2012-02-21 05:32:48 PST
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] 2012-02-23 07:42:33 PST
Based on comment 28, marking the bug as verified fixed.

Note You need to log in before you can comment on or make changes to this bug.