Bug 250969 – RTL Tab handling is imperfect

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ebrahim Byagowi]

Created attachment 464591 [details]
Safari vs Firefox

Happens both in WebKit tip of tree and stable 16.2,

Reproduce:
Compare this in Safari, Chrome and Firefox

data:text/html;charset=utf8,<textarea id=textArea style="font-size: 40px; width: 1000px; font-family: Tahoma" dir=rtl></textarea><script>var i = 0; setInterval(() => { i = (i + 1) % 8; textArea.innerText = 'ش\t'.repeat(i); textArea.select() }, 1000);</script>

Note the inserted characters position, Safari apparently isn't able to handle tab in RTL correctly.

It can be seen in the attached screenshot also, distribution of spaces between letter isn't correct, the first letter is out of view.

[alan]

Created attachment 464592 [details]
Test reduction

[alan]

Thank you for the detailed bug report!

[Radar WebKit Bug Importer]

<rdar://problem/104528894>

[alan]

There's at least 2 bugs here, the attached test reduction never worked in legacy line layout and modern line layout recently regressed it at 254828@main.
However I think the initial test case never worked in modern line layout and fixing 254828@main won't make it go away. Will file a separate bugzilla on it.

[alan]

ok, it looks like "unicode-bidi: isolate" is the culprit here. It's the combination of white-space: pre (for preserving trailing tab) and "isolate" (<- incorrect positioning),
where the non-isolate case recently regressed (see above) and the isolate case never worked in modern line layout (IFC).

[alan]

Created attachment 464596 [details]
Screen recording with local fix

it looks like it's about incorrect bidi level on trailing whitespace (tab) runs. Screen recording is with local fix.

[Ebrahim Byagowi]

> Screen recording is with local fix.

Looks good except that double highlighting which I wish they can go also, feel free to post the patch here so I can help on verifying whether it doesn't regress any other case and covers all the issues I'm seeing with tab.

[alan]

(In reply to Ebrahim Byagowi from comment #7)
> > Screen recording is with local fix.
> 
> Looks good except that double highlighting which I wish they can go also,
> feel free to post the patch here so I can help on verifying whether it
> doesn't regress any other case and covers all the issues I'm seeing with tab.
Thank you! (will be posting the patch soon, it just needs some more work)

[alan]

> Thank you! (will be posting the patch soon, it just needs some more work)
Actually I need to talk with Myles first before posting a patch. We may need to address this in ComplexTextController.

Let's take the following example: "ش	" where the inline direction is right-to-left.
1. During layout (at this point we are in logical space), first we measure ش, followed by the tab character.
2. By the time we measure the tab character we already advanced to position X (where X is the (logical) right edge of the measured ش glyph, let's ignore content box offsets for now.)
3. tab character width is horizontal position dependent. At position X, we measure it to be W.
4. Now at paint time, we take the tab glyph first (it's visually the first glyph), and measure it at position 0. 
This measured width may be very different from W due to the difference in X position. (and this is what's causing this bug).

Now the difference between legacy and IFC is that IFC merges runs whenever possible. In this case legacy constructs dedicated run for the trailing (visually leading) tab character.
With dedicated runs, we paint each glyphs at explicit positions computed during layout (e.g [tab] at 0px, [ش] at 50px vs [tabش] at 0px)
ComplexTextController still computes incorrect advanced width for the tab character, but with explicit run positions it's mostly unnoticeable (unless you select the content and get double highlights).
Soon after pre-wrap is changed to pre, legacy mergers runs and the bug shows.

We could address this in IFC by always constructing dedicated trailing tab runs, but it would not address the selection issue.

[alan]

Created attachment 464624 [details]
fixed double selection

This is with dedicated runs and logical tab position at paint time.

[Ebrahim Byagowi]

> fixed double selection

This looks great.

> is horizontal position dependent

Oh, now I see the issue also.

As said I like to get more involved with the development of the fix here, yet, I feel that causes more hassle to you so instead I just like to express that I'm very happy that WebKit's text rendering is getting a proper love it deserves, (also this isn't to rush anything, a proper fix indeed takes considerable time to be correctly figured out then applied)

So, thanks 😊

[alan]

(In reply to Ebrahim Byagowi from comment #11)
> > fixed double selection
> 
> This looks great.
> 
> > is horizontal position dependent
> 
> Oh, now I see the issue also.
> 
> As said I like to get more involved with the development of the fix here,
> yet, I feel that causes more hassle to you so instead I just like to express
> that I'm very happy that WebKit's text rendering is getting a proper love it
> deserves, (also this isn't to rush anything, a proper fix indeed takes
> considerable time to be correctly figured out then applied)
> 
> So, thanks 😊
:) thank you! I really appreciate that you are willing to take the trouble to apply the patch locally and run regression testing! So awesome.

[alan]

Created attachment 464654 [details]
Patch

[alan]

Filed bug 251169 for the overlapping selection issue.

[alan]

Comment on attachment 464654 [details]
Patch

Needs rebaselining.

[alan]

Created attachment 464665 [details]
Patch

[alan]

Created attachment 464668 [details]
Patch

[EWS]

Committed 259428@main (7b94b0c936fb): <https://commits.webkit.org/259428@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 464668 [details].

[Ebrahim Byagowi]

I tested tabs in RTL with tip of tree (5dc808f) and everything works as expected. Thank you very much 😊

[alan]

(In reply to Ebrahim Byagowi from comment #19)
> I tested tabs in RTL with tip of tree (5dc808f) and everything works as
> expected. Thank you very much 😊
Awesome! Thank you for confirming it!