Bug 127015 – Support WebSelections in WK2 on iOS

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

[Enrica Casucci]

This bug tracks the work required to support selections in non editable content on iOS.

[Enrica Casucci]

Created attachment 221212 [details]
Patch

[Benjamin Poulain]

Comment on attachment 221212 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        This is the first step towards adding support for selections in
> +        non editable content on iOS for WK2.
> +        In particular, this patch adds the basic plumbing to decide which
> +        gesture recognizers are enabled and empty stubs for the gestures
> +        that we'll need to support.

I think you should add an explanation about the sequence of events, and how it differs when the assistant is up or not.
It is far from trivial, the more descriptive the better.

> Source/WebKit2/Shared/PositionInformation.cpp:42
> +    encoder << point;
> +    encoder << isSameAssistedNode;
> +    encoder << wantsHighlight;
> +    encoder << clickableElementName;
> +    encoder << url;
> +    encoder << selectionRects;

I believe you can just use the SimpleArgumentCoder to encode/decode here.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:168
> +    PositionInformation _positionInfo;
> +    BOOL _hasValidPositionInfo;

Let's not shorten the name.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:225
> +    [_doubleTapGestureRecognizer.get() setDelegate:nil];
>      [_highlightLongPressGestureRecognizer.get() setDelegate:nil];
> -    [_textSelectionAssistant release];
> +    [_longPressGestureRecognizer.get() setDelegate:nil];
> +    [_twoFingerPanGestureRecognizer.get() setDelegate:nil];

I believe you can remove the .get() now.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:380
> +    // don't allow the highlight to be prevented by a selection gesture. press-and-hold on a link should highlight the link, not select it

Missing uppercase and period to form sentences.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:389
> +#define IS_SAME_PAIR(a,b,x,y) ((((a) == (x)) && ((b) == (y))) || (((b) == (x)) && ((a) == (y))))

Let's use a static inline function instead of a Macro, that way we get type checking.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:419
> +    if (_positionInfo.clickableElementName == String("IMG"))
> +        return @selector(_showImageSheet);
> +    else if (_positionInfo.clickableElementName == String("A"))

String -> ASCIILiteral.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:437
> +            // If the assisted node is the same return NO.

"return NO" -> ", prevent the default gesture from starting."

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:438
> +            _page->getPositionInformation(IntPoint(point), _positionInfo);

I would prefer roundedIntPoint(point).

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:449
> +            // Return NO is there is no node.
> +            // Return YES if it is a node that wants highlight or if there is an action for it.

I think instead of Return NO/YES, you should say the desired action. The reader can see the return value bellow but it is hard to know why you are preventing the gesture from starting.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:459
> +            _page->requestPositionInformation(IntPoint(point));

I would prefer roundedIntPoint(point).

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:471
> +        if (!_hasValidPositionInfo) {
> +            _page->getPositionInformation(IntPoint(point), _positionInfo);
> +            _hasValidPositionInfo = YES;
> +        }

Use ensurePositionInfoIsUpToDate instead?

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:485
> +        // FIXME: Add implementation

notImplemented().

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:509
> +    return _positionInfo.clickableElementName != String("img") && _positionInfo.clickableElementName != String("a") && !_positionInfo.selectionRects.isEmpty();

String -> ASCIILiteral.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:553
> +        case UIGestureRecognizerStateBegan:
> +            // FIXME: add implementation
> +            break;
> +        case UIGestureRecognizerStateEnded:
> +            // FIXME: add implementation
> +            break;
> +        case UIGestureRecognizerStateCancelled:
> +            // FIXME: add implementation
> +            break;
> +        default:
> +            break;

Indent.

[Sam Weinig]

Comment on attachment 221212 [details]
Patch

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

