Bug 169309 – Add TextIndicator support for providing a snapshot excluding selected content

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

[Wenson Hsieh]

<rdar://problem/30883525>

[Wenson Hsieh]

Created attachment 303731 [details]
Patch

[Tim Horton]

Comment on attachment 303731 [details]
Patch

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

TextIndicator parts are fine with me but we should get myles and smfr to take a peek at the other bits.

> Source/WebCore/ChangeLog:58
> +        Teach RenderReplaced to respect PaintBehaviorExcludeSelection by bailing from painting if it is selected and the
> +        paint behavior is enabled.

What happens for <div> with background color (block /or/ inline-block)? One of the reasons I think we need smfr's input.

> Source/WebCore/page/TextIndicator.h:52
> +enum TextIndicatorOption : uint16_t {

Ahhhh this is when you know things have gotten bad.

[Wenson Hsieh]

Created attachment 303848 [details]
Patch

[Tim Horton]

Comment on attachment 303848 [details]
Patch

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

> Source/WebCore/page/TextIndicator.cpp:178
> +        auto snapshotRect = enclosingIntRect(frame.view()->visualViewportRect());

Why visualViewportRect and not the snapshotRect? Does this mean that this snapshot is a different size than the others? That kind of breaks the model (e.g. you couldn't transition between the snapshots, which was the point of data.contentImageWithHighlight).

> Source/WebCore/platform/graphics/FontCascade.cpp:352
> +float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned to, HashSet<const Font*>* fallbackFonts, float* outWidthBeforeRange, float* outWidthAfterRange) const

Amazed that this is something we need to implement fresh.

> Source/WebCore/rendering/InlineTextBox.cpp:567
> +            if (!isHorizontal()) {

What about vertical-bt :|

[Wenson Hsieh]

Comment on attachment 303848 [details]
Patch

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

>> Source/WebCore/page/TextIndicator.cpp:178
>> +        auto snapshotRect = enclosingIntRect(frame.view()->visualViewportRect());
> 
> Why visualViewportRect and not the snapshotRect? Does this mean that this snapshot is a different size than the others? That kind of breaks the model (e.g. you couldn't transition between the snapshots, which was the point of data.contentImageWithHighlight).

We talked about this in person -- we actually do need the full visible contents in this case, so let's rename TextIndicatorOptionIncludeSnapshotWithoutSelection to something more explicit, like TextIndicatorOptionIncludeSnapshotOfAllVisibleContentWithoutSelection

>> Source/WebCore/platform/graphics/FontCascade.cpp:352
>> +float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned to, HashSet<const Font*>* fallbackFonts, float* outWidthBeforeRange, float* outWidthAfterRange) const
> 
> Amazed that this is something we need to implement fresh.

This is, indeed, the case. Previously, I took subruns of the whole text run and computed their widths which just used FontCascade::width, but Myles informed my that the Right Thing to do here is to use width iterators (either complex or simple, depending on the font) and advance to the appropriate location in the entire run.

>> Source/WebCore/rendering/InlineTextBox.cpp:567
>> +            if (!isHorizontal()) {
> 
> What about vertical-bt :|

Will add as a // FIXME

[Wenson Hsieh]

Created attachment 303872 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 303872 [details]
Patch for landing

Clearing flags on attachment: 303872

Committed r213614: <http://trac.webkit.org/changeset/213614>