Bug 232663

Created attachment 443192 [details] Screenshot of test case

Created attachment 443194 [details] Patch

Created attachment 443197 [details] Patch

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!

Created attachment 443276 [details] Patch

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

(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 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.

(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:)

Created attachment 443656 [details] Patch

(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.

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].

<rdar://problem/85243338>

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?

Created attachment 443880 [details] Typography panel in TextEdit

Created attachment 443881 [details] Text panel in Pages

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).

Guess I misunderstood. I thought these were the things we used for spelling correction and such.

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.)