Bug 205318 – Paint highlights specified in CSS Highlight API

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

[Megan Gardner]

Paint highlights specified in CSS Highlight API

[Megan Gardner]

Created attachment 385842 [details]
Patch

[Megan Gardner]

Created attachment 385903 [details]
Patch

[Megan Gardner]

Created attachment 385909 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 385909 [details]
Patch

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

Now that you can actually test the highlight API's results, I think you need some more tests:
1. Replace a HighlightRangeGroup:
    CSS.highlights.set("example-highlight1", highlightRangeGroup1);
   // make another one
    CSS.highlights.set("example-highlight1", highlightRangeGroup2);

2. Test the ::highlight cascade:
::highlight(example-highlight1) {...}
#foo::highlight(example-highlight1) {...}

testing things inside and outside #foo, and with the same and different named highlights.

> Source/WebCore/rendering/InlineTextBox.cpp:208
> +RenderObject::SelectionState InlineTextBox::rendererHighlightState(RenderObject* startObject, unsigned /*startPos*/, RenderObject* endObject, unsigned endPos) const

Can the arguments be RenderObject&?

> Source/WebCore/rendering/InlineTextBox.cpp:231
> +    if (&renderer() == startObject || renderer().isDescendantOf(startObject)) {
> +        if (startObject && endObject && startObject == endObject)
> +            return RenderObject::SelectionBoth;
> +        if (startObject)
> +            return RenderObject::SelectionStart;
> +    }
> +    if (&renderer() == endObject || renderer().isDescendantOf(endObject))
> +        return RenderObject::SelectionEnd;
> +
> +    RenderObject* selectionEnd = nullptr;
> +    auto* selectionDataEnd = endObject;
> +    if (selectionDataEnd)
> +        selectionEnd = rendererAfterPosition(*selectionDataEnd, endPos);
> +    SelectionIterator selectionIterator(startObject);
> +    for (auto* currentRenderer = startObject; currentRenderer && currentRenderer != endObject; currentRenderer = selectionIterator.next()) {
> +        if (currentRenderer == startObject || currentRenderer == endObject)
> +            continue;
> +        if (!currentRenderer->canBeSelectionLeaf())
> +            continue;
> +        if (&renderer() == currentRenderer)
> +            return RenderObject::SelectionInside;
> +    }

Was this copied from somewhere, and can it be shared with selection code?

> Source/WebCore/rendering/InlineTextBox.cpp:235
> +RenderObject::SelectionState InlineTextBox::highlightState(RenderObject* startObject, unsigned startPos, RenderObject* endObject, unsigned endPos) const

Can the arguments be RenderObject&?

> Source/WebCore/rendering/InlineTextBox.cpp:256
> +        // The position after a hard line break is considered to be past its end.
> +        ASSERT(start() + len() >= (isLineBreak() ? 1 : 0));
> +        unsigned lastSelectable = start() + len() - (isLineBreak() ? 1 : 0);
> +
> +        bool start = (state != RenderObject::SelectionEnd && startPos >= m_start && startPos < m_start + m_len);
> +        bool end = (state != RenderObject::SelectionStart && endPos > m_start && endPos <= lastSelectable);
> +        if (start && end)
> +            state = RenderObject::SelectionBoth;
> +        else if (start)
> +            state = RenderObject::SelectionStart;
> +        else if (end)
> +            state = RenderObject::SelectionEnd;
> +        else if ((state == RenderObject::SelectionEnd || startPos < m_start) && (state == RenderObject::SelectionStart || endPos > lastSelectable))
> +            state = RenderObject::SelectionInside;
> +        else if (state == RenderObject::SelectionBoth)
> +            state = RenderObject::SelectionNone;
> +    }

Please share this code with the existing selection code.

> Source/WebCore/rendering/InlineTextBox.cpp:632
> +                    if (!highlightMarkedTexts.isEmpty())
> +                    markedTexts.appendVector(WTFMove(highlightMarkedTexts));

