Bug 157490 – Numerous block selection issues on iOS

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

[Enrica Casucci]

This bug tracks the works required to fix a number of issues with block selections on iOS.

[Enrica Casucci]

Created attachment 278442 [details]
Patch

[WebKit Commit Bot]

Attachment 278442 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:478:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:389:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

Comment on attachment 278442 [details]
Patch

I found a problem with the patch. Removing the review flag.

[Darin Adler]

Comment on attachment 278442 [details]
Patch

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

A few comments even though this is not yet ready for review.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:884
> +    bool canShrink = false;

No need for this local variable.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:890
> +    if (renderer && renderer->childrenInline() && (is<RenderBlock>(*renderer) && !downcast<RenderBlock>(*renderer).inlineElementContinuation()) && !renderer->isTable())

This should just be a return statement.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:892
> +    return canShrink;

I would write it more like this:

    if (!node)
        return false;
    Element* element;
    if (is<Element>(*node))
        element = &downcast<Element>(*node);
    else {
        element = node->parentElement();
        if (!element)
            return false;
    }
    auto* renderer = element->renderer();
    if (!is<RenderBlock>(renderer))
        return false;
    auto& block = downcast<RenderBlock>(*renderer);
    return block.childrenInline && !block.inlineElementContinuation()) && !block.isTable();

Might want to put the whole "get from Node all the way to a RenderBlock" part into a separate helper function.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:895
> +static bool canShrinkToTextSelection(Range *range)

Argument type should be Range&, since it won’t work with a null pointer.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:902
> +static bool hasCustomLineHeight(Node* node)

Argument type should be Node&, since it won’t work with a null pointer.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:907
> +    if (renderer && renderer->style().lineHeight().isSpecified())
> +        return true;
> +    return false;

Should just be return x; rather than if (x) return true; return false;

    auto* renderer = node.renderer();
    return renderer && renderer->style().lineHeight().isSpecified();

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:921
> +            RenderText *renderText = downcast<RenderBlockFlow>(renderer)->findClosestTextAtAbsolutePoint(point);

Should be auto*, not RenderText *. Position of "*" and also better not to specify the type so the type can be as specific as possible

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1626
> +//        if (flags == None)
> +//            m_currentBlockSelection = nullptr;

Please don’t check in commented out code.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1641
> +PassRefPtr<Range> WebPage::switchToBlockSelectionAtPoint(const IntPoint& point, SelectionHandlePosition handlePosition)

New code should return RefPtr, not PassRefPtr.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1647
> +        Node *currentNode = &currentRange->startContainer();

Should be Node* with space after the *.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1669
> +        return (handlePoint.y() < m_blockRectForTextSelection.y());

No need for parentheses for return value.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1694
> +    SelectionHandlePosition handlePosition = baseIsStart ? SelectionHandlePosition::Bottom : SelectionHandlePosition::Top;
> +
>      

Extra blank line here, only need one.

[Enrica Casucci]

Created attachment 278521 [details]
Patch2

Fixes additional issue found and addresses Darin's comments.

[WebKit Commit Bot]

Attachment 278521 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:478:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:389:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 278521 [details]
Patch2

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

> Source/WebKit2/ChangeLog:11
> +        This patch fixes a number of issues with block selection on iOS like.
> +        We not longer eagerly choose block selection vs text selection and we

The grammar here is not quite right (sentence ending with like, "not", etc.)

[Enrica Casucci]

Committed revision 200660.