Summary: | [iOS DnD] Text indicators for dragged links should always be legible if the link is legible | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | WebCore Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aestes, bdakin, commit-queue, thorton, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2017-06-26 20:47:34 PDT
Created attachment 313893 [details]
Patch
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.
(Test on pages with tight leading, or links with no space between them and text. 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 on attachment 313893 [details]
Patch
I'll be going with a more nuanced approach here.
Created attachment 313899 [details]
Patch
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 Created attachment 314315 [details]
Patch
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 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 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. Created attachment 314343 [details]
Patch for landing
Comment on attachment 314343 [details] Patch for landing Committed r219033: <http://trac.webkit.org/changeset/219033> |