Bug 207319 - Draw underlines when specified in highlights
Summary: Draw underlines when specified in highlights
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-05 18:00 PST by Megan Gardner
Modified: 2020-02-11 13:53 PST (History)
13 users (show)

See Also:


Attachments
Patch (9.19 KB, patch)
2020-02-05 18:19 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (8.68 KB, patch)
2020-02-06 00:14 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2020-02-10 15:22 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.61 KB, patch)
2020-02-11 10:01 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.59 KB, patch)
2020-02-11 11:01 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (11.59 KB, patch)
2020-02-11 12:58 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-02-05 18:00:33 PST
Draw underlines when specified in highlights
Comment 1 Megan Gardner 2020-02-05 18:19:49 PST
Created attachment 389928 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-02-05 18:20:35 PST
<rdar://problem/59210451>
Comment 3 Simon Fraser (smfr) 2020-02-05 19:01:56 PST
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"?
Comment 4 Megan Gardner 2020-02-06 00:14:54 PST
Created attachment 389939 [details]
Patch
Comment 5 Simon Fraser (smfr) 2020-02-06 11:10:10 PST
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.
Comment 6 Megan Gardner 2020-02-10 15:22:42 PST
Created attachment 390301 [details]
Patch
Comment 7 Megan Gardner 2020-02-11 10:01:17 PST
Created attachment 390381 [details]
Patch
Comment 8 Megan Gardner 2020-02-11 11:01:51 PST
Created attachment 390392 [details]
Patch
Comment 9 Megan Gardner 2020-02-11 12:58:41 PST
Created attachment 390408 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-02-11 13:53:05 PST
Comment on attachment 390408 [details]
Patch for landing

Clearing flags on attachment: 390408

Committed r256360: <https://trac.webkit.org/changeset/256360>
Comment 11 WebKit Commit Bot 2020-02-11 13:53:07 PST
All reviewed patches have been landed.  Closing bug.