Bug 217116 – Repaint as needed when adding and removing highlights

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Repaint as needed when adding and removing highlights

[Megan Gardner]

Created attachment 410084 [details]
Patch

[Megan Gardner]

<rdar://problem/59076190>

[Simon Fraser (smfr)]

Comment on attachment 410084 [details]
Patch

Why do we need both this repaint code, and HighlightData::repaint()?

[Megan Gardner]

Because HighlightData(s) are ephemeral and only used for/while rendering in relation to CSS Highlights. They don't exist when ranges are added or removed from HighlightRangeGroup, and there isn't a direct relationship between the objects. So when we add or remove a new range, we need to mark that node as needing a repaint. Also, HighlightData::Repaint to be used specifically when HighlightData is being used for selections, which are stored an handled very differently than CSS Highlights. HighlightData used to be called SelectionData, but when I started to overload its usage for CSS Highlights, I changed the name. There is probably still work to be done to clean up this class and have the names be more reflective of their current use.

[Megan Gardner]

Created attachment 410151 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 410151 [details]
Patch

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

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:84
> +        repaintRange(range);

Can we avoid these repaints if the HighlightRangeGroup has no associated styles?

[Megan Gardner]

Comment on attachment 410151 [details]
Patch

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

>> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:84
>> +        repaintRange(range);
> 
> Can we avoid these repaints if the HighlightRangeGroup has no associated styles?

Not the way the data is structured currently, but when I change to match the spec (where the name is stored along side the ranges), then it will be possible, I'll file a bug to track the work.

[Megan Gardner]

Created attachment 410173 [details]
Patch

[Peng Liu]

Comment on attachment 410173 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        When adding and removing psudo highlights, make sure that the

A typo?
s/psudo/pseudo/

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:91
> +    for (Ref<HighlightRangeData>& data : m_rangesData)

Can we use auto here?

> LayoutTests/http/wpt/css/css-highlight-api/highlight-text-repaint.html:62
> +    <script>

Can be removed?

[Megan Gardner]

Created attachment 410257 [details]
Patch

[Megan Gardner]

Created attachment 410290 [details]
Patch for landing

[EWS]

Committed r267863: <https://trac.webkit.org/changeset/267863>

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

[Darin Adler]

Comment on attachment 410290 [details]
Patch for landing

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

Some follow-up ideas.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:58
> +static void repaintRange(const StaticRange& range)

Could change this to SimpleRange.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:71
> +    if (is_gt(ordering)) {
> +        startNode = &range.endContainer();
> +        endNode = &range.startContainer();
> +    }

Could do this:

    if (is_gt(ordering))
        std::swap(startNode, endNode);

Unclear why we are handling the case where they end up out of order, but not the case where one end or the other isn’t in the document any more, which can also happen. If we can prevent that from happening by updating the ranges as the DOM is mutated, then we can also prevent this out-of-order case from happening at that time as well.

> Source/WebCore/Modules/highlight/HighlightRangeGroup.cpp:74
> +    auto node = startNode;
> +    while (node != endNode) {

Can clean this up to use intersectingNodes. Likely would not need the is_eq check above at all if we did that, since I think it does the right thing.