Bug 220548 – Hairline on selection when bidi text is involved

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ebrahim Byagowi]

Created attachment 417468 [details]
hairline on selection

Steps to reproduce:
Open
data:text/html;charset=utf8,<div>نوامبر%2020
select all of the text and zoom in with pinch to zoom to see the issue better

Expected:
A clear selection, like Firefox and Chrome

Actual:
Spotting a white hairline on selection

More of different kinds of such hairlines can be easily spotted when opening https://fa.wikipedia.org/ and pressing ⌘+A perhaps we should reach to them one by one if they are not of the same root.

[Radar WebKit Bug Importer]

<rdar://problem/73362060>

[Ebrahim Byagowi]

This one doesn't involve bidi,

data:text/html;charset=utf8,<span%20style="font-family:sans-serif"><span%20lang="ja">フレンズ</span>,

[Ebrahim Byagowi]

This one still exists, just for the polishment worth to have a look I'd guess.

[Ebrahim Byagowi]

Created attachment 464685 [details]
Hairlines

@zalan, this will be a nice polish to have I believe, the screen cast shows how reliably one can see hairlines on Persian Wikipedia's homepage, https://fa.wikipedia.org other than minimal tests I've brought it.

[zalan]

(In reply to Ebrahim Byagowi from comment #4)
> Created attachment 464685 [details]
> Hairlines
> 
> @zalan, this will be a nice polish to have I believe, the screen cast shows
> how reliably one can see hairlines on Persian Wikipedia's homepage,
> https://fa.wikipedia.org other than minimal tests I've brought it.
:| this is an unfortunate side effect of how we measure text at soft wrap opportunity boundaries and happily join such "runs" without remeasuring them ignoring kerning (and ligature) adjustments.

let's take the following example:
<span>ンズ</span>text
we form the following "runs" at soft wrap opportunities and measure them individually before line breaking
[ン] -> 16px
[ズ] -> 16px
[text] -> 42px
Later as we form the line, we merge [ン][ズ] into one run. This run now has the width of 32px but with potential kerning (and ligature) it may very well be (slightly) incorrect.
When we get to selection painting (as background decoration) we have the following runs:
[ンズ] -> left 0px, right 32px
[text] -> left 32px, right 74px
Now we want to be precise with the selection boundaries, so we call FontCascade::adjustSelectionRectForText() on these individual runs and due to kerning (and ligature) we may see a (slightly) different widths this time (note that we measure ンズ as one continuous run this time) 
[ンズ] -> 31.12px (and with directional pixel snapping -> 31.5px)
[text] -> 42px

and we paint selection at the following positions 
  from 0px to 31.5px
and
  from 42px to 74px

which ends up producing a hairline gap.

Now the correct fix is to never simply sum individual run widths when merging, but it has some performance (and line breaking cycle) implications afaict.
The more practical fix may be to never shrink selection rect through this adjustment. Need to do some code reading first.

[zalan]

Created attachment 464688 [details]
no shrink selection rect

This is what the selection on the wiki page looks like after not letting the selection rect shrink.

[Ebrahim Byagowi]

> Need to do some code reading first.

Oh, that just sounds fantastic, now that I know this problem had reached into good hands, I won't make any noise about it anymore as I indeed understand something to not happen in favor of performance given current infrastructure and code architecture. Thanks 😊

[zalan]

Created attachment 464701 [details]
Patch

[Ebrahim Byagowi]

> Patch 

The patch works flawlessly when applied upon current tip of tree (ca0fcb99), and I'm hopeful it can be landed, thank you very much! 😊

[zalan]

(In reply to Ebrahim Byagowi from comment #9)
> > Patch 
> 
> The patch works flawlessly when applied upon current tip of tree (ca0fcb99),
> and I'm hopeful it can be landed, thank you very much! 😊
Thank you!

[zalan]

Created attachment 464702 [details]
Patch

[Ebrahim Byagowi]

> Patch

Just wanted to let you know I've tested the updated version of the patch also, awesome! 😊

[zalan]

Created attachment 464741 [details]
Patch

[zalan]

Same patch (now with a test case)

[EWS]

Committed 259537@main (df25b495844d): <https://commits.webkit.org/259537@main>

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