Bad indent.

> Source/WebCore/rendering/InlineTextBox.cpp:744
> +std::pair<unsigned, unsigned> InlineTextBox::highlightStartEnd(RenderObject* start, unsigned startPos, RenderObject* end, unsigned endPos) const

Can the arguments be RenderObject&?

> Source/WebCore/rendering/InlineTextBox.cpp:865
> +        {

Brace on previous line.

> Source/WebCore/rendering/InlineTextBox.cpp:866
> +        auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName}, &parent()->renderer().style());

Indent.

> Source/WebCore/rendering/InlineTextBox.cpp:1099
> +            Position endPos = createLegacyEditingPosition(staticRange->endContainer(), staticRange->endOffset());

Can we not use "legacy" things in new code? What's the replacement?

> Source/WebCore/rendering/InlineTextBox.cpp:1103
> +                int startOffset = startPos.deprecatedEditingOffset();

More deprecated.

> Source/WebCore/rendering/InlineTextBox.h:124
> +    std::pair<unsigned, unsigned> highlightStartEnd(RenderObject*, unsigned, RenderObject*, unsigned) const;

The arguments should get names.

> Source/WebCore/rendering/InlineTextBox.h:138
> +    RenderObject::SelectionState rendererHighlightState(RenderObject* , unsigned , RenderObject* , unsigned) const;
> +    RenderObject::SelectionState highlightState(RenderObject*, unsigned, RenderObject*, unsigned) const;

The names don't clarify how these are different.

> Source/WebCore/rendering/MarkedText.cpp:83
> -                    result.append({ offsetSoFar, offsets[i].value, offsets[*frontmost].markedText->type, offsets[*frontmost].markedText->marker });
> +                    result.append({ offsetSoFar, offsets[i].value, offsets[*frontmost].markedText->type, offsets[*frontmost].markedText->marker, offsets[*frontmost].markedText->highlightName});
>              } else {
>                  // The appended marked texts may not be in paint order. We will fix this up at the end of this function.
>                  for (unsigned j = 0; j < i; ++j) {
>                      if (!processedMarkedTexts.contains(offsets[j].markedText))
> -                        result.append({ offsetSoFar, offsets[i].value, offsets[j].markedText->type, offsets[j].markedText->marker });
> +                        result.append({ offsetSoFar, offsets[i].value, offsets[j].markedText->type, offsets[j].markedText->marker, offsets[j].markedText->highlightName });

I don't really know what's going on here. If text has a combination of highlights and spelling underlines, do we need to subdivide based on both, or should we run subdivision separately for spelling underlines vs. highlights?

> Source/WebCore/rendering/SelectionRangeData.h:43
> +class SelectionIterator {

I think this needs a better name now that you exposed it. It's not iterating over the selection.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:6
> +  <link rel="help" href="https://drafts.csswg.org/css-highlight-api-1/#creating-highlights">

This is not testing #creating-highlights, it's testing the painting I think?

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:8
> +  <meta name="assert" content="Highlights should be able to be specified across elements.">

Not quite sure what this means.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:30
> +        <span id="text1">One two three</span>
> +        <span id="text2">four five six</span>
> +        <span id="text3">seven eight nine</span>
> +        <span id="text4">ten eleven twelve</span>
> +        <span id="text5">thirteen fourteen fifteen</span>

At least one of these spans should not break on whitespace for better testing.

[Ryosuke Niwa]

Comment on attachment 385909 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Render highlights when present, similar to the way we render selection.

This description should appear below "reviewed by" line but above the list of tests.

> Source/WebCore/ChangeLog:1764
> +        Move some helpful functions so they are accessible.
> +

What's up with this change?

> Source/WebCore/rendering/InlineTextBox.cpp:591
> +        if (RuntimeEnabledFeatures::sharedFeatures().highlightAPIEnabled()) {

Instead of checking this at every caller, collectMarkedTextsForHighlights should have that check builtin.

> Source/WebCore/rendering/InlineTextBox.cpp:1098
> +            Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset());

I don't think this works. See what FrameSelection::updateAppearance() does.
We need to use the deepEquivalent of VisiblePosition, which is probably built at the end of each layout.
Don't create these VisiblePosition here; that would be incorrect & cause security bugs.
Instead, we ned to have some fixed time relative to updateLayout, perhaps post layout task,
where we build up all VisiblePositions needed for each highlight group's ranges.

>> Source/WebCore/rendering/InlineTextBox.cpp:1103
>> +                int startOffset = startPos.deprecatedEditingOffset();
> 
> More deprecated.

I don't think rendering code works with new non-deprecated node.

[Wenson Hsieh]

Comment on attachment 385909 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:749
> +        return { 0, 0};

Nit - Space between 0 and }

>> Source/WebCore/rendering/InlineTextBox.cpp:866
>> +        auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName}, &parent()->renderer().style());
> 
> Indent.

