Bug 173860 - [iOS DnD] Text indicators for dragged links should always be legible if the link is legible
Summary: [iOS DnD] Text indicators for dragged links should always be legible if the l...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-26 20:47 PDT by Wenson Hsieh
Modified: 2017-07-03 19:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.62 KB, patch)
2017-06-26 20:58 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2017-06-26 22:53 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2017-06-30 16:25 PDT, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (7.66 KB, patch)
2017-06-30 18:26 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2017-06-26 20:47:34 PDT
<rdar://problem/32974385>
Comment 1 Wenson Hsieh 2017-06-26 20:58:06 PDT
Created attachment 313893 [details]
Patch
Comment 2 Tim Horton 2017-06-26 21:08:22 PDT
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.
Comment 3 Tim Horton 2017-06-26 21:09:21 PDT
(Test on pages with tight leading, or links with no space between them and text.
Comment 4 Wenson Hsieh 2017-06-26 21:19:24 PDT
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.
Comment 5 Wenson Hsieh 2017-06-26 21:47:58 PDT
Comment on attachment 313893 [details]
Patch

I'll be going with a more nuanced approach here.
Comment 6 Wenson Hsieh 2017-06-26 22:53:25 PDT
Created attachment 313899 [details]
Patch
Comment 7 Tim Horton 2017-06-26 23:08:51 PDT
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
Comment 8 Wenson Hsieh 2017-06-30 16:25:19 PDT
Created attachment 314315 [details]
Patch
Comment 9 Tim Horton 2017-06-30 16:36:26 PDT
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?
Comment 10 Wenson Hsieh 2017-06-30 16:37:17 PDT
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.
Comment 11 Tim Horton 2017-06-30 16:38:30 PDT
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.
Comment 12 Wenson Hsieh 2017-06-30 18:26:03 PDT
Created attachment 314343 [details]
Patch for landing
Comment 13 Wenson Hsieh 2017-06-30 18:45:08 PDT
Comment on attachment 314343 [details]
Patch for landing

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