Bug 241442

Summary: Repaint rect does not take into account text-underline-offset
Product: WebKit Reporter: Jon Lee <jonlee554>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin, ews-watchlist, koivisto, mmaxfield, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 241471, 241474, 241488, 241490    
Bug Blocks:    
Attachments:
Description Flags
Test reduction
none
Test reduction
none
Patch
none
Patch
none
Patch none

Jon Lee
Reported 2022-06-08 18:03:00 PDT
https://codepen.io/legion80/pen/MWQPoRZ Hover over the two links. The hover should turn the underline color to red. It does when the decoration is within the bounding box of the text, but does not when it's outside.
Attachments
Test reduction (253 bytes, text/html)
2022-06-08 19:30 PDT, zalan 		no flags
Details
Test reduction (256 bytes, text/html)
2022-06-08 19:32 PDT, zalan 		no flags
Details
Patch (9.15 KB, patch)
2022-06-09 21:34 PDT, zalan 		no flags
DetailsFormatted DiffDiff
Patch (13.15 KB, patch)
2022-06-10 10:46 PDT, zalan 		no flags
DetailsFormatted DiffDiff
Patch (15.35 KB, patch)
2022-06-10 17:01 PDT, zalan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
zalan
Comment 1 2022-06-08 19:11:29 PDT
It looks like the underline is supposed to be part of the ink overflow but for some reason we don't take it into account when computing it. When ink overflow is inflated with box-shadow, repaint works just fine.
zalan
Comment 2 2022-06-08 19:15:24 PDT
https://jsfiddle.net/7f16hr4d/
zalan
Comment 3 2022-06-08 19:30:28 PDT
Created attachment 460109 [details] Test reduction
zalan
Comment 4 2022-06-08 19:32:35 PDT
Created attachment 460110 [details] Test reduction make it less flaky by adding some non-zero timeout
Radar WebKit Bug Importer
Comment 5 2022-06-08 19:50:39 PDT
<rdar://problem/94686075>
zalan
Comment 6 2022-06-09 21:34:33 PDT
Created attachment 460155 [details] Patch
zalan
Comment 7 2022-06-09 21:40:53 PDT
(In reply to zalan from comment #6) > Created attachment 460155 [details] > Patch need to come up with some test cases including vertical and flipped modes.
zalan
Comment 8 2022-06-10 10:46:13 PDT
Created attachment 460163 [details] Patch
Myles C. Maxfield
Comment 9 2022-06-10 11:24:55 PDT
Last time I tried to do this, it caused a large performance regression due to repaint rects intersecting new renderers, which caused more to be rendered than used to be rendered. Are you confident this doesn't cause a performance regression?
Myles C. Maxfield
Comment 10 2022-06-10 11:26:37 PDT
See https://bugs.webkit.org/show_bug.cgi?id=190774
zalan
Comment 11 2022-06-10 11:39:04 PDT
(In reply to Myles C. Maxfield from comment #9) > Last time I tried to do this, it caused a large performance regression due > to repaint rects intersecting new renderers, which caused more to be > rendered than used to be rendered. Are you confident this doesn't cause a > performance regression? Wasn't that mostly about RenderStyle::changeAffectsVisualOverflow (https://trac.webkit.org/changeset/244277)? None of that has changed here. This is about matching ink overflow produced by legacy line layout (and while this patch certainly has performance implications when certain properties are present, we don't really have a choice but inflate the ink overflow rect as needed).
zalan
Comment 12 2022-06-10 12:13:18 PDT
Comment on attachment 460163 [details] Patch repaintrect height went from 12 to 13 (probably because of the +1 to compensate for the integral ceiling in GraphicsContext::computeLineBoundsAndAntialiasingModeForText() will rebaseline.
zalan
Comment 13 2022-06-10 17:01:44 PDT
Created attachment 460171 [details] Patch
EWS
Comment 14 2022-06-10 22:34:51 PDT
Committed r295472 (251477@main): <https://commits.webkit.org/251477@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 460171 [details].
zalan
Comment 15 2022-06-11 06:56:29 PDT
Thank you for reporting this bug!
Darin Adler
Comment 16 2022-06-11 09:52:19 PDT
In future can just use “internals” instead of “window.internals” in almost all cases.
zalan
Comment 17 2022-06-11 15:51:17 PDT
(In reply to Darin Adler from comment #16) > In future can just use “internals” instead of “window.internals” in almost > all cases. Thanks, will do!
zalan
Comment 18 2022-07-10 06:54:29 PDT
*** Bug 242570 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.