Bug 173860 – [iOS DnD] Text indicators for dragged links should always be legible if the link is legible

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/32974385>

[Wenson Hsieh]

Created attachment 313893 [details]
Patch

[Tim Horton]

Comment on attachment 313893 [details]
Patch

Do we really want to paintAllContent always? We’re going to end up painting text outside the requested range now (even in the simple case), leading to more weird intrusions, and also background images and stuff. I dunno. Something conditional seems less crazy.

[Tim Horton]

(Test on pages with tight leading, or links with no space between them and text.

[Wenson Hsieh]

This is true. There's certainly a chance now of grabbing bits of nearby content that would be unwanted, but I considered this better than certain cases (such as the one in this bug) where the link isn't legible at all.

Really, what would be nice to do is compare the text color of the link against the estimated background color before taking the snapshot, and only use TextIndicatorOptionPaintAllContent if the text color is too close to the estimated background color, otherwise use TextIndicatorOptionUseBoundingRectAndPaintAllContentForComplexRanges. I'm not sure what constitutes "too close" though. Maybe something like differenceSquared(textColor, backgroundColor) > minColorDifference? Seems hacky, but might work well in practice.

[Wenson Hsieh]

Comment on attachment 313893 [details]
Patch

I'll be going with a more nuanced approach here.

[Wenson Hsieh]

Created attachment 313899 [details]
Patch

[Tim Horton]

Comment on attachment 313899 [details]
Patch

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

> Source/WebCore/page/TextIndicator.cpp:238
>      for (; !!renderer; renderer = renderer->parent()) {

This is climbing upwards; what if the text color comes from a child of the common ancestor instead of a parent? Should see if Antti has any ideas about how to do this well, it seems right up his alley

[Wenson Hsieh]

Created attachment 314315 [details]
Patch

[Tim Horton]

Comment on attachment 314315 [details]
Patch

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

> Source/WebCore/page/TextIndicator.cpp:230
> +        auto& color = node->renderer()->style().color();

Any need for a renderer null check? Any chance to get here with an anonymous element?

[Wenson Hsieh]

Comment on attachment 314315 [details]
Patch

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

Thanks!

>> Source/WebCore/page/TextIndicator.cpp:230
>> +        auto& color = node->renderer()->style().color();
> 
> Any need for a renderer null check? Any chance to get here with an anonymous element?

The is<RenderText>() check above should take care of this, since is<> bails on null.

[Tim Horton]

Comment on attachment 314315 [details]
Patch

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

>>> Source/WebCore/page/TextIndicator.cpp:230
>>> +        auto& color = node->renderer()->style().color();
>> 
>> Any need for a renderer null check? Any chance to get here with an anonymous element?
> 
> The is<RenderText>() check above should take care of this, since is<> bails on null.

Ah, sure.

[Wenson Hsieh]

Created attachment 314343 [details]
Patch for landing

[Wenson Hsieh]

Comment on attachment 314343 [details]
Patch for landing

Committed r219033: <http://trac.webkit.org/changeset/219033>