Also, space between markedText.highlightName and }

> Source/WebCore/rendering/InlineTextBox.cpp:888
> +                if (decorations.contains(TextDecoration::Underline)) {
> +                    style.textDecorationStyles.underlineColor = color;
> +                    style.textDecorationStyles.underlineStyle = decorationStyle;
> +                }
> +                if (decorations.contains(TextDecoration::Overline)) {
> +                    style.textDecorationStyles.overlineColor = color;
> +                    style.textDecorationStyles.overlineStyle = decorationStyle;
> +                }
> +                if (decorations.contains(TextDecoration::LineThrough)) {
> +                    style.textDecorationStyles.linethroughColor = color;
> +                    style.textDecorationStyles.linethroughStyle = decorationStyle;
> +                }

You could deduplicate logic in these if statements by using a single if statement, with decorations.containsAny().

> Source/WebCore/rendering/InlineTextBox.cpp:1110
> +                    markedTexts.append({ highlightStart, highlightEnd, MarkedText::Highlight, nullptr, highlightGroup.key});

Nit - Space between key and }

> Source/WebCore/rendering/MarkedText.cpp:31
> +#include <wtf/text/WTFString.h>

If the header includes WTFString.h, you shouldn't need this #include

> Source/WebCore/rendering/MarkedText.cpp:78
> +                    result.append({ offsetSoFar, offsets[i].value, offsets[*frontmost].markedText->type, offsets[*frontmost].markedText->marker, offsets[*frontmost].markedText->highlightName});

Nit - space between highlightName and }

> Source/WebCore/rendering/MarkedText.h:56
> +    String highlightName { };

Nit - just String highlightName; is fine here.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements-expected.html:20
> +<body>
> +    <body>

Multiple body tags.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:25
> +<body>
> +    <body>

Multiple body tags.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-expected.html:20
> +<body>
> +    <body>

Multiple body tags.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text.html:25
> +<body>
> +    <body>

Multiple body tags.

[Megan Gardner]

Comment on attachment 385909 [details]
Patch

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

>> Source/WebCore/rendering/InlineTextBox.cpp:1098
>> +            Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset());
> 
> I don't think this works. See what FrameSelection::updateAppearance() does.
> We need to use the deepEquivalent of VisiblePosition, which is probably built at the end of each layout.
> Don't create these VisiblePosition here; that would be incorrect & cause security bugs.
> Instead, we ned to have some fixed time relative to updateLayout, perhaps post layout task,
> where we build up all VisiblePositions needed for each highlight group's ranges.

Are you envisioning some sort of parallel grouping of VisiblePositions? I'm not sure when you are thinking these should be built up. 
But this does work at least for simple websites/test cases.

>> Source/WebCore/rendering/MarkedText.h:56
>> +    String highlightName { };
> 
> Nit - just String highlightName; is fine here.

you sure? I thought I needed a default constructor because this struct only has the implicit constructor, and if I want to make sure this is initialized, I thought I needed to do this explicitly?

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:9
> +    <style>

