Bug 135476 – Implement long mouse press over links. Part of 135257 - Add long mouse press gesture.

• Component: New Bugs
• Status: RESOLVED FIXED

[Peyton Randolph]

Implement long mouse press over links. Part of 135257 - Add long-press gesture to Mac.

[Peyton Randolph]

Created attachment 235850 [details]
Patch

[Peyton Randolph]

Comment on attachment 235850 [details]
Patch

webkit-patch failed to edit the changelog. Uploading a new patch.

[Peyton Randolph]

Created attachment 235855 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 235855 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:167
> +const double longMousePressDelay = 0.5;

This could use std::chrono so the units are explicit.

> Source/WebCore/page/EventHandler.cpp:168
> +const int longMousePressHysteresis = 5;

What is this value? Pixels? The name should make it clear.

> Source/WebCore/page/EventHandler.cpp:1583
> +    // FIXME: bubble long mouse press up to UI process.

This comment doesn't make sense here, since EventHandler is also used in non-mulitprocess mode.

> Source/WebCore/page/EventHandler.cpp:1588
> +    // FIXME: bubble long mouse press up to UI process.

Ditto, etc.

> Source/WebCore/page/EventHandler.cpp:1627
> +        if (FrameView* view = m_frame.view())
> +            view->setCursor(pointerCursor());

It seems a bit odd to reset the cursor here. Why?

> Source/WebCore/page/EventHandler.h:384
> +    bool mouseHysteresisExceeded(const FloatPoint&, const int) const;

The second parameter should be named here.

[Peyton Randolph]

