Bug 157681 – Text selection is basically impossible on plain text pages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

Navigate to http://ohhla.com/anonymous/mf_doom/madvill/moneyfld.mfd.txt and try to select text.
It is impossible.

rdar://problem/26065660

[Enrica Casucci]

Created attachment 278862 [details]
Patch

[Darin Adler]

Comment on attachment 278862 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1662
> +    if (!m_blockRectForTextSelection.height())
> +        return false;

What about very small heights?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1864
> +static bool rectIsTooBigForSelection(const IntRect& blockRect, const Frame& frame)

Consider inline maybe?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1866
> +    const static CGFloat factor = 0.97;

I know this code was just moved, but:

Why does this need to be a CGFloat? Why not just a float or a double? Doesn’t seem like CG is involved here.

Also, const static is not better than const.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1867
> +    return blockRect.height() > (frame.view()->unobscuredContentRect().height() * factor);

I know this code was just moved, but:

What guarantees view() is non-null?

No need for the parentheses in this expression.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1880
> +        if (renderer->style().preserveNewline())
> +            m_blockRectForTextSelection = renderer->absoluteBoundingBoxRect(true);

This seems not exactly right to me. Why is the only relevant flag “preserveNewline”? What about all the other related properties, like breakOnlyAfterWhiteSpace and auto wrap, etc.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1882
> +            RefPtr<Range> paragraphRange = enclosingTextUnitOfGranularity(visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode), ParagraphGranularity, DirectionForward);

I know this code was just moved, but:

I suggest auto rather than writing out the type RefPtr<Range>.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2409
> +                info.isSelectable = renderer->style().userSelect() != SELECT_NONE;
> +                if (info.isSelectable && !hitNode->isTextNode())
> +                    info.isSelectable = !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame());

This whole thing could use just &&:

    info.isSelectable = renderer->style().userSelect() != SELECT_NONE
        && !hitNode->isTextNode()
        && !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame());

[Darin Adler]

I would like to see test coverage for this. It doesn’t just affect plain text, so it can be HTML tests.

[Enrica Casucci]

Thanks for the review.
I'll address your comments. See my replies below to a your questions.
(In reply to comment #2)
> Comment on attachment 278862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278862&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1662
> > +    if (!m_blockRectForTextSelection.height())
> > +        return false;
> 
> What about very small heights?
I tried to follow the original WK1 code that did not discard very small blocks.
A height of 0 is explicitly set when we want to exclude the possibility of block selection.

> I know this code was just moved, but:
> 
> What guarantees view() is non-null?

I thought that having a WebPage is a guarantee that the view is not null. Nowhere else in this file where a view is referenced we check for null.

[Enrica Casucci]

(In reply to comment #2)
> This seems not exactly right to me. Why is the only relevant flag
> “preserveNewline”? What about all the other related properties, like
> breakOnlyAfterWhiteSpace and auto wrap, etc.
> 
I forgot to answer one of your questions.
I believe this is the only relevant one. What we care about is properties that play a role when computing start and end of paragraph.

[Enrica Casucci]

> This whole thing could use just &&:
> 
>     info.isSelectable = renderer->style().userSelect() != SELECT_NONE
>         && !hitNode->isTextNode()
>         && !rectIsTooBigForSelection(info.bounds, *result.innerNodeFrame());

Maybe I'm wrong, but I don't think this is equivalent.
Written this way, if hitNode is a text node then isSelectable is false, which is not what we want.
We want to allow selection on text nodes if they are selectable. We want to limit the size of selections on non text nodes.

[Enrica Casucci]

Committed revision 200972.

[Chris Dumez]

Comment on attachment 278862 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1882
>> +            RefPtr<Range> paragraphRange = enclosingTextUnitOfGranularity(visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode), ParagraphGranularity, DirectionForward);
> 
> I know this code was just moved, but:
> 
> I suggest auto rather than writing out the type RefPtr<Range>.

The patch that landed used:
auto* paragraphRange = enclosingTextUnitOfGranularity(visiblePositionInFocusedNodeForPoint(frame, point, isInteractingWithAssistedNode), ParagraphGranularity, DirectionForward).get();

This is unsafe because enclosingTextUnitOfGranularity() returns a RefPtr and transfers ownership of the range to the caller. This was causing crashes that Zalan is fixing via Bug 158155.

[Darin Adler]

Darn. I was suggesting "auto", not "auto*".