Bug 217417 – Paint CSS highlights over images

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Paint CSS highlights over images

[Megan Gardner]

Created attachment 410725 [details]
Patch

[Darin Adler]

Comment on attachment 410725 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:159
> +    auto* parentRenderer = parent();

Put this inside the if?

> Source/WebCore/rendering/RenderReplaced.cpp:160
> +    const RenderStyle* parentStyle;

This unused local is why Windows build is failing and should be deleted.

> Source/WebCore/rendering/RenderReplaced.cpp:161
> +    Color highlightColor;

This is unused and should be deleted.

> Source/WebCore/rendering/RenderReplaced.cpp:167
> +                Position startPosition = rangeData->startPosition.value();

auto?

> Source/WebCore/rendering/RenderReplaced.cpp:169
> +                auto* startRenderer = startPosition.deprecatedNode()->renderer();

Should use containerNode here, not deprecatedNode. Same below.

> Source/WebCore/rendering/RenderReplaced.cpp:170
> +                unsigned startOffset = startPosition.deprecatedEditingOffset();

Should use computeOffsetInContainerNode here, not deprecatedEditingOffset. Same below.

> Source/WebCore/rendering/RenderReplaced.cpp:174
> +                    continue;

Used continue here but nested if above. Should pick one style and stick with it.

> Source/WebCore/rendering/RenderReplaced.cpp:177
> +                highlightData.setRenderRange({ startRenderer, endRenderer, startOffset, endOffset });

I think we need a helper function to make an optional render range from range data to avoid that long 9 lines of code above and the extra if nesting too. But could land like this and refactor I guess.

> Source/WebCore/rendering/RenderReplaced.cpp:178
> +                

Extra blank line

> Source/WebCore/rendering/RenderReplaced.cpp:182
> +                    auto renderStyle = parentRenderer->getUncachedPseudoStyle({ PseudoId::Highlight, highlight.key }, parentStyle);

Put this inside the if? Also could just name it style.

> Source/WebCore/rendering/RenderReplaced.cpp:242
> +    bool drawHighlight = shouldDrawHighlight(paintInfo);

I suggest we use a transparent highlight color to signify this instead of a separate boolean.

> Source/WebCore/rendering/RenderReplaced.cpp:246
> +        if (!highlightColor.isVisible())

Just do this isVisible check below instead off checking the boolean.

> Source/WebCore/rendering/RenderReplaced.cpp:252
>          if (selectionState() == HighlightState::None)

Don’t we need to set drawHighlight to false here? (Or in my new proposal, set highlightColor to invalid or transparent.) Or maybe optimize to not even call shouldDrawHighlight or potentialHighlightColor or even shouldDrawSelectionTint in this case? I am confused about this.

> Source/WebCore/rendering/RenderReplaced.cpp:285
> +        LayoutRect selectionPaintingRect = localSelectionRect(false);

auto

> Source/WebCore/rendering/RenderReplaced.h:98
> +    virtual bool shouldDrawHighlight(PaintInfo&) const;

Maybe this can just return the color instead of a bool.

[Megan Gardner]

Created attachment 410883 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 410883 [details]
Patch

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

> Source/WebCore/rendering/HighlightData.cpp:169
> +    Position startPosition = rangeData.startPosition.value();
> +    Position endPosition = rangeData.endPosition.value();

auto?

> Source/WebCore/rendering/HighlightData.h:76
> +    bool setRenderRange(HighlightRangeData&);

const HighlightRangeData&

What does the return value mean? Either use an enum or a comment.

> Source/WebCore/rendering/RenderReplaced.cpp:162
> +    parentStyle = &parentRenderer->style();

Just const RenderStyle* parentStyle = here

