Bug 220885

Summary: WebKit doesn't automatically right-align Adlam
Product: WebKit Reporter: Fuqiao Xue <xfq.free>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, cmarcelo, darin, ews-watchlist, ishida, mmaxfield, smoley, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WebKit doesn't automatically right-align Adlam
none
Patch none

Description Fuqiao Xue 2021-01-22 19:22:09 PST
Adlam text displayed in a block element with 'dir' set to 'auto' should be automatically right-aligned, unless the base direction is set to LTR.

See https://github.com/w3c/afrlreq/issues/13 for more info.
Comment 1 Smoley 2021-01-27 17:47:48 PST
Can you please attach a test html file as well? Thanks
Comment 2 Radar WebKit Bug Importer 2021-01-27 17:47:57 PST
<rdar://problem/73688496>
Comment 3 Smoley 2021-01-27 17:51:34 PST
Actually it looks like this page may work: http://adlamtesting.appspot.com
Comment 4 Smoley 2021-01-27 17:54:09 PST
On second thought that page is showing it in a <textarea> but at least it provides some test text.
Comment 5 Fuqiao Xue 2021-01-28 19:09:25 PST
Created attachment 418695 [details]
WebKit doesn't automatically right-align Adlam

Here's a test HTML file.
Comment 6 Myles C. Maxfield 2021-01-29 15:20:36 PST
The content has dir="auto"
Comment 7 Myles C. Maxfield 2021-01-29 15:22:14 PST
If the text is moved out from the intermediate <p> element, it becomes right aligned. Somehow the <p> element is causing the problem.
Comment 8 Myles C. Maxfield 2021-01-29 15:23:32 PST
(In reply to Myles C. Maxfield from comment #7)
> If the text is moved out from the intermediate <p> element, it becomes right
> aligned. Somehow the <p> element is causing the problem.

Whoops, this is only true if you insert some RTL text like Arabic at the beginning of the string.
Comment 9 Myles C. Maxfield 2021-01-29 15:25:24 PST
(In reply to Myles C. Maxfield from comment #8)
> (In reply to Myles C. Maxfield from comment #7)
> > If the text is moved out from the intermediate <p> element, it becomes right
> > aligned. Somehow the <p> element is causing the problem.
> 
> Whoops, this is only true if you insert some RTL text like Arabic at the
> beginning of the string.

Yeah, ignore what I said about the <p> element. The <p> element has nothing to do with this.
Comment 10 Myles C. Maxfield 2021-01-29 15:32:10 PST
The first place to check would be StringImpl::defaultWritingDirection()
Comment 11 Myles C. Maxfield 2021-01-29 15:33:48 PST
oh hahahaha we're terrible

auto charDirection = u_charDirection(is8Bit() ? m_data8[i] : m_data16[i]);

super broken
Comment 12 Myles C. Maxfield 2021-01-29 15:48:48 PST
Created attachment 418778 [details]
Patch
Comment 13 EWS 2021-01-29 16:18:32 PST
Committed r272084: <https://trac.webkit.org/changeset/272084>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418778 [details].
Comment 14 Darin Adler 2021-01-29 17:04:30 PST
Comment on attachment 418778 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.cpp:1672
>  UCharDirection StringImpl::defaultWritingDirection(bool* hasStrongDirectionality)

Some day to clean up: Feels to me like we should move this function to StringView or make it a non-member function that takes a StringView, in some place where we have code about writing direction and bidirectional algorithms. And give it a better interface with return values instead of an out argument. And add a fast path so we aren’t calling u_charDirection in common cases.
Comment 15 Darin Adler 2021-01-29 17:04:30 PST
Comment on attachment 418778 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.cpp:1672
>  UCharDirection StringImpl::defaultWritingDirection(bool* hasStrongDirectionality)

Some day to clean up: Feels to me like we should move this function to StringView or make it a non-member function that takes a StringView, in some place where we have code about writing direction and bidirectional algorithms. And give it a better interface with return values instead of an out argument. And add a fast path so we aren’t calling u_charDirection in common cases.
Comment 16 Myles C. Maxfield 2021-01-29 17:50:00 PST
(In reply to Darin Adler from comment #15)
> Comment on attachment 418778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418778&action=review
> 
> > Source/WTF/wtf/text/StringImpl.cpp:1672
> >  UCharDirection StringImpl::defaultWritingDirection(bool* hasStrongDirectionality)
> 
> Some day to clean up: Feels to me like we should move this function to
> StringView or make it a non-member function that takes a StringView, in some
> place where we have code about writing direction and bidirectional
> algorithms. And give it a better interface with return values instead of an
> out argument. And add a fast path so we aren’t calling u_charDirection in
> common cases.

Yeah, Alex and I had a conversation about the direction to move this class. Sounds like we all agree :)
Comment 17 r12a 2022-02-03 04:28:28 PST
Thanks for fixing this.