> Source/WebKit2/Shared/PositionInformation.h:37
> +struct PositionInformation {

This name seems a bit generic to me.

> Source/WebKit2/Shared/PositionInformation.h:45
> +    bool isSameAssistedNode;

Is same as what?

[Enrica Casucci]

Created attachment 221300 [details]
Patch2

[Benjamin Poulain]

Comment on attachment 221300 [details]
Patch2

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

> Source/WebKit2/ChangeLog:4
> +        Support WebSelections in WK2 on iOS.
> +        https://bugs.webkit.org/show_bug.cgi?id=127015

Awesome changelog!

> Source/WebKit2/ChangeLog:11
> +        gesture recognizers are enabled and empty stubs for the gestures

and add empty stubs?

> Source/WebKit2/ChangeLog:37
> +            We are then able tp make an informed decision about whether

Typo: tp -> to

> Source/WebKit2/Shared/PositionInformation.h:37
> +struct PositionInformation {

InteractionInformationAtPosition maybe?

> Source/WebKit2/Shared/PositionInformation.h:40
> +        , wantsHighlight(true)

Shouldn't this be false by default?

Is it still needed?

> Source/WebKit2/Shared/PositionInformation.h:44
> +    WebCore::IntPoint point;

Maybe we should pass a floating point and round in the WebProcess?
I have the dream some day we will handle input on floating points position :)

(Not important for landing this).

> Source/WebKit2/Shared/PositionInformation.h:45
> +    bool isSameAssistedNode;

Maybe "nodeAtPositionIsSameAssistedNode"

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:39
> +#import <UIKit/UIGestureRecognizer_Private.h>
>  #import <UIKit/UIFont_Private.h>

Include order.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:424
> +    if (_positionInformation.clickableElementName == String(ASCIILiteral("IMG")))
> +        return @selector(_showImageSheet);
> +    else if (_positionInformation.clickableElementName == String(ASCIILiteral("A")))
> +        return @selector(_showLinkSheet);

Did you need the String() with ASCIILiteral()? If so, I should fix operator==.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:817
> +        const HTMLElement* element = toHTMLElement(hitNode);
> +        if (!element)

Is this right? IIRC, toHTMLElement will just static_cast. On the other hand, hitNode is already check for nullity above, and could be a SVN element.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:834
> +        if (element->hasLocalName(HTMLNames::appletTag) || element->hasLocalName(HTMLNames::embedTag) || element->hasLocalName(HTMLNames::objectTag))
> +            info.wantsHighlight = false;
> +        else if (element->renderer() && element->renderer()->isImage()) {
> +            URL url = toRenderImage(element->renderer())->cachedImage()->url();
> +            if (!url.string().isNull()) {
> +                info.wantsHighlight = true;
> +                info.url = url.string();
> +            }
> +        } else if (element->isLink()) {
> +            info.wantsHighlight = true;
> +            info.url = element->getAttribute(HTMLNames::hrefAttr).string();
> +        } else if (isHTMLSelectElement(hitNode))
> +            info.wantsHighlight = true;
> +        else
> +            info.wantsHighlight = hitNode->isContentEditable();

Why not set wantsHighlight to yet in all of those cases?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:853
> +

You can remove the empty line.

[Enrica Casucci]

Part one landed 162103.

[Enrica Casucci]

Created attachment 221474 [details]
Build fix

[Enrica Casucci]

Landed build fix 162204.

[Enrica Casucci]

Created attachment 221787 [details]
Patch3

[Benjamin Poulain]

Comment on attachment 221787 [details]
Patch3

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

> Source/WebKit2/Shared/ios/WKGestureTypes.h:57
>      WKSelectionTouchEnded,
>      WKSelectionTouchEndedMovingForward,
>      WKSelectionTouchEndedMovingBackward,
> -    WKSelectionTouchEndedNotMoving
> +    WKSelectionTouchEndedNotMoving,
> +    WKWebSelectionTouchStarted,
> +    WKWebSelectionTouchMoved,
> +    WKWebSelectionTouchEnded,

What about prefixing with WKEditableContentSelection and WKNonEditableContentSelection?

The UIKit names we use are pretty bad.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:712
> +    BOOL hasWebSelection = _webSelectionAssistant != nil && !CGRectIsEmpty(_webSelectionAssistant.get().selectionFrame);

_webSelectionAssistant != nil -> _webSelectionAssistant or !!_webSelectionAssistant

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:386
> +    IntPoint constrainedPoint = (m_assistedNode) ? constrainPoint(point, &frame, m_assistedNode.get()) : point;

No need for the parenthesis around m_assistedNode.

[Enrica Casucci]

Created attachment 221933 [details]
Patch4

[Enrica Casucci]

Created attachment 223781 [details]
block selection part1

[Benjamin Poulain]

Comment on attachment 223781 [details]
block selection part1

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

> Source/WebKit2/Shared/ios/WKGestureTypes.h:77
> +typedef enum {
> +    WKNone = 0,
> +    WKWordIsNearTap = 1,
> +    WKIsBlockSelection = 2
> +} WKSelectionFlags;
> +

If this is only used by Objective-C++ and C++, you could use a typed enum instead.

If those values are exclusive, I would use the name SelectionType instead of SelectionFlags.

> Source/WebKit2/UIProcess/API/ios/WKInteractionView.mm:1077
> +    {

The bracket should be on the previous line.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1068
> +    const int blockSelectionStartWidth = 100;
> +    const int blockSelectionStartHeight = 100;

Does this have to be defined on the header? Could it be moved to the implementation?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:430
> +        if (!range)
> +            return range;

I would use "return nullptr". The code might be a little misleading otherwise.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:438
> +            if (boundingRect.isEmpty())

You could use instead "i == 0", which can be optimized by the compiler.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:443
> +        if (boundingRect.width() > m_blockSelectionDesiredSize.width() && boundingRect.height() > m_blockSelectionDesiredSize.height())

The same condition is repeated bellow. I think a nice little static inline function naming this would be good.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:450
> +    if (!currentNode->isElementNode())
> +        currentNode = currentNode->parentElement();

Now this is confusing me.
If we had a text node above, we now take its parent element? Is there a missing return?

[Benjamin Poulain]

This looks sane to me. Maybe Ryosuke should double check the selection code?

[Enrica Casucci]

Comment on attachment 223781 [details]
block selection part1

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

>> Source/WebKit2/Shared/ios/WKGestureTypes.h:77
>> +
> 
> If this is only used by Objective-C++ and C++, you could use a typed enum instead.
> 
> If those values are exclusive, I would use the name SelectionType instead of SelectionFlags.

For now they are, but the idea was that when adding more they could be combined. All the other enums in this file are not. I'll change them all at once in another patch.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:1068
>> +    const int blockSelectionStartHeight = 100;
> 
> Does this have to be defined on the header? Could it be moved to the implementation?

I'll do that.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:450
>> +        currentNode = currentNode->parentElement();
> 
> Now this is confusing me.
> If we had a text node above, we now take its parent element? Is there a missing return?

Yes, we had a text node, but it did not meet the size requirements. We are then looking for the enclosing block to see if we want to turn it into a block selection.

[Enrica Casucci]

Created attachment 224810 [details]
block selection part2

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:441:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:743:  The parameter name "handlePosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


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

[Ryosuke Niwa]

Comment on attachment 224810 [details]
block selection part2

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

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:748
> +    return ((widthRatio > minMagnitudeRatio && xOriginShiftRatio < maxDisplacementRatio) &&
> +            (heightRatio > minMagnitudeRatio && yOriginShiftRatio < maxDisplacementRatio));

Nit: && should be at the beginning of the second line.
Also, it should be indented by 4 spaces instead of being aligned.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:758
> +    return (first->commonAncestorContainer(ec)->ownerDocument() == second->commonAncestorContainer(ec)->ownerDocument() &&
> +            first->compareBoundaryPoints(Range::START_TO_START, second, ASSERT_NO_EXCEPTION) <= 0 &&
> +            first->compareBoundaryPoints(Range::END_TO_END, second, ASSERT_NO_EXCEPTION) >= 0);

Ditto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:991
> +        ExceptionCode ec;
> +        Node* node = currentRange->commonAncestorContainer(ec);

We should probably use ASSERT_NO_EXCEPTION here (probably always has it as the default argument).

[Benjamin Poulain]

Comment on attachment 224810 [details]
block selection part2

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

Quick first round.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:500
> +    IntRect boundingRect;

This looks unused.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:501
> +    ExceptionCode ec;

ASSERT_NO_EXCEPTION or IGNORE_EXCEPTION instead?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:629
> +        if (static_cast<WKGestureRecognizerState>(gestureState) == WKGestureRecognizerStateEnded && flags & WKIsBlockSelection)

Maybe time to remove all the static_cast and do the conversion at the beginning of the function?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:721
> +static const int MaxHitTests = 10;

The first letter of the constant is uppercase.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:753
> +    ExceptionCode ec;

IGNORE_EXCEPTION or ASSERT_NO_EXCEPTION instead?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:793
> +    static const CGFloat maxDistance = 1000;

CGFloat -> float.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:836
> +        BOOL better = !(rectsEssentiallyTheSame(copyRect, currentBox, .05));

BOOL -> bool.

The name "better" does not provide enough information about the variable.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:846
> +                case WKHandleTop:
> +                case WKHandleBottom:
> +                    better = (copyRect.height() > currentBox.height());
> +                    break;
> +                case WKHandleRight:
> +                case WKHandleLeft:
> +                    better = (copyRect.width() > currentBox.width());
> +                    break;

Style.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:871
> +        distance *= multiple;
> +        distance = ceilf(distance);

distance = ceilf(distance * multiple);

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:893
> +    float distance = 1;
> +    RefPtr<Range> best;
> +    IntRect bestRect;

We should reduce the declaration-use span of those variables.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:907
> +    const CGFloat multiple = powf(maxDistance - 1, 1.0/(MaxHitTests - 1));

Looks like this should be In a function, it is used in two places.

CGFloat -> float.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:990
> +        ExceptionCode ec;

You should use IGNORE_EXCEPTION instead.

[Enrica Casucci]

Created attachment 224878 [details]
Block selection part2

Thank you Ryosuke and Ben for the feedback. Here is the new patch that addresses your comments.

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:441:  The parameter name "point" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:743:  The parameter name "handlePosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


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

[Enrica Casucci]

Created attachment 224880 [details]
Block selection part2

Fixes the style issue and removes the redundant exports.

[Benjamin Poulain]

Comment on attachment 224880 [details]
Block selection part2

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

> Source/WebKit2/UIProcess/WebPageProxy.h:461
> +    void updateBlockSelectionWithTouches(const WebCore::IntPoint, uint32_t touches, uint32_t handlePosition);

WithTouches -> WithTouch?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:716
> +static const int maxHitTests = 10;

If you make a single template (see comment bellow), this constant could simply be in the common function itself.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:772
> +        return IntPoint(box.x() + box.width() / 2, box.y());

x + width -> maxX?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:774
> +        return IntPoint(box.maxX(), box.y() + box.height() / 2);

maxY

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:776
> +        return IntPoint(box.x() + box.width() / 2, box.maxY());

ditto

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:778
> +        return IntPoint(box.x(), box.y() + box.height() / 2);

ditto

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:791
> +    RefPtr<Range> best;

bestRange?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:873
> +PassRefPtr<Range> WebPage::contractedRangeFromHandle(Range* currentRange, WKHandlePosition handlePosition, WKSelectionFlags& flags)

Any way to make expandedRangeFromHandle and contractedRangeFromHandle client of a single template handling the loop?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:875
> +    // Shrinking with a base and extent will always give better results.  If we only have a single element,

Two spaces after the period.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:876
> +    // see if we can break that down to a base and extent.  Shrinking base and extent is comparatively straightforward.

Ditto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:899
> +    RefPtr<Range> best;

bestRange?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:987
> +        if (renderer && renderer->childrenInline() && (renderer->isRenderBlock() && toRenderBlock(renderer)->inlineElementContinuation() == nil) && !renderer->isTable()) {

Instead of == nil, it should use ! on the value.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:998
> +    RefPtr<Range> currentRange = m_currentBlockSelection ? m_currentBlockSelection.get() : frame.selection().selection().toNormalizedRange();

Why are m_currentBlockSelection and frame.selection().selection() different?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1010
> +    float current, expanded, contracted;

Need update to match WebKit's style.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1105
> +

Blank line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1108
> +

Empty line.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1416
> +            info.bounds = hitNode->renderer()->absoluteBoundingBoxRect(true);

useTransform is the default, you could drop the argument here.

[Enrica Casucci]

Thanks for the review

> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:772
> > +        return IntPoint(box.x() + box.width() / 2, box.y());
> 
> x + width -> maxX?
No, it is x + width/2. It is true for all the other cases you pointed out.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:873
> > +PassRefPtr<Range> WebPage::contractedRangeFromHandle(Range* currentRange, WKHandlePosition handlePosition, WKSelectionFlags& flags)
> 
> Any way to make expandedRangeFromHandle and contractedRangeFromHandle client of a single template handling the loop?
Yes, I'll do it in another patch. I'll add a fix me.

> 
> Why are m_currentBlockSelection and frame.selection().selection() different?

Because once you set a range as Selection it gets normalized.

[Enrica Casucci]

Committed revision 164497.