Closed Bug 503541 Opened 11 years ago Closed 11 years ago

Fine grained control of gesture registration on widgets

Categories

(Core :: Widget: Win32, defect, P2)

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2b1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.1pre) Gecko/20090702 Shiretoko/3.5.1pre (.NET CLR 3.5.30729)
Build Identifier: 

On the gesture registration process, we should be able to register different gesture behaviors for different widgets. For example, on content we want panning to work, but not on the toolbar or other places of the UI. In addition, we would want to register horizontal panning on the tab strip while keep vertical panning on content.

Working on it.

Reproducible: Always
Blocks: 488715
OS: Windows NT → Windows 7
Assignee: nobody → felipc
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch WIP simple patch 1 (obsolete) — Splinter Review
This is a simple patch that doesn't give a good control yet but it gives the immediate results that we were looking for, which specially improves the interactions with menu (now the UI doesn't pan, only contentt; and you can just start dragging your finger over a menu and release it when you choose the option you want).

I had a better patch but I had to go back a little because I ran into some problems. Namely:
- now we just register touch for the content. I want to register touch for everywhere on the browser, and just don't-ask/block the PAN where we don't want to. I tried this route but the MozillaContentWindowClass has some child windows that I haven't figured out how to detect yet and blocking PAN in these makes the pan not to bubble up to the content window.
- I want to register panning for other scrollable items like <tree> but I don't know yet how to figure out which kind of element the nsWindow is being created to.
Blocks: 503889
So what this patch does is the following:

- add a new flag on nsWidgetInitData specifying the kind of gesture registration that a widget want, setting the default as eGestureDefault, which means registering for gestures but not for panning

- on widget creation, ScrollPort asks for eGesturePanAndFeedback, which allows panning feedback on content and richlistviews and other areas were it is working

- on widget creation, menus and trees asks only for eGesturePan, which allows panning

- on gesture happening, we check if it was registered as eGesturePanAndFeedback before triggering the feedback code
Attachment #387995 - Attachment is obsolete: true
Here's a cleaned up patch that applies after bug 352093 landed.

A view of the situation:

Now that scrollports are windowless, the gesture code itself will need some reworking, since the touch API relies on gesture registration for specific windows. The previous patch registered the gesture Pan+Feedback for all scrollport windows, so things like listviews and richlistviews scrolled with panning and displayed feedback.

The real way to solve this now is to decide in real time the gesture we want to receive. By reading the touch API docs, this is possible through the use of WM_GESTURENOTIFY message, which happens just before a gesture will begin, and give the chance of calling SetGestureConfig with the desired gesture to receive. Upon receiving a WM_GESTURENOTIFY message, we need to verify what element is positioned under the cursor and decide which kind of gesture we want to receive (basically at this point it is whether we want to receive panning or not). How could I go to this route?

Still, I'm posting this updated patch because it will in any way be the base for a new one, and this one is already useful because it fixes trees and menus panning, which currently don't work on trunk.
Th(In reply to comment #3)
> The real way to solve this now is to decide in real time the gesture we want to
> receive. By reading the touch API docs, this is possible through the use of
> WM_GESTURENOTIFY message, which happens just before a gesture will begin, and
> give the chance of calling SetGestureConfig with the desired gesture to
> receive. Upon receiving a WM_GESTURENOTIFY message, we need to verify what
> element is positioned under the cursor and decide which kind of gesture we want
> to receive (basically at this point it is whether we want to receive panning or
> not). How could I go to this route?

The best way to do this is probably to define a new event type in nsGUIEvent that is fired from WM_GESTURENOTIFY. You can add fields to the event to return the information you need. Code in nsPresShell can handle the event and figure out what kind of gesture(s) are expected. nsQueryContentEvent does something similar for IMEs already.
Roc, here's a work in progress version of the implementation if you could take a look and see if I'm getting things right. All the content area works as expected, and the tree works ok as well.

I'm currently stuck in making the menus work, because I'm checking if the scrollableFrame has actual visible scrollbars (this is needed for the content area) using the GetActualScrollbarSizes(), but menus don't have scrollbars.

I tried using nsLayoutUtils::GetNearestScrollingView and nsScrollPortView::CanScroll, but none seemed to work.

My next approach was checking if we're in a nsMenuPopupFrame, but I can't get:
nsMenuPopupFrame * menupopup = do_QueryFrame(frame);
to compile. It says there's am ambiguous access there. Same for nsXulScrollFrame.


What would you suggest? Ideally I don't think it's going to be a good idea to make special cases for the menus, the trees and other possible ones. If there's something in XUL which will report me if I can actually scroll something (a menu that overflowed, the tab strip overflowed), that would be the best I guess.
Attachment #390024 - Attachment is obsolete: true
Attachment #390756 - Attachment is obsolete: true
You could get the nsIScrollableView and call CanScroll.

And don't use wannapan as a variable name :)
> nsIFrame* rootFrame = this->GetRootFrame();
> nsPoint ptInRoot = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, rootFrame);
> nsIFrame* targetFrame = nsLayoutUtils::GetFrameForPoint(rootFrame, ptInRoot);

