Bug 191341 – Positioned text underline can look like a strike-through

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

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.

[Radar WebKit Bug Importer]

<rdar://problem/45861658>

[Myles C. Maxfield]

Created attachment 354165 [details]
Patch

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

Committed r237955: <https://trac.webkit.org/changeset/237955>

[Darin Adler]

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

[Darin Adler]

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.