Implement long mouse press over links. Part of 135257 - Add long-press gesture to Mac.
Created attachment 235850 [details] Patch
Comment on attachment 235850 [details] Patch webkit-patch failed to edit the changelog. Uploading a new patch.
Created attachment 235855 [details] Patch
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.
(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.
Created attachment 235922 [details] Patch
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/
Created attachment 236247 [details] Patch
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!
(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.
Created attachment 236311 [details] Patch
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).
(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.
Created attachment 236321 [details] Patch
Comment on attachment 236321 [details] Patch Clearing flags on attachment: 236321 Committed r172367: <http://trac.webkit.org/changeset/172367>
All reviewed patches have been landed. Closing bug.