Last Comment Bug 680692 - GPOS contextual chaining positioning does not work
: GPOS contextual chaining positioning does not work
Status: VERIFIED FIXED
[qa!]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla10
Assigned To: Khaled Hosny
:
Mentors:
Depends on: 695857
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-20 12:41 PDT by Khaled Hosny
Modified: 2011-12-01 14:33 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
-
fixed


Attachments
Backported upstream patch (2.62 KB, patch)
2011-08-20 12:41 PDT, Khaled Hosny
jfkthame: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Test file (187.12 KB, text/html)
2011-08-20 12:44 PDT, Khaled Hosny
no flags Details
Current rendering (FireFox 6) (7.92 KB, image/png)
2011-08-20 12:47 PDT, Khaled Hosny
no flags Details
After applying the patch (mozilla-centeral) (7.52 KB, image/png)
2011-08-20 12:48 PDT, Khaled Hosny
no flags Details

Description Khaled Hosny 2011-08-20 12:41:52 PDT
Created attachment 554656 [details] [diff] [review]
Backported upstream patch

Due to a bug in HarfBuzz, contextual chaining positioning are broken and do not get applied it all. It have been fixed in HarfBuzz master (http://cgit.freedesktop.org/harfbuzz/commit/?id=cc1a8a938b4c13e76b58825a9e1951c4134e634a). The attached patch is the original upstream commit applied to mozilla-centeral, I built and tested it and it seem to work as expected.
Comment 1 Khaled Hosny 2011-08-20 12:44:46 PDT
Created attachment 554658 [details]
Test file

This is a test file showing the issue with embedded woff font (the font is OFL licensed, base64 encoded just for convenience).
Comment 2 Khaled Hosny 2011-08-20 12:47:12 PDT
Created attachment 554659 [details]
Current rendering (FireFox 6)
Comment 3 Khaled Hosny 2011-08-20 12:48:09 PDT
Created attachment 554660 [details]
After applying the patch (mozilla-centeral)
Comment 4 Jonathan Kew (:jfkthame) 2011-08-24 04:17:23 PDT
I'd really prefer to take a full harfbuzz update soon, but if we don't get that done in good time before the next Aurora train leaves, we should consider cherry-picking this fix.
Comment 5 Jonathan Kew (:jfkthame) 2011-11-05 04:51:08 PDT
Comment on attachment 554656 [details] [diff] [review]
Backported upstream patch

As bug 695857 (harfbuzz update) is still awaiting review, I think we should take this fix now as a stop-gap. Hence r=me.
Comment 6 Jonathan Kew (:jfkthame) 2011-11-05 04:53:35 PDT
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0355183e7170
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 01:40:00 PDT
Do we need this in Firefox 9?
Comment 8 Ed Morley [:emorley] 2011-11-06 05:39:04 PST
https://hg.mozilla.org/mozilla-central/rev/0355183e7170
Comment 9 Jonathan Kew (:jfkthame) 2011-11-06 12:34:42 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Do we need this in Firefox 9?

Well... we've been shipping the buggy behavior for several versions now, but it would be nice to fix it sooner rather than later. In particular, I believe (based on email correspondence with the font developer concerned) that this affects a new Arabic webfont that the BBC Arabic site is about to deploy. So until we fix it, that's a pretty major site that will look bad in Firefox. There are presumably other affected fonts/sites as well, though I don't know how widespread the issue is.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-06 14:21:04 PST
Comment on attachment 554656 [details] [diff] [review]
Backported upstream patch

Review of attachment 554656 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should do this.

This is a fairly simple patch that will fix rendering on a major site.
Comment 11 Alex Keybl [:akeybl] 2011-11-07 10:48:02 PST
Comment on attachment 554656 [details] [diff] [review]
Backported upstream patch

[Triage Comment]
* Approving for aurora given the affect to BBC's rollout
* Denying for beta since this wouldn't cause a re-roll

Please make sure to land this today to make the cutover.
Comment 13 Paul Silaghi, QA [:pauly] 2011-11-22 07:03:33 PST
The test file https://bug680692.bugzilla.mozilla.org/attachment.cgi?id=554658 is displayed like in the attachment https://bug680692.bugzilla.mozilla.org/attachment.cgi?id=554660 on:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111121 Firefox/11.0a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111121 Firefox/10.0a2

Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; rv:11.0a1) Gecko/20111121 Firefox/11.0a1
Mozilla/5.0 (Windows NT 6.1; rv:10.0a2) Gecko/20111121 Firefox/10.0a2

Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111121 Firefox/11.0a1
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111121 Firefox/10.0a2

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111121 Firefox/11.0a1

Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111121 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111122 Firefox/11.0a1

Is that correct ? Can I change the resolution to verified fixed ?
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-22 07:57:21 PST
I believe that is correct. Khaled, please reopen this bug if it is not working the way you intended.

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