Bug 170399 - REGRESSION (r211382): Arabic text of non-system font cannot be justified
Summary: REGRESSION (r211382): Arabic text of non-system font cannot be justified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Safari 10
Hardware: iPhone / iPad iOS 10
: P2 Major
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 170586 170654 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-04-03 05:30 PDT by mohammedmadina
Modified: 2017-04-10 08:45 PDT (History)
9 users (show)

See Also:


Attachments
A blog post showing the justification bug in Arabic text. (498.02 KB, image/png)
2017-04-03 05:30 PDT, mohammedmadina
no flags Details
Patch (10.84 KB, patch)
2017-04-06 22:55 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (11.05 KB, patch)
2017-04-07 12:47 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mohammedmadina 2017-04-03 05:30:22 PDT
Created attachment 306073 [details]
A blog post showing the justification bug in Arabic text.

Arabic text of non-system fonts cannot be justified correctly after the iOS 10.3 update. The bug results in unreadable text in most websites that use outer fonts.
Comment 1 Radar WebKit Bug Importer 2017-04-04 16:58:15 PDT
<rdar://problem/31442008>
Comment 2 Myles C. Maxfield 2017-04-06 22:54:42 PDT
*** Bug 170586 has been marked as a duplicate of this bug. ***
Comment 3 Myles C. Maxfield 2017-04-06 22:55:35 PDT
Created attachment 306470 [details]
Patch
Comment 4 Simon Fraser (smfr) 2017-04-06 23:11:28 PDT
Comment on attachment 306470 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        When we perform justification, we adjust glyphs' advances to add extra space between words.
> +        ComplexTextController maintains an invariant where m_totalWidth is equal to the sum of these
> +        advances. However, in RTL text, inserting extra justification space to the left of a glyph
> +        would break that invariant, and would increase the advances of two glyphs instead of just
> +        one. Then, when we go to draw the text, the sum of the advances is wider than m_totalWidth,
> +        which means the glyphs would be drawn outside of their container.

When did this regress and why?

> Source/WebCore/ChangeLog:16
> +        Test: ComplexTextControllerTest.TotalWidthWithJustification

Should we not also have layout tests?
Comment 5 Myles C. Maxfield 2017-04-06 23:20:00 PDT
Comment on attachment 306470 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        which means the glyphs would be drawn outside of their container.
> 
> When did this regress and why?

r211382, because there were no tests for it.

>> Source/WebCore/ChangeLog:16
>> +        Test: ComplexTextControllerTest.TotalWidthWithJustification
> 
> Should we not also have layout tests?

I think the API test is sufficient. Creating a layout test requires making a new font, but the API test doesn't require that.
Comment 6 Simon Fraser (smfr) 2017-04-07 12:31:23 PDT
(In reply to Myles C. Maxfield from comment #5)
> Comment on attachment 306470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306470&action=review
> 
> >> Source/WebCore/ChangeLog:14
> >> +        which means the glyphs would be drawn outside of their container.
> > 
> > When did this regress and why?
> 
> r211382, because there were no tests for it.

Please update the changelog with this info.
Comment 7 Said Abou-Hallawa 2017-04-07 12:41:25 PDT
Comment on attachment 306470 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:14
>>>> +        which means the glyphs would be drawn outside of their container.
>>> 
>>> When did this regress and why?
>> 
>> r211382, because there were no tests for it.
> 
> Please update the changelog with this info.

When looking the attached picture https://bug-170399-attachments.webkit.org/attachment.cgi?id=306073, I see the Arabic text is justified by spaces. Should it be justified by Kashidas instead? Was this covered by this patch? Or is this what the css of the page selects to do? 
Also I saw text overlapping which happens with the English words Apple and RED. Will this fixed also by this patch?
Comment 8 Myles C. Maxfield 2017-04-07 12:46:51 PDT
(In reply to Said Abou-Hallawa from comment #7)
> Comment on attachment 306470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306470&action=review
> 
> >>>> Source/WebCore/ChangeLog:14
> >>>> +        which means the glyphs would be drawn outside of their container.
> >>> 
> >>> When did this regress and why?
> >> 
> >> r211382, because there were no tests for it.
> > 
> > Please update the changelog with this info.
> 
> When looking the attached picture
> https://bug-170399-attachments.webkit.org/attachment.cgi?id=306073, I see
> the Arabic text is justified by spaces. Should it be justified by Kashidas
> instead? Was this covered by this patch? Or is this what the css of the page
> selects to do? 
> Also I saw text overlapping which happens with the English words Apple and
> RED. Will this fixed also by this patch?

One day, yes, it would be great if we could justify using Kashidas. However, we currently have no concept of this. This patch is simply a bug fix.
Comment 9 Myles C. Maxfield 2017-04-07 12:47:36 PDT
Created attachment 306529 [details]
Patch
Comment 10 WebKit Commit Bot 2017-04-07 14:38:00 PDT
Comment on attachment 306529 [details]
Patch

Clearing flags on attachment: 306529

Committed r215117: <http://trac.webkit.org/changeset/215117>
Comment 11 WebKit Commit Bot 2017-04-07 14:38:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Jon Lee 2017-04-09 23:30:34 PDT
(In reply to Myles C. Maxfield from comment #8)
> (In reply to Said Abou-Hallawa from comment #7)
> > Comment on attachment 306470 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306470&action=review
> > 
> > >>>> Source/WebCore/ChangeLog:14
> > >>>> +        which means the glyphs would be drawn outside of their container.
> > >>> 
> > >>> When did this regress and why?
> > >> 
> > >> r211382, because there were no tests for it.
> > > 
> > > Please update the changelog with this info.
> > 
> > When looking the attached picture
> > https://bug-170399-attachments.webkit.org/attachment.cgi?id=306073, I see
> > the Arabic text is justified by spaces. Should it be justified by Kashidas
> > instead? Was this covered by this patch? Or is this what the css of the page
> > selects to do? 
> > Also I saw text overlapping which happens with the English words Apple and
> > RED. Will this fixed also by this patch?
> 
> One day, yes, it would be great if we could justify using Kashidas. However,
> we currently have no concept of this. This patch is simply a bug fix.

Do we already have a bug for this enhancement?
Comment 13 Myles C. Maxfield 2017-04-10 08:13:32 PDT
(In reply to Jon Lee from comment #12)
> (In reply to Myles C. Maxfield from comment #8)
> > (In reply to Said Abou-Hallawa from comment #7)
> > > Comment on attachment 306470 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=306470&action=review
> > > 
> > > >>>> Source/WebCore/ChangeLog:14
> > > >>>> +        which means the glyphs would be drawn outside of their container.
> > > >>> 
> > > >>> When did this regress and why?
> > > >> 
> > > >> r211382, because there were no tests for it.
> > > > 
> > > > Please update the changelog with this info.
> > > 
> > > When looking the attached picture
> > > https://bug-170399-attachments.webkit.org/attachment.cgi?id=306073, I see
> > > the Arabic text is justified by spaces. Should it be justified by Kashidas
> > > instead? Was this covered by this patch? Or is this what the css of the page
> > > selects to do? 
> > > Also I saw text overlapping which happens with the English words Apple and
> > > RED. Will this fixed also by this patch?
> > 
> > One day, yes, it would be great if we could justify using Kashidas. However,
> > we currently have no concept of this. This patch is simply a bug fix.
> 
> Do we already have a bug for this enhancement?

I don't think so.
Comment 14 Jon Lee 2017-04-10 08:45:56 PDT
*** Bug 170654 has been marked as a duplicate of this bug. ***