Bug 94101 – Touch adjustment for context menu gestures

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 158559 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Allan Sandfeld Jensen]

(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.

[Antonio Gomes]

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?

[Allan Sandfeld Jensen]

(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.

[Antonio Gomes]

> > 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>?

[Allan Sandfeld Jensen]

(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?

[Antonio Gomes]

(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; }

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Created attachment 158732 [details]
Patch

[Allan Sandfeld Jensen]

(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.

[Terry Anderson]

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.

[Allan Sandfeld Jensen]

Created attachment 159067 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Antonio Gomes]

(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.

[Antonio Gomes]

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.

[Allan Sandfeld Jensen]

(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?

[Allan Sandfeld Jensen]

Committed r125898: <http://trac.webkit.org/changeset/125898>