Bug 178625 – REGRESSION(r222090): [HarfBuzz] Arabic shaping is broken except for first word in line

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Khaled Hosny]

In certain websites (e.g. https://fonts.google.com/?subset=arabic, http://www.linuxac.org/forum/threads/73644), Arabic shaping is broken except in the first word in each line; the Arabic letters are disjointed. It does not seem to affect all Arabic text, though.

This seems to be a regression as I only noticed it a few days ago.

[Michael Catanzaro]

With what version of WebKitGTK+? Trunk, 2.18.0, 2.18.1?

[Khaled Hosny]

(In reply to Michael Catanzaro from comment #1)
> With what version of WebKitGTK+? Trunk, 2.18.0, 2.18.1?

2.18.1

[Carlos Garcia Campos]

Created attachment 324676 [details]
Patch

This patch fixes the issue without breaking material design icons. I haven't run the tests yet, though.

[Carlos Garcia Campos]

Comment on attachment 324676 [details]
Patch

This broke several tests, I'll investigate it.

[Carlos Garcia Campos]

Created attachment 324979 [details]
Patch

[Michael Catanzaro]

*** Bug 178856 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Comment on attachment 324979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324979&action=review

> Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:153
> +        // FIXME: cover all other cases mentioned in the spec (ie. brackets or quotation marks).
> +        // https://bugs.webkit.org/show_bug.cgi?id=177003.

This is probably important.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> Comment on attachment 324979 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324979&action=review
> 
> > Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:153
> > +        // FIXME: cover all other cases mentioned in the spec (ie. brackets or quotation marks).
> > +        // https://bugs.webkit.org/show_bug.cgi?id=177003.
> 
> This is probably important.

Not that much, this fixes edge cases like for example 

text [text with different script] more text

same applies for << >> and { }. It can happen that because of inherit or common, we end up using a different font for the open and close brackets. I would need a test case to fix this, though.

[Carlos Garcia Campos]

Committed r224015: <https://trac.webkit.org/changeset/224015>

[Anass Ahmed]

This is the first time I notice that the first word is properly rendered, but the remaining text is garbled. :)

Waiting for the fix to reach Koji on Fedora to be able to test.

[Khaled Hosny]

Created attachment 324999 [details]
Test file for brackets handling

(In reply to Carlos Garcia Campos from comment #8)
> I would need a test case to fix this, though.

That is easy, in the attached HTML file the period should be rendered the same in both lines (you need the font from http://www.amirifont.org/), but currently the second line is different because the closing bracket takes the script of the Latin text before it and subsequently the period is rendered with the Latin script instead of Arabic.

It should be noted that both Firefox and Chrome do not seem to handle this, so it seems not to be a priority (LibreOffice does, but I wrote that code and it isn’t a web browser).

[Carlos Garcia Campos]

(In reply to Khaled Hosny from comment #11)
> Created attachment 324999 [details]
> Test file for brackets handling
> 
> (In reply to Carlos Garcia Campos from comment #8)
> > I would need a test case to fix this, though.
> 
> That is easy, in the attached HTML file the period should be rendered the
> same in both lines (you need the font from http://www.amirifont.org/), but
> currently the second line is different because the closing bracket takes the
> script of the Latin text before it and subsequently the period is rendered
> with the Latin script instead of Arabic.
> 
> It should be noted that both Firefox and Chrome do not seem to handle this,
> so it seems not to be a priority (LibreOffice does, but I wrote that code
> and it isn’t a web browser).

Thanks! Could you move the test case and your explanation to bug #177003, please? This bug is closed now, so I'm afraid I will forget about this.