(In reply to comment #4)
> (From update of attachment 235855 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235855&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:167
> > +const double longMousePressDelay = 0.5;
> 
> This could use std::chrono so the units are explicit.

Done.

> 
> > Source/WebCore/page/EventHandler.cpp:168
> > +const int longMousePressHysteresis = 5;
> 
> What is this value? Pixels? The name should make it clear.

Renamed to longMousePressPointsHysteresis.

> 
> > Source/WebCore/page/EventHandler.cpp:1583
> > +    // FIXME: bubble long mouse press up to UI process.
> 
> This comment doesn't make sense here, since EventHandler is also used in non-mulitprocess mode.

Reworded to FIXME: bubble long mouse press up to the client.

> 
> > Source/WebCore/page/EventHandler.cpp:1588
> > +    // FIXME: bubble long mouse press up to UI process.
> 
> Ditto, etc.

Done.
> 
> > Source/WebCore/page/EventHandler.cpp:1627
> > +        if (FrameView* view = m_frame.view())
> > +            view->setCursor(pointerCursor());
> 
> It seems a bit odd to reset the cursor here. Why?

Removed. The thinking was to reset the cursor because if the user moves their mouse a significant distance after recognizing the long press, it looks odd if the cursor is still a pointer, i-bar, or a wacky custom cursor. I added this because HandleDrag does the same. But it's not really necessary.

> 
> > Source/WebCore/page/EventHandler.h:384
> > +    bool mouseHysteresisExceeded(const FloatPoint&, const int) const;
> 
> The second parameter should be named here.

Named pointsThreshold.

[Peyton Randolph]

Created attachment 235922 [details]
Patch

[Tim Horton]

Comment on attachment 235922 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:167
> +const std::chrono::milliseconds longMousePressDelay = std::chrono::milliseconds(500);

s/std::chrono::milliseconds(500)/500_ms/

[Peyton Randolph]

Created attachment 236247 [details]
Patch

[Tim Horton]

Comment on attachment 236247 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:166
> +#if ENABLE(LONG_MOUSE_PRESS)

Simon noted that you might want to get rid of these ENABLEs, since there's nothing particularly scary or platform-specific about this feature, and it has/will have a runtime flag, and won't do anything if the client doesn't care, anyway. That said, I think that can happen later, perhaps in https://bugs.webkit.org/show_bug.cgi?id=135721.

> Source/WebCore/page/EventHandler.cpp:168
> +const int longMousePressPointsHysteresis = 5;

Maybe "maximumDistanceForLongMousePress", or something (that's not a great name either, but a little better). "pointsHysteresis" reads oddly to me.

> Source/WebCore/page/EventHandler.cpp:780
> +        beginLongMousePress();

Is this "beginTrackingPotentialLongMousePress"? because I don't think the *long* mouse press is technically beginning yet. But I dunno!

> Source/WebCore/page/EventHandler.cpp:1585
> +    // FIXME: bubble long mouse press up to the client.

Capital 'B', perhaps. Also, do you have a bug number for these?

> Source/WebCore/page/EventHandler.cpp:1619
> +    if (mouseEvent.button() != LeftButton || mouseEvent.type() != PlatformEvent::MouseMoved)

Why would long press be restricted to the left mouse button? Should we not support long-press of any arbitrary button and let the client decide which ones it cares about? I could see that being useful.

> Source/WebCore/page/EventHandler.cpp:1630
> +    if (!mouseHysteresisExceeded(mouseEvent.position(), longMousePressPointsHysteresis))
> +        return false;
> +
> +    cancelLongMousePress();
> +
> +    return false;

I think this would read better as just:

if (mouseHysteresisExceeded(mouseEvent.position(), longMousePressPointsHysteresis))
    cancelLongMousePress();

return false;

> Source/WebCore/page/EventHandler.cpp:3527
> +bool EventHandler::mouseHysteresisExceeded(const FloatPoint& viewportLocation, const int pointsThreshold) const

This is still technically about drag, in some sense. Maybe "dragDistanceExceedsThreshold"? That sounds good in an if statement ("if dragDistanceExceedsThreshold").

> Source/WebCore/page/EventHandler.cpp:3535
> +    return abs(delta.width()) >= pointsThreshold || abs(delta.height()) >= pointsThreshold;

Should this be something like delta.diagonalLengthSquared() >= pointsThreshold * pointsThreshold, to make a circle of hysteresis?

I see you got this from existing code, so maybe don't change it, but I wonder why!

[Peyton Randolph]

(In reply to comment #9)
> (From update of attachment 236247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236247&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:166
> > +#if ENABLE(LONG_MOUSE_PRESS)
> 
> Simon noted that you might want to get rid of these ENABLEs, since there's nothing particularly scary or platform-specific about this feature, and it has/will have a runtime flag, and won't do anything if the client doesn't care, anyway. That said, I think that can happen later, perhaps in https://bugs.webkit.org/show_bug.cgi?id=135721.

I agree with doing so later. I wasn't up-to-date on WebKit conventions when I added the flag. Ideally the runtime switch (https://bugs.webkit.org/show_bug.cgi?id=135682) would have come first.

> 
> > Source/WebCore/page/EventHandler.cpp:168
> > +const int longMousePressPointsHysteresis = 5;
> 
> Maybe "maximumDistanceForLongMousePress", or something (that's not a great name either, but a little better). "pointsHysteresis" reads oddly to me.

I've renamed to MaximumLongMousePressDragDistance and added a comment that its units are in points.

> 
> > Source/WebCore/page/EventHandler.cpp:780
> > +        beginLongMousePress();
> 
> Is this "beginTrackingPotentialLongMousePress"? because I don't think the *long* mouse press is technically beginning yet. But I dunno!

The term is borrowed from UIKit's gesture recognizers. It means "begin determining if a long mouse press is occurring." 

I like your interpretation though. I will rename this and cancelLongMousePress to cancelTrackingPotentialLongMousePress as well.

> 
> > Source/WebCore/page/EventHandler.cpp:1585
> > +    // FIXME: bubble long mouse press up to the client.
> 
> Capital 'B', perhaps. Also, do you have a bug number for these?

Capitalized and added bug number, https://bugs.webkit.org/show_bug.cgi?id=135580

> 
> > Source/WebCore/page/EventHandler.cpp:1619
> > +    if (mouseEvent.button() != LeftButton || mouseEvent.type() != PlatformEvent::MouseMoved)
> 
> Why would long press be restricted to the left mouse button? Should we not support long-press of any arbitrary button and let the client decide which ones it cares about? I could see that being useful.

Ah, good idea. For this patch, I would like to restrict ourselves to left mouse presses because it's difficult enough test the single case of left-mouse-pressing links. I filed a bug tracking the extension to arbitrary mouse clicks: https://bugs.webkit.org/show_bug.cgi?id=135767.

> 
> > Source/WebCore/page/EventHandler.cpp:1630
> > +    if (!mouseHysteresisExceeded(mouseEvent.position(), longMousePressPointsHysteresis))
> > +        return false;
> > +
> > +    cancelLongMousePress();
> > +
> > +    return false;
> 
> I think this would read better as just:
> 
> if (mouseHysteresisExceeded(mouseEvent.position(), longMousePressPointsHysteresis))
>     cancelLongMousePress();
> 
> return false;
> 
Done.

> > Source/WebCore/page/EventHandler.cpp:3527
> > +bool EventHandler::mouseHysteresisExceeded(const FloatPoint& viewportLocation, const int pointsThreshold) const
> 
> This is still technically about drag, in some sense. Maybe "dragDistanceExceedsThreshold"? That sounds good in an if statement ("if dragDistanceExceedsThreshold").

While dragging requires a mouse button to be pressed, this method does not. Maybe mouseMovementExceedsThreshold?

> 
> > Source/WebCore/page/EventHandler.cpp:3535
> > +    return abs(delta.width()) >= pointsThreshold || abs(delta.height()) >= pointsThreshold;
> 
> Should this be something like delta.diagonalLengthSquared() >= pointsThreshold * pointsThreshold, to make a circle of hysteresis?
> 
> I see you got this from existing code, so maybe don't change it, but I wonder why!

I agree, the box model is pretty weird :/ I'm going to leave it as-is since this code is used by dragging, so I don't know if there's something else counting on this behavior.

[Peyton Randolph]

Created attachment 236311 [details]
Patch

[Tim Horton]

Comment on attachment 236311 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:136
> +const std::chrono::milliseconds LongMousePressRecognitionDelay = 500_ms;
> +const int MaximumLongMousePressDragDistance = 5; // in points.

Lowercase leading letters was actually correct, the things around here are wrong :D

> Source/WebCore/page/EventHandler.h:384
> +    bool mouseMovementExceedsThreshold(const FloatPoint&, const int pointsThreshold) const;

Sounds good to me!

Don't think you need the 'const' on the int parameter (some would even say const reference is overkill for FloatPoint, but I will leave that be).

[Peyton Randolph]

(In reply to comment #12)
> (From update of attachment 236311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236311&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:136
> > +const std::chrono::milliseconds LongMousePressRecognitionDelay = 500_ms;
> > +const int MaximumLongMousePressDragDistance = 5; // in points.
> 
> Lowercase leading letters was actually correct, the things around here are wrong :D

Ah, the constants above lie. Fixed.
> 
> > Source/WebCore/page/EventHandler.h:384
> > +    bool mouseMovementExceedsThreshold(const FloatPoint&, const int pointsThreshold) const;
> 
> Sounds good to me!
> 
> Don't think you need the 'const' on the int parameter (some would even say const reference is overkill for FloatPoint, but I will leave that be).

Removed the const int.

[Peyton Randolph]

Created attachment 236321 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 236321 [details]
Patch

Clearing flags on attachment: 236321

Committed r172367: <http://trac.webkit.org/changeset/172367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.