Summary: | REGRESSION(r222090): [HarfBuzz] Arabic shaping is broken except for first word in line | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Khaled Hosny <dr.khaled.hosny> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | anass.1430, bugs-noreply, cgarcia, mcatanzaro | ||||||||
Priority: | P3 | Keywords: | Gtk | ||||||||
Version: | Other | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugzilla.redhat.com/show_bug.cgi?id=1506590 | ||||||||||
Bug Depends on: | 178788 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Khaled Hosny
2017-10-21 13:03:24 PDT
With what version of WebKitGTK+? Trunk, 2.18.0, 2.18.1? (In reply to Michael Catanzaro from comment #1) > With what version of WebKitGTK+? Trunk, 2.18.0, 2.18.1? 2.18.1 Created attachment 324676 [details]
Patch
This patch fixes the issue without breaking material design icons. I haven't run the tests yet, though.
Comment on attachment 324676 [details]
Patch
This broke several tests, I'll investigate it.
Created attachment 324979 [details]
Patch
*** Bug 178856 has been marked as a duplicate of this bug. *** 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. (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. Committed r224015: <https://trac.webkit.org/changeset/224015> 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. 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). (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. |