Bug 207319

Created attachment 389928 [details]
Patch

<rdar://problem/59210451>

Comment on attachment 389928 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        When determining if we have any text decorations, currently we were only lookin at the lineStyle,

lookin

> Source/WebCore/rendering/InlineTextBox.cpp:612
> +    bool highlightDecorations = !collectMarkedTextsForHighlights(TextPaintPhase::Decoration).isEmpty();

haveHighlightDecorations.
For symmetry, add bool haveTextDecorations = !textDecorations.isEmpty() and use it below.

> Source/WebCore/rendering/InlineTextBox.cpp:1203
> +    auto textDecorations = lineStyle().textDecorationsInEffect() | TextDecorationPainter::textDecorationsInEffectForStyle(markedText.style.textDecorationStyles);

Took me a while to unpack this, with the auto and the |. It would be clearer as:
auto textDecorations = lineStyle().textDecorationsInEffect()
textDecorations.add(TextDecorationPainter::textDecorationsInEffectForStyle(markedText.style.textDecorationStyles)) or something.

> Source/WebCore/rendering/TextDecorationPainter.cpp:390
> +OptionSet<TextDecoration> TextDecorationPainter::textDecorationsInEffectForStyle(const TextDecorationPainter::Styles style)

const TextDecorationPainter::Styles&

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-decorations-expected.html:16
> +    #style1 {
> +        background-color: yellow;
> +        color:green;
> +    }
> +    #style2 {
> +        background-color: blue;
> +        color:red;
> +    }
> +    #style3 {
> +        background-color: purple;
> +        color:pink;
> +    }

Why I am I not seeing underline, overline or line-through here?

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-decorations.html:5
> +    <title>Multiple custom highlight pseudo elements.</title>

This seems stale.

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-decorations.html:6
> +    <link rel="help" href="https://drafts.csswg.org/css-highlight-api-1/#creating-highlights">

This seems wrong.

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-decorations.html:8
> +    <meta name="assert" content="Text decorations in highlights should be able to be specified.">

"should be able to be specified" is a bit vague. Maybe "should display"?

Created attachment 389939 [details]
Patch

Comment on attachment 389939 [details]
Patch

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

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-decorations-expected.html:11
> +    O<span id="style1">n</span>e t<span id="style1">w</span>o th<span id="style1">ree</span>

Use class not id (and change the selector above to .style1. Also make the text bigger so that failures are more obvious (more pixels).

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-decorations.html:13
> +        <style>
> +        ::highlight(example-highlight1) {
> +            text-decoration: underline;
> +        }
> +        </style>

Extra indent here.

Created attachment 390301 [details]
Patch

Created attachment 390381 [details]
Patch

Created attachment 390392 [details]
Patch

Created attachment 390408 [details]
Patch for landing

Comment on attachment 390408 [details]
Patch for landing

Clearing flags on attachment: 390408

Committed r256360: <https://trac.webkit.org/changeset/256360>

All reviewed patches have been landed.  Closing bug.