Just that they can start in one element and end in another.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-across-elements.html:31
> +

The test is really about the highlight ranges and them crossing each of these spans. I'm not sure what breaking the spans on non-whitespace would help test about highlights.

[Chris Dumez]

Comment on attachment 385909 [details]
Patch

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

>>> Source/WebCore/rendering/MarkedText.h:56
>>> +    String highlightName { };
>> 
>> Nit - just String highlightName; is fine here.
> 
> you sure? I thought I needed a default constructor because this struct only has the implicit constructor, and if I want to make sure this is initialized, I thought I needed to do this explicitly?

I think Wenson is right. The implicit constructor should call the String's default constructor. type and endOffset not having default values above is fishy though.

[Megan Gardner]

Created attachment 386032 [details]
Patch

[Megan Gardner]

I plan on renaming SelectionRangeData.cpp and SelectionRangeData.h, but anticipated some bikesheding around the name, so I am putting off the expensive file renames until PsudoElementRangeData is approved, or we come up with a better name.

[Megan Gardner]

(In reply to Megan Gardner from comment #9)
> Created attachment 386032 [details]
> Patch

Ok, I'll pull it out then. I actually plan on make explicit constructors for this struct as it's gotten complicated enough that I think it would benefit from explicit constructors, but I'll do that in a follow up patch, as this is already getting out of control.

[Megan Gardner]

(In reply to Megan Gardner from comment #11)
> (In reply to Megan Gardner from comment #9)
> > Created attachment 386032 [details]
> > Patch
> 
> Ok, I'll pull it out then. I actually plan on make explicit constructors for
> this struct as it's gotten complicated enough that I think it would benefit
> from explicit constructors, but I'll do that in a follow up patch, as this
> is already getting out of control.

Tried to pull it out, it won't build, so I'll leave it in and deal with it when I make the explicit constructors.

[Ryosuke Niwa]

Comment on attachment 386032 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:178
> +    updateEllipsisState(state);

Is there really a point in separating this into its own helper function? It's only used once here.
Why don't we just inline the code here instead?

> Source/WebCore/rendering/InlineTextBox.cpp:182
> +RenderObject::SelectionState InlineTextBox::pseudoElementState(RenderObject::SelectionState state, PseudoElementRangeData& rangeData) const

This is too generic of name. We're trying to compute the highlight state really so I'd call this highlightPseudoElementState instead.

> Source/WebCore/rendering/InlineTextBox.cpp:1022
> +    if (!RuntimeEnabledFeatures::sharedFeatures().highlightAPIEnabled()) {

Nit: no curly braces around a single line statement.

> Source/WebCore/rendering/InlineTextBox.cpp:1034
> +        // <MMG> get peudoStyle here to save calculations

Remove or use FIXME.

> Source/WebCore/rendering/InlineTextBox.cpp:1047
> +                    

Nit: whitespace.

> Source/WebCore/rendering/InlineTextBox.cpp:1048
> +                    PseudoElementRangeData highlightData = PseudoElementRangeData(renderer().view());

Use auto? as in `auto highlightData = ~`.

> Source/WebCore/rendering/InlineTextBox.cpp:1050
> +                    

Nit: Whitespace.

> Source/WebCore/rendering/SelectionRangeData.cpp:46
> +struct PseudoElementData {

PseudoElementData is too generic of a name. I wouldn't call selection or highlight groups as a pseudo element.
It usually refers to things like ::before & ::after: https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements

I'd call it RangePseudoElementData or simply HighlightData instead
since we can think of selection highlight as a type of highlight.

> Source/WebCore/rendering/SelectionRangeData.cpp:162
> +    // <MMG> these were pulled from elswhere in this file, refactor to make this work. Hopefully.

Remove or use FIXME.

> Source/WebCore/rendering/SelectionRangeData.h:43
> -class SelectionRangeData {
> +class PseudoElementRangeData {

Again, this is too generic of name. It should be probably something like HighlightRangeData.

> ChangeLog:1
> [phone]  Megan Gardner  <[e-mail]>

Revert

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60
> -               BuildableName = "libANGLE-shared.dylib"
> +               BuildableName = "libANGLE.a"

Revert.

[Ryosuke Niwa]

(In reply to Megan Gardner from comment #10)
> I plan on renaming SelectionRangeData.cpp and SelectionRangeData.h, but
> anticipated some bikesheding around the name, so I am putting off the
> expensive file renames until PsudoElementRangeData is approved, or we come
> up with a better name.

I don't think PsudoElementRangeData is a good name. Pseudo element is way more than just selection or highlight groups. They refer to things like ::first-line, ::before, ::after, etc... https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements

It would be too confusing to use PseudoElement prefix to refer to data structures related to selection & highlight groups.

[Wenson Hsieh]

Comment on attachment 386032 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:577
> +                auto highlightMarkedTexts = collectMarkedTextsForHighlights(TextPaintPhase::Foreground);
> +                if (!highlightMarkedTexts.isEmpty())
> +                    markedTexts.appendVector(WTFMove(highlightMarkedTexts));

Nit - this is indented by one extra level.

> Source/WebCore/rendering/InlineTextBox.cpp:819
> +                if (decorations.containsAny({ TextDecoration::Underline, TextDecoration::Overline, TextDecoration::Overline })) {

This should be TextDecoration::Underline, TextDecoration::Overline, TextDecoration::LineThrough

[Megan Gardner]

Created attachment 386125 [details]
Patch

[Megan Gardner]

Took out the rename, working on the leaks, but figured I could get some feedback in the meantime.

[Sam Weinig]

Comment on attachment 386125 [details]
Patch

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

Excited to see this coming together!

> Source/WebCore/rendering/InlineTextBox.cpp:1030
> +    for (auto& highlightGroup : renderer().document().highlightMap().map()) {

You can make sue of destructing here. for (auto& [highlightKey, highlightGroup] : renderer().document().highlightMap().map()) {

> Source/WebCore/rendering/InlineTextBox.cpp:1031
> +        auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, highlightGroup.key }, &parent()->renderer().style());

Not sure if parent()->renderer()/parent()->renderer()->style() is expensive, but probably worth hoisting access outside the loop.

> Source/WebCore/rendering/InlineTextBox.cpp:1035
> +                Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset());
> +                Position endPos = createLegacyEditingPosition(staticRange->endContainer(), staticRange->endOffset());

Why is new code using LegacyEditingPositions? By the name of it, it seems like someone didn't want it to continued to be used.

> Source/WebCore/rendering/InlineTextBox.cpp:1044
> +                    auto highlightData = SelectionRangeData(renderer().view());

Probably want to use constructor syntax rather than assignment.

> Source/WebCore/rendering/InlineTextBox.cpp:1045
> +                    highlightData.setContext({startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset)});

Please add spaces after / before the braces.

> Source/WebCore/rendering/InlineTextBox.h:138
> +    RenderObject::SelectionState rendererHighlightState(RenderObject* , unsigned , RenderObject* , unsigned) const;
> +    RenderObject::SelectionState highlightState(RenderObject*, unsigned, RenderObject*, unsigned) const;

Do these functions have implementations? They seem to be missing from the patch.

> Source/WebCore/rendering/InlineTextBox.h:148
> +    void updateEllipsisState(RenderObject::SelectionState);

Do this function have an implementation? It seems to be missing from the patch.

[Ryosuke Niwa]

(In reply to Sam Weinig from comment #18)
> Comment on attachment 386125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386125&action=review
>
> > Source/WebCore/rendering/InlineTextBox.cpp:1035
> > +                Position startPos = createLegacyEditingPosition(staticRange->startContainer(), staticRange->startOffset());
> > +                Position endPos = createLegacyEditingPosition(staticRange->endContainer(), staticRange->endOffset());
> 
> Why is new code using LegacyEditingPositions? By the name of it, it seems
> like someone didn't want it to continued to be used.

Unfortunately, the rendering code hasn't been updated to handle non-legacy Position offsets. We should fix that but we decided it shouldn't be a blocker for this feature.

[Megan Gardner]

Created attachment 386157 [details]
Patch

[Megan Gardner]

Created attachment 386168 [details]
Patch

[Megan Gardner]

Created attachment 386171 [details]
Patch

[Wenson Hsieh]

Comment on attachment 386171 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:783
> +    highlightMap().clear();

Nit - it looks like this will create m_highlightMap if it didn't already exist; we might want to consider something like this instead:

if (m_highlightMap)
    m_highlightMap->clear();

[Megan Gardner]

Created attachment 386244 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 386244 [details]
Patch

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

> Source/WebCore/rendering/InlineTextBox.cpp:162
> +    // If there are ellipsis following, make sure their selection is updated.
> +    if (m_truncation != cNoTruncation && root().ellipsisBox()) {

It's unclear why ellipsis won't affect CSS highlight groups. Did we test this?
If not, can we file a bug somewhere to track it or add a FIXME?

> Source/WebCore/rendering/InlineTextBox.cpp:804
> +    case MarkedText::Highlight:
> +        {

{ should appear after : in the previous line like so:
case MarkedText::Highlight: {

> Source/WebCore/rendering/InlineTextBox.cpp:807
> +            auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName }, &parent()->renderer().style());
> +            
> +            if (renderStyle) {

Declare renderStyle inside if as in:
if (auto renderStyle = ~)

> Source/WebCore/rendering/InlineTextBox.cpp:1021
> +    

Nit: whitespace.

> Source/WebCore/rendering/InlineTextBox.cpp:1025
> +    

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:1034
> +        if (renderStyle) {

Continue when renderStyle is null instead to avoid deep nesting.

> Source/WebCore/rendering/InlineTextBox.cpp:1045
> +                    

Nit: whitespace.

> Source/WebCore/rendering/InlineTextBox.cpp:1047
> +                    highlightData.setContext({startRenderer, endRenderer, static_cast<unsigned>(startOffset), static_cast<unsigned>(endOffset)});

Just FYI, there is no guarantee that offset is anything sane. It could be way higher than the actual number of characters or the children.

Also, why don't we continue when either startRenderer or endRenderer is null here instead of checking it in setContext?

> Source/WebCore/rendering/InlineTextBox.cpp:1048
> +                    

Ditto.

> Source/WebCore/rendering/InlineTextBox.h:138
> +    

Nit: whitespace.

> Source/WebCore/rendering/SelectionRangeData.cpp:159
> +    if ((context.start() && !context.end()) || (context.end() && !context.start()))

Just do: !!context.start() != !!context.end().
But we should just get rid of this code entirely by checking this condition in the caller instead.
Then we can assert that both start & end are set here.

> Source/WebCore/rendering/SelectionRangeData.cpp:166
> +    if (&renderer == m_selectionContext.start() || renderer.isDescendantOf(m_selectionContext.start())) {

Why do we need this check? SelectionRangeData::apply doesn't do this.

> Source/WebCore/rendering/SelectionRangeData.cpp:168
> +        if (m_selectionContext.start() && m_selectionContext.end() && m_selectionContext.start() == m_selectionContext.end())
> +            return RenderObject::SelectionBoth;

How could this be correct?
It would mean that every descendent of a render object which is both start & end would be marked as SelectionBoth.
Perhaps you had to do this because you're not currently used canonicalized position out of VisiblePosition?

This most certainly needs to be fixed in a follow up. I suggest adding FIXME here at least.

> Source/WebCore/rendering/SelectionRangeData.cpp:170
> +        if (m_selectionContext.start())
> +            return RenderObject::SelectionStart;

Ditto.

> Source/WebCore/rendering/SelectionRangeData.cpp:173
> +    if (&renderer == m_selectionContext.end() || renderer.isDescendantOf(m_selectionContext.end()))
> +        return RenderObject::SelectionEnd;

Ditto.

[Megan Gardner]

Created attachment 386270 [details]
Patch

[Megan Gardner]

Comment on attachment 386244 [details]
Patch

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

>> Source/WebCore/rendering/InlineTextBox.cpp:162
>> +    if (m_truncation != cNoTruncation && root().ellipsisBox()) {
> 
> It's unclear why ellipsis won't affect CSS highlight groups. Did we test this?
> If not, can we file a bug somewhere to track it or add a FIXME?

They probably are affected, I didn't test this yet, but this code is in the 'wrong' place, so I'll file a radar to just clean it all up.

>> Source/WebCore/rendering/SelectionRangeData.cpp:166
>> +    if (&renderer == m_selectionContext.start() || renderer.isDescendantOf(m_selectionContext.start())) {
> 
> Why do we need this check? SelectionRangeData::apply doesn't do this.

It doesn't work if we don't check descendants. I expect this is a fallout of not using VisiblePositions yet, I will review when I make that fix.

>> Source/WebCore/rendering/SelectionRangeData.cpp:168
>> +            return RenderObject::SelectionBoth;
> 
> How could this be correct?
> It would mean that every descendent of a render object which is both start & end would be marked as SelectionBoth.
> Perhaps you had to do this because you're not currently used canonicalized position out of VisiblePosition?
> 
> This most certainly needs to be fixed in a follow up. I suggest adding FIXME here at least.

Yes, I didn't read this one before I responded to the one above. I'll add a note about the radar I've already filed.

[Ryosuke Niwa]

(In reply to Megan Gardner from comment #27)
> Comment on attachment 386244 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386244&action=review
> 
> >> Source/WebCore/rendering/InlineTextBox.cpp:162
> >> +    if (m_truncation != cNoTruncation && root().ellipsisBox()) {
> > 
> > It's unclear why ellipsis won't affect CSS highlight groups. Did we test this?
> > If not, can we file a bug somewhere to track it or add a FIXME?
> 
> They probably are affected, I didn't test this yet, but this code is in the
> 'wrong' place, so I'll file a radar to just clean it all up.

Please also file a WebKit bugzilla bug & relate to this Bugzilla bug.

> >> Source/WebCore/rendering/SelectionRangeData.cpp:168
> >> +            return RenderObject::SelectionBoth;
> > 
> > How could this be correct?
> > It would mean that every descendent of a render object which is both start & end would be marked as SelectionBoth.
> > Perhaps you had to do this because you're not currently used canonicalized position out of VisiblePosition?
> > 
> > This most certainly needs to be fixed in a follow up. I suggest adding FIXME here at least.
> 
> Yes, I didn't read this one before I responded to the one above. I'll add a
> note about the radar I've already filed.

Ditto.

[Ryosuke Niwa]

Comment on attachment 386270 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:784
> +    if (m_highlightMap)
> +        m_highlightMap->clear();

Nice!

> Source/WebCore/rendering/InlineTextBox.cpp:805
> +    case MarkedText::Highlight: {
> +        if (auto renderStyle = parent()->renderer().getUncachedPseudoStyle({ PseudoId::Highlight, markedText.highlightName }, &parent()->renderer().style())) {

Now that you've wrapped the whole thing, the outer { ~ } is unnecessary.

> Source/WebCore/rendering/SelectionRangeData.cpp:165
> +    // FIXME: we shouldln't have to check that a renderer is a descendant of the render node from the range. This is likely because we aren't using VisiblePositions yet. Planned fix in a followup: <rdar://problem/58095923>

Please split this comment into two lines since it's really long.
And also file a Bugzilla bug & prefer to that instead.
People in the open source community won't be able to follow this otherwise.

> LayoutTests/imported/w3c/ChangeLog:7
> +

Please state from where thees tests are imported, and as of which Git hash.

[Megan Gardner]

Comment on attachment 386270 [details]
Patch

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

>> LayoutTests/imported/w3c/ChangeLog:7
>> +
> 
> Please state from where thees tests are imported, and as of which Git hash.

They aren't actually imported, I wrote them, with the intent of upstreaming them eventually, as there are no web platform tests yet.

[Ryosuke Niwa]

Comment on attachment 386270 [details]
Patch

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

>>> LayoutTests/imported/w3c/ChangeLog:7
>>> +
>> 
>> Please state from where thees tests are imported, and as of which Git hash.
> 
> They aren't actually imported, I wrote them, with the intent of upstreaming them eventually, as there are no web platform tests yet.

Okay, then they need to be under LayoutTests/http/wpt or just to LayoutTests/highlight.
There is no need to put them in different directories.

> LayoutTests/highlight/highlight-world-leak.html:5
> +<head>
> +<script src="../resources/js-test-pre.js"></script>
> +</head>

Put this script element above <script> below instead of having it in head.

> LayoutTests/highlight/highlight-world-leak.html:21
> +        }, 10);

Hm... this is dangerous approach. We have a conservative GC so it's possible that the document won't be collected.
As I said previously in person, we should be creating ~20 framers and make sure at least a few of them e.g. 5 of them are collected instead.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-cascade.html:4
> +  <meta charset="utf-8">

Please don't mix 2 space indentation with 4 space indentation.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text-replace.html:4
> +  <meta charset="utf-8">

Ditto about identation.

> LayoutTests/imported/w3c/web-platform-tests/css/css-highlight-api/highlight-text.html:4
> +  <meta charset="utf-8">

Ditto.

[Megan Gardner]

Created attachment 386279 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 386279 [details]
Patch for landing

Clearing flags on attachment: 386279

Committed r253857: <https://trac.webkit.org/changeset/253857>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58130458>

[Michael Catanzaro]

Hey Megan, sorry I'm late with this but there is one warning:

[497/899] Building CXX object Source/W...ources/UnifiedSource-043dd90b-15.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-15.cpp:7:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/rendering/SelectionRangeData.cpp: In member function ‘WebCore::RenderObject::SelectionState WebCore::SelectionRangeData::selectionStateForRenderer(WebCore::RenderObject&)’:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/rendering/SelectionRangeData.cpp:179:19: warning: variable ‘selectionEnd’ set but not used [-Wunused-but-set-variable]
  179 |     RenderObject* selectionEnd = nullptr;
      |                   ^~~~~~~~~~~~

I think the solution is:

diff --git a/Source/WebCore/rendering/SelectionRangeData.cpp b/Source/WebCore/rendering/SelectionRangeData.cpp
index d2188905703..ef82b36d5d4 100644
--- a/Source/WebCore/rendering/SelectionRangeData.cpp
+++ b/Source/WebCore/rendering/SelectionRangeData.cpp
@@ -176,10 +176,6 @@ RenderObject::SelectionState SelectionRangeData::selectionStateForRenderer(Rende
     if (&renderer == m_selectionContext.end())
         return RenderObject::SelectionEnd;
 
-    RenderObject* selectionEnd = nullptr;
-    auto* selectionDataEnd = m_selectionContext.end();
-    if (selectionDataEnd)
-        selectionEnd = rendererAfterOffset(*selectionDataEnd, m_selectionContext.endOffset().value());
     SelectionIterator selectionIterator(m_selectionContext.start());
     for (auto* currentRenderer = m_selectionContext.start(); currentRenderer && currentRenderer != m_selectionContext.end(); currentRenderer = selectionIterator.next()) {
         if (currentRenderer == m_selectionContext.start() || currentRenderer == m_selectionContext.end())


Do you agree? Or are there important side effects to rendererAfterOffset()? If so, we can cast the return value to (void) to suppress the warning.

[Megan Gardner]

No, I expect that is was an error as is supposed to be used in the next line, as that is what happens in the code that this was pulled from. Please do not commit that fix. I will look into this soon.