Bug 232663

Summary: Wavy decorations don't cover the whole line length
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Layout and RenderingAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, clopez, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, mmaxfield, pdr, simon.fraser, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/31540
Bug Depends on: 207175    
Bug Blocks:    
Attachments:
Description Flags
Test case
none
Screenshot of test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Typography panel in TextEdit
none
Text panel in Pages none

Description Manuel Rego Casasnovas 2021-11-03 05:21:16 PDT
Created attachment 443191 [details]
Test case

Wavy decorations not always cover the whole line length.

We only paint whole waves, so we might fail to cover the whole length of the line in some cases:
https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/TextDecorationPainter.cpp#85

See attached example and screenshot.
Comment 1 Manuel Rego Casasnovas 2021-11-03 05:21:32 PDT
Created attachment 443192 [details]
Screenshot of test case
Comment 2 Manuel Rego Casasnovas 2021-11-03 05:48:48 PDT
Created attachment 443194 [details]
Patch
Comment 3 Manuel Rego Casasnovas 2021-11-03 06:54:29 PDT
Created attachment 443197 [details]
Patch
Comment 4 Manuel Rego Casasnovas 2021-11-03 06:58:55 PDT
I guess the patch would need some new rebaselines, but I'd like to know your thoughts about the approach.

As this is removing adjustStepToDecorationLength() the waves are shorter. If we want to keep a similar size than the one we have currently, we might need to tweak the values in getWavyStrokeParameters(). Something like this:
diff --git a/Source/WebCore/style/InlineTextBoxStyle.cpp b/Source/WebCore/style/InlineTextBoxStyle.cpp
index b4c4fc4b9509..854948b38179 100644
--- a/Source/WebCore/style/InlineTextBoxStyle.cpp
+++ b/Source/WebCore/style/InlineTextBoxStyle.cpp
@@ -160,8 +160,8 @@ WavyStrokeParameters getWavyStrokeParameters(float fontSize)
 {
     WavyStrokeParameters result;
     // More information is in the WavyStrokeParameters definition.
-    result.controlPointDistance = fontSize * 1.5 / 16;
-    result.step = fontSize / 4.5;
+    result.controlPointDistance = fontSize * 1.7 / 16;
+    result.step = fontSize / 4.3;
     return result;
 }


Here's a comparison of the results in trunk, trunk + attached patch, and trunk + attached patch + the tweaks described above:
https://i.imgur.com/0EsJSsA.png

Please let me know your thoughts, thanks!
Comment 5 Manuel Rego Casasnovas 2021-11-03 22:48:30 PDT
Created attachment 443276 [details]
Patch
Comment 6 Myles C. Maxfield 2021-11-05 12:24:17 PDT
Comment on attachment 443276 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        To fix this we're modifying strokeWavyTextDecoration() method.

How does this work with <span>something<span>nested</span>else</span>? Wouldn’t the waves not match up? I think the solution to this is to implement the spec’s concept of “decorating box” which I don’t think we’ve implemented. https://bugs.webkit.org/show_bug.cgi?id=190772
Comment 7 Manuel Rego Casasnovas 2021-11-07 22:06:24 PST
(In reply to Myles C. Maxfield from comment #6)
> Comment on attachment 443276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443276&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        To fix this we're modifying strokeWavyTextDecoration() method.
> 
> How does this work with <span>something<span>nested</span>else</span>?
> Wouldn’t the waves not match up? I think the solution to this is to
> implement the spec’s concept of “decorating box” which I don’t think we’ve
> implemented. https://bugs.webkit.org/show_bug.cgi?id=190772

Yes they don't match up, but we already have a similar issue right now. So not sure if that's a blocker for this or not.

This is a screenshot comparing current status (left) vs this patch (right):
https://i.imgur.com/XIQGMPc.png

And here's the same screenshot but making the nested span "font-size: 150%" (as that changes the thickness of the wavy line too):
https://i.imgur.com/nkPntRx.png
Comment 8 Myles C. Maxfield 2021-11-07 22:48:22 PST
Comment on attachment 443276 [details]
Patch

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

> LayoutTests/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Instead of a DRT test, a better test would be something that draws text of a known width under a big transform, and clips out everything except the area under the text where there's no underline today, and compares that with an -expected-mismatch test against pure white. That will make sure that we draw something covering the entire span of text.
Comment 9 Manuel Rego Casasnovas 2021-11-08 01:25:18 PST
(In reply to Myles C. Maxfield from comment #8)
> Comment on attachment 443276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443276&action=review
> 
> > LayoutTests/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> Instead of a DRT test, a better test would be something that draws text of a
> known width under a big transform, and clips out everything except the area
> under the text where there's no underline today, and compares that with an
> -expected-mismatch test against pure white. That will make sure that we draw
> something covering the entire span of text.

Good idea about the test, I've created a PR in WPT with tests following that idea: https://github.com/web-platform-tests/wpt/pull/31540

Also that test help me to found a mistake on my Blink fix for this issue. O:)
Comment 10 Manuel Rego Casasnovas 2021-11-08 22:57:52 PST
Created attachment 443656 [details]
Patch
Comment 11 Manuel Rego Casasnovas 2021-11-09 04:29:23 PST
(In reply to Manuel Rego Casasnovas from comment #4)
> I guess the patch would need some new rebaselines, but I'd like to know your
> thoughts about the approach.

I've been checking this and it looks like we don't need new rebaselines, as I don't find -expected.png for the tests that use "wavy" text decorations.

So now the patch includes the WPT tests, and I understand it's good to go.
Comment 12 EWS 2021-11-10 00:38:26 PST
Committed r285567 (244075@main): <https://commits.webkit.org/244075@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443656 [details].
Comment 13 Radar WebKit Bug Importer 2021-11-10 00:39:21 PST
<rdar://problem/85243338>
Comment 14 Darin Adler 2021-11-10 14:35:08 PST
As I understand it, the original wavy line algorithm was intended to match platform wavy lines on iOS or macOS. Is that right? Are we now doing something different that we think is better?
Comment 15 Myles C. Maxfield 2021-11-10 17:24:36 PST
Created attachment 443880 [details]
Typography panel in TextEdit
Comment 16 Myles C. Maxfield 2021-11-10 17:24:51 PST
Created attachment 443881 [details]
Text panel in Pages
Comment 17 Myles C. Maxfield 2021-11-10 17:25:42 PST
I don't think macOS has wavy underlines. At least I don't see it in the Typography panel of TextEdit (screenshot attached) or in the Text panel in Pages (screenshot attached).
Comment 18 Darin Adler 2021-11-10 18:56:17 PST
Guess I misunderstood. I thought these were the things we used for spelling correction and such.
Comment 19 Myles C. Maxfield 2021-11-10 21:13:17 PST
Nope. Those are dots, and use a different codepath. (Microsoft Word uses wavy underlines to denote spelling/grammar errors, but Apple OSes use dots instead.)