Context menu gesture recently started using the touch adjustment code, but is using the best-clickable algorithm designed to find nodes that can be activated by tap gestures. Since some elements such as images have extra context-menu items but are not activable, and many elements that are activatable does not have extra context-menu items, a more specialized touch adjustment filter should be used.
Created attachment 158559 [details] Patch
Comment on attachment 158559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158559&action=review > Source/WebCore/page/EventHandler.cpp:2576 > + case PlatformEvent::GestureTwoFingerTap: two finger tap also shows the context menu? Is it commonly implemented like that? Doesn't that conflict with double tap to zoom etc? > Source/WebCore/page/TouchAdjustment.cpp:92 > +bool nodeProvidesContextMenuItems(Node* node) > +{ What about the custom context menu (doesnt webkit support that?) http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#context-menus
(In reply to comment #2) > (From update of attachment 158559 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158559&action=review > > > Source/WebCore/page/EventHandler.cpp:2576 > > + case PlatformEvent::GestureTwoFingerTap: > > two finger tap also shows the context menu? Is it commonly implemented like that? Doesn't that conflict with double tap to zoom etc? > I was under the impression it sends a right-mouse button press on iOS devices, but since they don't use these functions, it could also be removed for now. I am bit unhappy with the gesture names though, I would prefer if they described their function instead of how they are activated (GestureContextMenu in this case), but that is something to fix another time, if more people agree.
Comment on attachment 158559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158559&action=review >> Source/WebCore/page/TouchAdjustment.cpp:92 >> +{ > > What about the custom context menu (doesnt webkit support that?) http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#context-menus What about text nodes?
(In reply to comment #4) > (From update of attachment 158559 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158559&action=review > > >> Source/WebCore/page/TouchAdjustment.cpp:92 > >> +{ > > > > What about the custom context menu (doesnt webkit support that?) http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#context-menus > As far as I can tell, we have no support for that. > What about text nodes? Text nodes only provides extra context menus when they are selected or are editable. I check for editable, but I haven't done the check for selected yet.
> > What about text nodes? > > Text nodes only provides extra context menus when they are selected or are editable. I check for editable, but I haven't done the check for selected yet. what will happen if you right click "text" in <div> some text </div>?
(In reply to comment #6) > > > What about text nodes? > > > > Text nodes only provides extra context menus when they are selected or are editable. I check for editable, but I haven't done the check for selected yet. > > what will happen if you right click "text" in <div> some text </div>? You get the standard context-menu for the page offering your to go back or forward in the history and a few other options. If you select "text" and right click you get options relating to your selection. Are you implying the gesture should not only issue a context-menu event, but start by selecting the nearest word?
(In reply to comment #7) > (In reply to comment #6) > > > > What about text nodes? > > > > > > Text nodes only provides extra context menus when they are selected or are editable. I check for editable, but I haven't done the check for selected yet. > > > > what will happen if you right click "text" in <div> some text </div>? I am check all possibilities before considering adopting the cross-platform implementation. In BlackBerry we also use touch_adjustment (fat fingers) for text select the closest word on touch_press on #text node. > You get the standard context-menu for the page offering your to go back or forward in the history and a few other options. If you select "text" and right click you get options relating to your selection. Are you implying the gesture should not only issue a context-menu event, but start by selecting the nearest word? No. that is done by a "setting": 30 public: 31 EditingBehavior(EditingBehaviorType type) 32 : m_type(type) 33 { ,,, 60 // On Mac, when processing a contextual click, the object being clicked upon should be selected. 61 bool shouldSelectOnContextualMenuClick() const { return m_type == EditingMacBehavior; }
(In reply to comment #8) > No. that is done by a "setting": > Thanks, I missed that option because it was in editing, and I already accepted all editable nodes. I will see what I can do. I am also still trying to figure out how to best touch adjust to existing selections.
Created attachment 158732 [details] Patch
(In reply to comment #4) > (From update of attachment 158559 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158559&action=review > > >> Source/WebCore/page/TouchAdjustment.cpp:92 > >> +{ > > > > What about the custom context menu (doesnt webkit support that?) http://www.whatwg.org/specs/web-apps/current-work/multipage/interactive-elements.html#context-menus > > What about text nodes? In the latest patch I check for canBeSelectionLeaf(), this means not only text, but anything that can be selected is considered good targets if shouldSelectOnContextualMenuClick() is true.
Comment on attachment 158559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158559&action=review >>> Source/WebCore/page/EventHandler.cpp:2576 >>> + case PlatformEvent::GestureTwoFingerTap: >> >> two finger tap also shows the context menu? Is it commonly implemented like that? Doesn't that conflict with double tap to zoom etc? > > I was under the impression it sends a right-mouse button press on iOS devices, but since they don't use these functions, it could also be removed for now. > I am bit unhappy with the gesture names though, I would prefer if they described their function instead of how they are activated (GestureContextMenu in this case), but that is something to fix another time, if more people agree. I second the motion of only applying this patch to a GestureLongPress for the time being.
Created attachment 159067 [details] Patch
Comment on attachment 159067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159067&action=review > Source/WebCore/page/EventHandler.cpp:2554 > + case PlatformEvent::GestureLongPress: I think we usually called this TapAndHold, but im not sure what is the best
(In reply to comment #14) > (From update of attachment 159067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159067&action=review > > > Source/WebCore/page/EventHandler.cpp:2554 > > + case PlatformEvent::GestureLongPress: > > I think we usually called this TapAndHold, but im not sure what is the best Tap by itself it a gesture of a quick touch down and up. TouchAndHold sounds more appropriated.
Comment on attachment 159067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159067&action=review Lets get the namings discussed here changed. > Source/WebCore/page/TouchAdjustment.cpp:91 > +bool nodeProvidesContextMenuItems(Node* node) I think we can exclude "node" from the beginning, since the parameter is a 'node'. > Source/WebCore/page/TouchAdjustment.cpp:105 > + // FIXME: To improve the adjusted point, each individual word should be a separate subtarget. See FatFingers.
(In reply to comment #16) > (From update of attachment 159067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=159067&action=review > > Lets get the namings discussed here changed. I would prefer opening another bug for that, and as I said earlier, to support possibly different gestures I would prefer the cross-platform gestures in WebCore were named after function and not by how they are activated. > > Source/WebCore/page/TouchAdjustment.cpp:105 > > + // FIXME: To improve the adjusted point, each individual word should be a separate subtarget. > > See FatFingers. I did, I added the comment after seeing a similar comment there, or did I miss something more?
Committed r125898: <http://trac.webkit.org/changeset/125898>