Bug 94101 - Touch adjustment for context menu gestures
Summary: Touch adjustment for context menu gestures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-15 06:35 PDT by Allan Sandfeld Jensen
Modified: 2012-08-17 07:32 PDT (History)
2 users (show)

See Also:


Attachments
Patch (19.24 KB, patch)
2012-08-15 06:39 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (19.79 KB, patch)
2012-08-16 00:36 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (23.63 KB, patch)
2012-08-17 03:06 PDT, Allan Sandfeld Jensen
tonikitoo: review+
tonikitoo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-08-15 06:35:06 PDT
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.
Comment 1 Allan Sandfeld Jensen 2012-08-15 06:39:37 PDT
Created attachment 158559 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-08-15 06:44:39 PDT
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
Comment 3 Allan Sandfeld Jensen 2012-08-15 06:54:26 PDT
(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 4 Antonio Gomes 2012-08-15 08:03:29 PDT
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?
Comment 5 Allan Sandfeld Jensen 2012-08-15 08:10:43 PDT
(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.
Comment 6 Antonio Gomes 2012-08-15 09:28:45 PDT
> > 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>?
Comment 7 Allan Sandfeld Jensen 2012-08-15 09:54:54 PDT
(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?
Comment 8 Antonio Gomes 2012-08-15 10:55:12 PDT
(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; }
Comment 9 Allan Sandfeld Jensen 2012-08-15 11:10:54 PDT
(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.
Comment 10 Allan Sandfeld Jensen 2012-08-16 00:36:27 PDT
Created attachment 158732 [details]
Patch
Comment 11 Allan Sandfeld Jensen 2012-08-16 00:40:38 PDT
(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 12 Terry Anderson 2012-08-16 08:37:33 PDT
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.
Comment 13 Allan Sandfeld Jensen 2012-08-17 03:06:46 PDT
Created attachment 159067 [details]
Patch
Comment 14 Kenneth Rohde Christiansen 2012-08-17 03:10:28 PDT
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
Comment 15 Antonio Gomes 2012-08-17 06:38:00 PDT
(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 16 Antonio Gomes 2012-08-17 06:49:20 PDT
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.
Comment 17 Allan Sandfeld Jensen 2012-08-17 06:55:31 PDT
(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?
Comment 18 Allan Sandfeld Jensen 2012-08-17 07:32:14 PDT
Committed r125898: <http://trac.webkit.org/changeset/125898>