Bug 170399 – REGRESSION (r211382): Arabic text of non-system font cannot be justified

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

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.

[Radar WebKit Bug Importer]

<rdar://problem/31442008>

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 306470 [details]
Patch

[Simon Fraser (smfr)]

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?

[Myles C. Maxfield]

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.

[Simon Fraser (smfr)]

(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.

[Said Abou-Hallawa]

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?

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

Created attachment 306529 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306529 [details]
Patch

Clearing flags on attachment: 306529

Committed r215117: <http://trac.webkit.org/changeset/215117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jon Lee]

(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?

[Myles C. Maxfield]

(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.

[Jon Lee]

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