Bug 169869 – Teach TextIndicator to estimate the background color of the given Range

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

Teach TextIndicator to give a best guess at the background color of the Range being snapshotted, falling back to a default (e.g. the document color) if the background is an image.

[Wenson Hsieh]

<rdar://problem/31127272>

[Wenson Hsieh]

Created attachment 304909 [details]
Patch

`wekbit-patch upload` is failing for me; attempting to upload manually.

[Wenson Hsieh]

Created attachment 304946 [details]
Rebased on master

Rebased on master

[Simon Fraser (smfr)]

Comment on attachment 304946 [details]
Rebased on master

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

> Source/WebCore/page/TextIndicator.cpp:232
> +    auto boundingRectForRange = IntRect(range.absoluteBoundingRect());

Maybe you want enclosingIntRect()?

> Source/WebCore/page/TextIndicator.cpp:244
> +        if (style.hasBackground() && style.backgroundColor() != Color::transparent)
> +            parentRendererBackgroundColors.append(style.backgroundColor());

Should this logic use visitedDependentColor to avoid leaking history state?

[Wenson Hsieh]

Comment on attachment 304946 [details]
Rebased on master

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

>> Source/WebCore/page/TextIndicator.cpp:232
>> +    auto boundingRectForRange = IntRect(range.absoluteBoundingRect());
> 
> Maybe you want enclosingIntRect()?

Oh, yes, that's right. On closer inspection, simply flooring the values (by using the IntRect constructor) is wrong here.

>> Source/WebCore/page/TextIndicator.cpp:244
>> +            parentRendererBackgroundColors.append(style.backgroundColor());
> 
> Should this logic use visitedDependentColor to avoid leaking history state?

Good catch! Fixed.

[Wenson Hsieh]

Landed in <https://trac.webkit.org/changeset/214231>