Hmm, is this supposed to work on a popup window as well? I think this might be my problem... When I test this over a popupmenu I'm never getting a nsMenuFrame or nsMenuPopupFrame in the targetFrame (or its parents)
Oh wait, no, don't do that.  You shouldn't get the targetFrame that way. See "if (dispatchUsingCoordinates)" below ... that will find the right frame for you, use that path.
Attached patch Using WM_GESTURENOTIFY - v2 (obsolete) — Splinter Review
yay, so here is a fully working patch which provides real time control for the gesture registration using the WM_GESTURENOTIFY message. An event is sent and then nsPresShell decides if we should be generating panning or regular mousedown/mousemove events on the initial point of contact.  There are several one-off cases for some widgets (scrollbars, treeviews, menus) to make sure everything works consistently.

Roc, could you please take a look and tell me what you think about this patch?
Attachment #391711 - Attachment is obsolete: true
Attachment #392588 - Flags: review?(roc)
Attached patch Using WM_GESTURENOTIFY - v2.1 (obsolete) — Splinter Review
oops minor change on the patch, I had left two commented lines while making some experiments
Attachment #392588 - Attachment is obsolete: true
Attachment #392621 - Flags: review?(roc)
Attachment #392588 - Flags: review?(roc)
+  NS_IMETHOD DecideGestureEvent(nsGUIEvent* aEvent,
+                                nsIFrame* targetFrame) = 0;

Needs documentation. And shouldn't aEvent be nsGestureNotifyEvent?

+PresShell::DecideGestureEvent(nsGUIEvent* aEvent,
+                                nsIFrame* targetFrame)

Fix indent

+  PRBool                mDisplayPanFeedback;

PRPackedBool

+  enum ePanDirection {
+    ePanNone,
+    ePanVertical,
+    ePanHorizontal
+  };

Can't we support panning in both directions simultaneously?

+  PRBool        displayPanFeedback;

PRPackedBool