> Source/WebCore/rendering/RenderReplaced.cpp:165
> +    for (auto& highlight : document().highlightMap().map()) {
> +        for (auto& rangeData : highlight.value->rangesData()) {
> +            if (!rangeData->startPosition || !rangeData->endPosition)

If a document has many highlight ranges, this is going to start to be a perf problem.

> Source/WebCore/rendering/RenderReplaced.cpp:176
> +            if (auto style = parentRenderer->getUncachedPseudoStyle({ PseudoId::Highlight, highlight.key }, parentStyle))

Why does this use the parent renderer for getUncachedPseudoStyle and the style? Why not this renderer?

> Source/WebCore/rendering/RenderReplaced.cpp:739
> +bool RenderReplaced::verifySelectionState(HighlightState state, HighlightData& rangeData) const

"verify" isn't really telling me what this function does.

> Source/WebCore/rendering/RenderReplaced.h:98
> +    virtual bool shouldDrawHighlight(PaintInfo&) const;

const PaintInfo&

> Source/WebCore/rendering/RenderReplaced.h:100
> +    bool verifySelectionState(HighlightState, HighlightData&) const;

const HighlightData&

[Megan Gardner]

Comment on attachment 410883 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:165
>> +            if (!rangeData->startPosition || !rangeData->endPosition)
> 
> If a document has many highlight ranges, this is going to start to be a perf problem.

Yes, I believe we might need to look into other options, but this is also how we render ranges for text, so we'll need a cross-cutting solution.

>> Source/WebCore/rendering/RenderReplaced.cpp:176
>> +            if (auto style = parentRenderer->getUncachedPseudoStyle({ PseudoId::Highlight, highlight.key }, parentStyle))
> 
> Why does this use the parent renderer for getUncachedPseudoStyle and the style? Why not this renderer?

Because I was following what I did for inlineTextBox, but I believe that actually isn't the same a this, probably no reason to not just use this renderer, since it is one.

[Tim Horton]

Comment on attachment 410883 [details]
Patch

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

>>> Source/WebCore/rendering/RenderReplaced.cpp:165
>>> +            if (!rangeData->startPosition || !rangeData->endPosition)
>> 
>> If a document has many highlight ranges, this is going to start to be a perf problem.
> 
> Yes, I believe we might need to look into other options, but this is also how we render ranges for text, so we'll need a cross-cutting solution.

DocumentMarkers have a significantly (I think) more performance-thoughtful architecture for painting/lookup time. Might be worth looking at.

[Megan Gardner]

Comment on attachment 410883 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderReplaced.cpp:165
>>>> +            if (!rangeData->startPosition || !rangeData->endPosition)
>>> 
>>> If a document has many highlight ranges, this is going to start to be a perf problem.
>> 
>> Yes, I believe we might need to look into other options, but this is also how we render ranges for text, so we'll need a cross-cutting solution.
> 
> DocumentMarkers have a significantly (I think) more performance-thoughtful architecture for painting/lookup time. Might be worth looking at.

I'll have to go back and look, but I thought there was a reason we specifically did not use these in the first place. But I'd have to refresh to defend that position.

[Tim Horton]

(In reply to Megan Gardner from comment #7)
> Comment on attachment 410883 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410883&action=review
> 
> >>>> Source/WebCore/rendering/RenderReplaced.cpp:165
> >>>> +            if (!rangeData->startPosition || !rangeData->endPosition)
> >>> 
> >>> If a document has many highlight ranges, this is going to start to be a perf problem.
> >> 
> >> Yes, I believe we might need to look into other options, but this is also how we render ranges for text, so we'll need a cross-cutting solution.
> > 
> > DocumentMarkers have a significantly (I think) more performance-thoughtful architecture for painting/lookup time. Might be worth looking at.
> 
> I'll have to go back and look, but I thought there was a reason we
> specifically did not use these in the first place. But I'd have to refresh
> to defend that position.

I'm not saying "use DocumentMarkers" necessarily. I'm saying "maybe learn something from their implementation".

[Megan Gardner]

Created attachment 410915 [details]
Patch

[Peng Liu]

Comment on attachment 410915 [details]
Patch

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

> Source/WebCore/rendering/HighlightData.cpp:176
> +    if (!startRenderer || !endRenderer)

Nit. Looks like we can bail out earlier to avoid two computeOffsetInContainerNode().

> Source/WebCore/rendering/HighlightData.h:76
> +    bool setRenderRange(HighlightRangeData&); // returns if the setting of the renderRange from the HighlightRangeData has been successful.

I guess you mean "returns true"?

> Source/WebCore/rendering/RenderReplaced.cpp:236
> +            drawHighlight = false;

Nit. Can we use one line like below?
drawHighlight = highlightColor.isVisible();

[Darin Adler]

What happened to my suggestion of removing the boolean?

[Megan Gardner]

Created attachment 411185 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411185 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:151
> +Color RenderReplaced::calculateHighlightColor(const PaintInfo& paintInfo) const

How does this work in the face of multiple overlapping ranges with non-opaque colors? Don't we have to paint each one?

> Source/WebCore/rendering/RenderReplaced.cpp:153
> +    if (document().printing() || paintInfo.paintBehavior.contains(PaintBehavior::ExcludeSelection))

The caller should be checking ExcludeSelection, not this function.

> Source/WebCore/rendering/RenderReplaced.cpp:160
> +    const RenderStyle* parentStyle = &parentRenderer->style();

Why the parent still?

> Source/WebCore/rendering/RenderReplaced.cpp:166
> +            auto highlightData = HighlightData(view());

Do we need to make a HighlightData each time through the loop?

> Source/WebCore/rendering/RenderReplaced.h:99
> +    bool verifySelectionState(HighlightState, const HighlightData&) const;

I think this could use a better name. I'm not sure what I expect it to do with a term like "verify".

[Megan Gardner]

Comment on attachment 411185 [details]
Patch

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

>> Source/WebCore/rendering/RenderReplaced.cpp:151
>> +Color RenderReplaced::calculateHighlightColor(const PaintInfo& paintInfo) const
> 
> How does this work in the face of multiple overlapping ranges with non-opaque colors? Don't we have to paint each one?

The spec is not clear on this point. It focuses more on priority, which I have not implemented yet. Baring priority, the style that was added first wins.

[Megan Gardner]

Created attachment 411197 [details]
Patch

[Darin Adler]

Comment on attachment 411197 [details]
Patch

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

I think this patch is OK, but there are quite a few things that should be improved; not sure if I should have said review+ given the Simon made comments and did not say that.

> Source/WebCore/rendering/HighlightData.cpp:166
> +bool HighlightData::setRenderRange(HighlightRangeData& rangeData)

Is there a reason you didn’t take Simon’s suggestion of making this "const HighlightRangeData&"?

> Source/WebCore/rendering/HighlightData.cpp:169
> +    auto startPosition = rangeData.startPosition.value();
> +    auto endPosition = rangeData.endPosition.value();

I suggest checking these for null here in this function, rather than doing it in the caller.

(Wow, just noticed that startPosition and endPosition in HighlightRangeData are optional. I don’t understand why. Position already has a null value and I don’t see why we also need them to be optional. I guess I can come through and clean that up separately.)

> Source/WebCore/rendering/HighlightData.cpp:175
> +    auto* startRenderer = startPosition.containerNode()->renderer();
> +    auto* endRenderer = endPosition.containerNode()->renderer();
> +    
> +    if (!startRenderer || !endRenderer)
> +        return false;

I think this also needs to check the containerNode values for nullptr (or check the positions for null, same thing). Unless there’s a guarantee they will never be null.

> Source/WebCore/rendering/RenderReplaced.cpp:157
> +            if (!rangeData->startPosition || !rangeData->endPosition)
> +                continue;

I suggest moving this check inside setRenderRange.

> Source/WebCore/rendering/RenderReplaced.cpp:223
> +    Color highlightColor = Color();

There’s no need to write "= Color()", that will happen even if we don’t specify a value explicitly.

> Source/WebCore/rendering/RenderReplaced.cpp:266
> +

Stray blank line here we should delete.

> LayoutTests/http/wpt/css/css-highlight-api/highlight-image-expected-mismatched.html:13
> +    <div id="style1"><img src="../../images/blank-highlight.png"></div>
> +    <div id="style2"><img src="../../images/blank-highlight.png"></div>
> +    <div id="style3"><img src="../../images/blank-highlight.png"></div>

In a mismatch test, there’s no point in having three different test cases here. If any of the three works, then the test will pass. If we want to test three different things, then we need to find a way to do this as a match test, or do it with three separate mismatch tests.

[Megan Gardner]

Created attachment 411236 [details]
Patch

[Wenson Hsieh]

> > Source/WebCore/rendering/HighlightData.cpp:175
> > +    auto* startRenderer = startPosition.containerNode()->renderer();
> > +    auto* endRenderer = endPosition.containerNode()->renderer();
> > +    
> > +    if (!startRenderer || !endRenderer)
> > +        return false;
> 
> I think this also needs to check the containerNode values for nullptr (or
> check the positions for null, same thing). Unless there’s a guarantee they
> will never be null.

I think it's actually possible for a Position to be non-null but have a null containerNode(), in the case where its AnchorType is either PositionIsBeforeAnchor or PositionIsAfterAnchor and the anchor node is unparented; I think checking for containerNode() would be the safer option here.

[Megan Gardner]

Created attachment 411239 [details]
Patch

[Megan Gardner]

Created attachment 411253 [details]
Patch for landing

[Darin Adler]

Comment on attachment 411239 [details]
Patch

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

> Source/WebCore/rendering/HighlightData.cpp:205
> +        if (currentRenderer == m_renderRange.start() || currentRenderer == highlightEnd)

Just realized the second half of this expression isn’t ever true — it’s already checked by the loop condition.

> Source/WebCore/rendering/HighlightData.h:76
> +    bool setRenderRange(const HighlightRangeData&); // returns true if the setting of the renderRange from the HighlightRangeData has been successful.

I would have written this comment: "// Returns true if successful."

> Source/WebCore/rendering/RenderReplaced.cpp:156
> +            

I suggest removing this blank line.

> Source/WebCore/rendering/RenderReplaced.cpp:220
> +    

I suggest not adding this blank line.

> Source/WebCore/rendering/RenderReplaced.h:98
> +    virtual Color calculateHighlightColor() const;

Not sure why this is marked virtual. I don’t see a derived class overriding it.

> LayoutTests/http/wpt/css/css-highlight-api/highlight-image-expected-mismatched.html:9
> +<head>
> +    <meta charset="utf-8">
> +    <title>Multiple custom highlight pseudo elements.</title>
> +    <link rel="help" href="https://drafts.csswg.org/css-highlight-api-1/#creating-highlights">
> +    <link rel="match" href="highlight-text-expected.html">
> +    <meta name="assert" content="Multiple highlights should be able to be specified.">
> +</head>

None of this is needed in the reference file; but maybe WPT tests normally do repeat these?

> LayoutTests/http/wpt/css/css-highlight-api/highlight-image-expected-mismatched.html:11
> +    <div id="style1"><img src="../../images/blank-highlight.png"></div>

Surprised that we do not need to set the display:inline-block.

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

This title seems wrong.

> LayoutTests/http/wpt/css/css-highlight-api/highlight-image.html:7
> +    <link rel="match" href="highlight-text-expected.html">

This is wrong.

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

This text does not match this test.

> LayoutTests/http/wpt/css/css-highlight-api/highlight-image.html:12
> +        #img1 {
> +            display: inline-block;
> +        }

Why is this needed?

[Simon Fraser (smfr)]

Comment on attachment 411253 [details]
Patch for landing

Clearing cq+ based on Darin's comments.

[Darin Adler]

Those could be addressed in follow-ups, but some of the things in the test do seem wrong.

[Megan Gardner]

Created attachment 411263 [details]
Patch

[EWS]

Committed r268436: <https://trac.webkit.org/changeset/268436>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411263 [details].

[Megan Gardner]

<rdar://problem/58459054>

[Megan Gardner]

Reopening to fix test name.

[Megan Gardner]

Created attachment 411355 [details]
Patch

[Megan Gardner]

Created attachment 411356 [details]
Patch

[EWS]

Committed r268487: <https://trac.webkit.org/changeset/268487>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411356 [details].