Bug 191341 - Positioned text underline can look like a strike-through
Summary: Positioned text underline can look like a strike-through
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-06 17:11 PST by Myles C. Maxfield
Modified: 2018-11-09 06:58 PST (History)
8 users (show)

See Also:


Attachments
Patch (6.06 KB, patch)
2018-11-07 15:18 PST, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2018-11-06 17:11:51 PST
It allows negative values. We probably shouldn't allow it to look like a strike-through because that could change the semantic meaning of the text.
Comment 1 Radar WebKit Bug Importer 2018-11-06 17:12:25 PST
<rdar://problem/45861658>
Comment 2 Myles C. Maxfield 2018-11-07 15:18:47 PST
Created attachment 354165 [details]
Patch
Comment 3 Simon Fraser (smfr) 2018-11-07 15:24:26 PST
Comment on attachment 354165 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        We should just clamp the value so it can't go above the baseline.
> +
> +        We shouldn't do this at parse time because it's totally reasonable for text-underline-position: under to want
> +        a negative text-underline-offset. Instead, we just do it at used value time.

Will the spec describe this behavior?

> Source/WebCore/style/InlineTextBoxStyle.cpp:62
> +        return fontMetrics.ascent() + std::max(0.0f, underlineOffset.lengthValue());

Slightly prefer std::max<float>(0, underlineOffset.lengthValue())

> Source/WebCore/style/InlineTextBoxStyle.cpp:64
> +        return fontMetrics.ascent() + std::max(0.0f, fontMetrics.underlinePosition() + underlineOffset.lengthOr(0));

Ditto.
Comment 4 Myles C. Maxfield 2018-11-07 17:08:41 PST
Committed r237955: <https://trac.webkit.org/changeset/237955>
Comment 5 Darin Adler 2018-11-09 06:57:02 PST
Comment on attachment 354165 [details]
Patch

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

> Source/WebCore/style/InlineTextBoxStyle.cpp:38
> +    int gap = std::max<int>(1, ceilf(defaultGap / 2.0));

This involves conversion to floating point and back, and also conversion between two different floating point types. Unnecessarily complex and inefficient:

    defaultGap is an int
    we divide it by 2.0, a double, which means converting it to a double first
    then we call ceilf, which takes a float, which means converting the double to a float
    then we pass it to std::max<int>, which converts it back to an int

Assuming that we don’t care about the behavior for std::numeric_limits<int>::max(), and I think we don’t since the rest of the code is written without any allowance for integer overflow, we can get the same result without involving double or float:

    int gap = std::max(1, (defaultGap + 1) / 2);
Comment 6 Darin Adler 2018-11-09 06:58:48 PST
Comment on attachment 354165 [details]
Patch

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

> Source/WebCore/style/InlineTextBoxStyle.cpp:83
> +        desiredOffset = std::max<float>(desiredOffset, fontMetrics.ascent());
> +        return desiredOffset;

No need for two lines of code here. We should just return the value; no need to write it back into the local variable first.