Summary: | Teach TextIndicator to estimate the background color of the given Range | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
Component: | WebKit Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, megan_gardner, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Wenson Hsieh
2017-03-19 17:43:23 PDT
Created attachment 304909 [details]
Patch
`wekbit-patch upload` is failing for me; attempting to upload manually.
Created attachment 304946 [details]
Rebased on master
Rebased on master
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? 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. Landed in <https://trac.webkit.org/changeset/214231> |