+    // XXX For now, trees are complex to decide if we should pan or not (based if all rows are visible
+    // or not), so we will just always pan which is not so bad.
+    if (currentFrameType == nsGkAtoms::leafBoxFrame) {
+      panDirection = nsGestureNotifyEvent::ePanVertical;
+      break;

I think you need to give trees their own type, this will catch all sorts of non-tree stuff.

I think we should have a virtual method on nsIFrame, GetTouchPanningMode or something, that frames can override or just recursively call on their parent by default. Or can we share this code with the mousewheel processing in nsEventStateManager? Are there situations where a frame should be panned by touch and mousewheel scrolling should just send an event without scrolling, or vice versa? Basically I'm not comfortable with having touch-panning logic sitting in a lump here in the presshell, or distributed among the frames. We need to identify some more fundamental concept and base the gesture decisions on that.

Olli Pettay is going to have to review here ... he's the events man, and also a multimodal interface guru!
(In reply to comment #12)
> +  NS_IMETHOD DecideGestureEvent(nsGUIEvent* aEvent,
> +                                nsIFrame* targetFrame) = 0;
> 
> Needs documentation. And shouldn't aEvent be nsGestureNotifyEvent?

Yeah, should be nsGestureNotifyEvent. Will fix and write documentation. 

> Fix indent
> PRPackedBool

Okay

> +  enum ePanDirection {
> +    ePanNone,
> +    ePanVertical,
> +    ePanHorizontal
> +  };
> 
> Can't we support panning in both directions simultaneously?

Panning works in both directions, this is just the initial movement needed to trigger panning. If we activate both, user can't select text or drag things on any area that is decided to be scrollable. The default behavior here which works well in most of the pages is to go for vertical panning and horizontal text selection. 

> +    // XXX For now, trees are complex to decide if we should pan or not (based
> if all rows are visible
> +    // or not), so we will just always pan which is not so bad.
> +    if (currentFrameType == nsGkAtoms::leafBoxFrame) {
> +      panDirection = nsGestureNotifyEvent::ePanVertical;
> +      break;
> 
> I think you need to give trees their own type, this will catch all sorts of
> non-tree stuff.

Okay, I thought leafBoxFrame referred only to trees. I didn't find an specific frame type only for trees returned from GetType(), so maybe I can use do_QueryFrame with a nsTreeBodyFrame* and see if it's null or not. (I actually was doing this before)

> I think we should have a virtual method on nsIFrame, GetTouchPanningMode or
> something, that frames can override or just recursively call on their parent by
> default. Or can we share this code with the mousewheel processing in
> nsEventStateManager? Are there situations where a frame should be panned by
> touch and mousewheel scrolling should just send an event without scrolling, or
> vice versa? Basically I'm not comfortable with having touch-panning logic
> sitting in a lump here in the presshell, or distributed among the frames. We
> need to identify some more fundamental concept and base the gesture decisions
> on that.
> 
> Olli Pettay is going to have to review here ... he's the events man, and also a
> multimodal interface guru!

Okay, let's get input from Olli to see what are the possible approaches!

Regarding having a virtual method on nsIFrame, I think it shouldn't be too hard to go on that direction from this patch, but this way the decision of the panning mode would be scattered in various places, and the frames would be aware of their content anyway since some frame types are used for various type of elements.  Maybe it could be in nsIContent or XULElement or something like that? (or even in a XUL binding...) (I'm just throwing some ideas here).
Either way, where would we ultimately call this method to decide the panning mode? in presShell itself?
(In reply to comment #13)
> Okay, I thought leafBoxFrame referred only to trees. I didn't find an specific
> frame type only for trees returned from GetType(), so maybe I can use
> do_QueryFrame with a nsTreeBodyFrame* and see if it's null or not. (I actually
> was doing this before)

do_QueryFrame sounds good

> Regarding having a virtual method on nsIFrame, I think it shouldn't be too hard
> to go on that direction from this patch, but this way the decision of the
> panning mode would be scattered in various places, and the frames would be
> aware of their content anyway since some frame types are used for various type
> of elements.  Maybe it could be in nsIContent or XULElement or something like
> that? (or even in a XUL binding...) (I'm just throwing some ideas here).
> Either way, where would we ultimately call this method to decide the panning
> mode? in presShell itself?

I'm not sure. Maybe Olli has some ideas.
Attachment #392621 - Flags: review?(Olli.Pettay)
So perhaps DecideGestureEvent could go to the event state manager, and it would 
be called in PostHandleEvent when handling NS_GESTURENOTIFY_EVENT.
Atm DecideGestureEvent isn't too complicated so IMO there is no need
to add any methods to nsIFrame, but just keep everything in one place
(close to mouse wheel handling).
And if we at some point want to allow scripts to decide how the gesture should
be handled, NS_GESTURENOTIFY_EVENT could be exposed to JS. Then if the
preventDefault() is called, PostHandleEvent wouldn't be called, but
script would override the default handling.

About the patch:
- s/true/PR_TRUE/
- Add some documentation about nsGestureNotifyEvent
Attachment #392621 - Flags: review?(Olli.Pettay) → review-
Here's a new version of the patch:
- addressing the comments below
- more documentation about DecideGestureEvent and nsGestureNotifyEvent
- moved to nsEventStateManager and called from PostHandleEvent

also, minor change: added ePanBoth direction to the enum. We don't use it right now (as I described, with that it's not possible to select text because it would be an ambiguous action), but it's not a reason to keep it off the list since it's possible to set up that way and could be used in the future.
Attachment #392621 - Attachment is obsolete: true
Attachment #393946 - Flags: review?(Olli.Pettay)
Attachment #392621 - Flags: review?(roc)
Attachment #393946 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 393946 [details] [diff] [review]
Using WM_GESTURENOTIFY - v2.2

>+nsEventStateManager::DecideGestureEvent(nsGestureNotifyEvent* aEvent,
>+                              nsIFrame* targetFrame)
Align the 2nd parameter under the 1st.

>+{
>+
>+  NS_ASSERTION(aEvent->message == NS_GESTURENOTIFY_EVENT_START,
>+               "DecideGestureEvent called with a non-gesture event");
>+
>+  /* Check the ancestor tree to decide if any frame is willing* to receive a
>+   * scroll event. If that's the case, the current touch gesture will be used
>+   * as a pan gesture; otherwise it'll be a regular mousedown/mousemove/click
>+   * event.
>+   *
>+   * *willing: determine if it makes sense to pan the element using scroll events:
Nit, scroll events don't do anything, they happen after scrolling. So you mean something else. DOMMouseScroll/MozPixelScroll?

>+#ifdef MOZ_XUL
>+    // XXX For now, trees are complex to decide if we should pan or not
>+    // (based if all rows are visible or not), so we will just always
>+    // pan which is not so bad.
>+    nsTreeBodyFrame* treeFrame = do_QueryFrame(current);
>+    if (treeFrame) {
>+      panDirection = nsGestureNotifyEvent::ePanVertical;
>+      break;
>+    }
Would it be really that difficult to check "scrollability"?
nsTreeBodyFrame has mVerticalOverflow and mHorizontalOverflow.
Just add some getter methods to check those?
Or do you need some other information too?


>+#endif
>+
>+    nsIScrollableFrame* scrollableFrame = do_QueryFrame(current);
>+    if (scrollableFrame) {
>+      if (current->GetContent() && current->GetContent()->IsNodeOfType(nsINode::eXUL)) {
What if xul element is styled like any normal html element? Or vise-versa
http://mozilla.pettay.fi/moztests/xulinxhtml.xhtml
I know, perhaps not a very important case, but still something to test.
nsIFrame::IsFrameOfType() with nsIFrame::eXULBox might be useful.


>@@ -2781,16 +2887,22 @@ nsEventStateManager::PostHandleEvent(nsP
...
>+  case NS_GESTURENOTIFY_EVENT_START:
>+    {
>+      DecideGestureEvent(static_cast<nsGestureNotifyEvent*>(aEvent), mCurrentTarget);
>+    }
>+    break;
if (nsEventStatus_eConsumeNoDefault != *aStatus) {
  DecideGestureEvent(static_cast<nsGestureNotifyEvent*>(aEvent), mCurrentTarget);
}
Just in case someone wants to prevent the default handling.

>+++ b/layout/base/nsPresShell.cpp
>@@ -6149,16 +6149,17 @@ PresShell::HandleEvent(nsIView         *
>         return shell->HandlePositionedEvent(subshellRootView, targetFrame,
>                                             aEvent, aEventStatus);
>       }
>     }
>     
>     if (!targetFrame) {
>       targetFrame = frame;
>     }
>+
>     return HandlePositionedEvent(aView, targetFrame, aEvent, aEventStatus);
>   }
No need for this extra newline.

>+/*
>+ * Gesture Notify Event:
>+ *
>+ * This event is the first event generated when the user touches
>+ * the screen with a finger, and it's meant to decide what kind
>+ * of action we'll use for that touch interaction.
>+ *
>+ * The event is dispatched to the UI
Nit, perhaps not the best wording here, "UI".
Though, not sure what would be better. "layout"?

r+ if you either fix the tree handling or explain why it is so complex, and
also check if using IsFrameOfType would make more sense.

I wish I could test this all. Perhaps time to get a laptop with touch screen and win7 :)
Attached patch Using WM_GESTURENOTIFY - v2.3 (obsolete) — Splinter Review
Patch addressing Olli's comments above, carrying forward r+

(In reply to comment #17)
> Nit, scroll events don't do anything, they happen after scrolling. So you mean
> something else. DOMMouseScroll/MozPixelScroll?
Yes, it was MozPixelScroll

> 
> >+#ifdef MOZ_XUL
> >+    // XXX For now, trees are complex to decide if we should pan or not
> >+    // (based if all rows are visible or not), so we will just always
> >+    // pan which is not so bad.
> >+    nsTreeBodyFrame* treeFrame = do_QueryFrame(current);
> >+    if (treeFrame) {
> >+      panDirection = nsGestureNotifyEvent::ePanVertical;
> >+      break;
> >+    }
> Would it be really that difficult to check "scrollability"?
> nsTreeBodyFrame has mVerticalOverflow and mHorizontalOverflow.
> Just add some getter methods to check those?
> Or do you need some other information too?
Thanks for the suggestion, worked fine! I didn't touch the <tree>
before because I was afraid there were deep magic going on there,
but this gives the correct information needed.

> 
> 
> >+#endif
> >+
> >+    nsIScrollableFrame* scrollableFrame = do_QueryFrame(current);
> >+    if (scrollableFrame) {
> >+      if (current->GetContent() && current->GetContent()->IsNodeOfType(nsINode::eXUL)) {
> What if xul element is styled like any normal html element? Or vise-versa
> http://mozilla.pettay.fi/moztests/xulinxhtml.xhtml
> I know, perhaps not a very important case, but still something to test.
> nsIFrame::IsFrameOfType() with nsIFrame::eXULBox might be useful.

Changed it for eXULBox and the behavior IMO is indeed better on the test page. The xul <box> with overflow:hidden in the page now don't decide for panning which is better.
Also with this we don't need to use the nsIContent of the frame, so it's a good change.

> if (nsEventStatus_eConsumeNoDefault != *aStatus) {
>   DecideGestureEvent(static_cast<nsGestureNotifyEvent*>(aEvent),
> mCurrentTarget);
> }
> Just in case someone wants to prevent the default handling.

Ok, done.

> >+ * The event is dispatched to the UI
> Nit, perhaps not the best wording here, "UI".
> Though, not sure what would be better. "layout"?

Yeah, layout sounds better for me too.


What other reviews or superreviews do I need for this patch?
Attachment #394136 - Flags: review+
Attached patch Using WM_GESTURENOTIFY - v2.4 (obsolete) — Splinter Review
Same as before, but this one actually applies to tree
Attachment #394136 - Attachment is obsolete: true
Attachment #394178 - Flags: review+
Pushed to the try server
(In reply to comment #18) 
> What other reviews or superreviews do I need for this patch?
Maybe roc could still look at the patch?
+   * *willing: determine if it makes sense to pan the element using scroll events:
+   *   - For web content: if there are any visible scrollbars on the touch point
+   *   - For XUL: if it's an scrollable element that can currently scroll in some direction.
+   *
+   * Note: we'll have to one-off various cases to ensure a good usable behavior
+   */
+  nsGestureNotifyEvent::ePanDirection panDirection = nsGestureNotifyEvent::ePanNone;
+  PRBool displayPanFeedback = PR_FALSE;
+  for (nsIFrame* current = targetFrame; current; current = nsLayoutUtils::GetCrossDocParentFrame(current)) {

These lines seem pretty long.

The documentation for DecideGestureEvent should make it clear that the decision is stored in aEvent.

Otherwise looks great.
Thanks! Added remark about storing the decision in aEvent and broke the long lines.
Attachment #394178 - Attachment is obsolete: true
Attachment #394338 - Flags: review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/5ae98dff8d5d

We'll wait a couple of days prior to requesting approval1.9.2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 504913
Duplicate of this bug: 503888
Attachment #394338 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 394338 [details] [diff] [review]
Using WM_GESTURENOTIFY - v2.5

a191=beltzner
pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b0dda4a9e728
Keywords: fixed1.9.2
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2b1
Blocks: 548100
You need to log in before you can comment on or make changes to this bug.