Double-tap gestures are not recognized on OS X

RESOLVED FIXED in mozilla23

Status

()

Core
DOM: Events
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Brandon Waterloo, Assigned: Brandon Waterloo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla23
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

5 years ago
Double-tap gestures are not recognized in widget/cocoa/nsChildView.mm.  This is the "smart-zoom" gesture on OS X--two fingers tapping twice each (for a total of four touches).

Implement recognition of this gesture.

NOTE: Cocoa doesn't automagically recognize this gesture like it does for rotate, zoom, and swipe--recognition must be implemented manually.
(Assignee)

Updated

5 years ago
Assignee: nobody → waterlo1
Blocks: 674371
(Assignee)

Updated

5 years ago
Summary: Double-tap gestures are not recognized → Double-tap gestures are not recognized on OS X
Status: NEW → ASSIGNED
Component: General → DOM: Events
Product: Firefox → Core
Version: 21 Branch → Trunk
This needs to be a chrome-only event like the other gestures that we currently support.
Created attachment 725597 [details] [diff] [review]
Double-tap gesture intergration v0.0

First patch submitted to address double tap recognition on OS X, please comment and provide feedback. Thanks!
Attachment #725597 - Flags: feedback?(jAwS)
See Also: → bug 688990
Comment on attachment 725597 [details] [diff] [review]
Double-tap gesture intergration v0.0

Review of attachment 725597 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +800,5 @@
>        case "MozTapGesture":
>          aEvent.preventDefault();
> +        //this._doAction(aEvent, ["tap"]);
> +        console.log("Lets do some double-tapping!");
> +        dump("Lets do some double-tapping\n");

I think this file can be left alone.

::: dom/interfaces/events/nsIDOMSimpleGestureEvent.idl
@@ +48,5 @@
>   *
>   * MozTapGesture - Generated when the user executes a two finger
>   * tap gesture on the input device. Client coordinates contain the
>   * center point of the tap.
> + * (XXX Mac only)

Changing from "not implemented on Mac" to "Mac only" is incorrect.

::: widget/cocoa/nsChildView.h
@@ +249,5 @@
>    // mCumulativeRotation keeps track of the total amount of rotation
>    // performed during a rotate gesture so we can send that value with
>    // the final MozRotateGesture event.
> +  //
> +  // mResetTapZoom keeps track of whether tap zoom should be reset to

mResetTapZoom isn't defined in this patch. Did you intend to reference mResetTap (which is commented out)?

@@ +277,5 @@
> +  // within the double tap gesture interval.
> +  //
> +  // mIsTouched is used to detect whether a touch event has occured
> +  // within the touch gesture interval.
> +  NSTimeInterval mFirstTapTime;

It would be better if you placed these descriptions next to the variables they are describing.

::: widget/cocoa/nsChildView.mm
@@ +109,5 @@
>  #endif
>  
>  bool gUserCancelledDrag = false;
>  
> +NSTimeInterval touchInterval = 0.1;

Were you able to get this interval from OS X? Is there an API to query in case Apple changes the detection heuristics?

@@ +2941,5 @@
> +      NSTimeInterval deltaTapTime = [NSDate timeIntervalSinceReferenceDate] - mFirstTapTime;
> +      if (deltaTapTime <= [NSEvent doubleClickInterval]) {
> +        [self tapWithEvent: anEvent];
> +      }
> +      mIsTapped = false;

Setting mIsTapped to false here is a misrepresentation. Maybe there is a better name for the variable so it doesn't look awkward that on the tap event you are setting isTapped to false?

@@ +2947,5 @@
> +      mFirstTapTime = [NSDate timeIntervalSinceReferenceDate];
> +      mIsTapped = true;
> +    }
> +  } else if([touches count] == 1) {
> +    if(mIsTouched) {

mIsTouched is never set to true, so the code within this block is effectively dead.

@@ +2985,5 @@
> +
> +  nsAutoRetainCocoaObject kungFuDeathGrip(self);
> +
> +  switch (mGestureState) {
> +  case eGestureState_TapGesture:

nit, indent the case statements two spaces from the switch so it is easier to see the control flow here.
Attachment #725597 - Flags: feedback?(jAwS)
Created attachment 727094 [details] [diff] [review]
Double-tap gesture intergration v0.1

mResetTapZoom isn't defined in this patch. Did you intend to reference mResetTap (which is commented out)?

I removed both these member variables they should not be placed in this area of the code.

Were you able to get this interval from OS X? Is there an API to query in case Apple changes the detection heuristics?

Yes

NSTimeInterval - https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Miscellaneous/Foundation_DataTypes/Reference/reference.html

doubleClickInterval - https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSEvent_Class/Reference/Reference.html

Setting mIsTapped to false here is a misrepresentation. Maybe there is a better name for the variable so it doesn't look awkward that on the tap event you are setting isTapped to false?

mIsTapped was renamed to mAcceptSecondTap.

mIsTouched is never set to true, so the code within this block is effectively dead.

Code that was forgotten was added to allow for two touch recognition that does not occur at exactly the same time, but rather +/- 0.1 sec from each other.
Attachment #725597 - Attachment is obsolete: true
Attachment #727094 - Flags: feedback?(jAwS)
Attachment #727094 - Attachment is patch: true
Comment on attachment 727094 [details] [diff] [review]
Double-tap gesture intergration v0.1

Review of attachment 727094 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsChildView.mm
@@ +109,5 @@
>  #endif
>  
>  bool gUserCancelledDrag = false;
>  
> +NSTimeInterval touchInterval = 0.1;

How did you arrive at the value of 0.1 here? Is there a way for a user to change this value on their machine such that this hardcoded-value of 0.1 will not be inline with the user's expectations?
Attachment #727094 - Flags: feedback?(jAwS)
Created attachment 727521 [details] [diff] [review]
Double-tap gesture intergration v0.2
Attachment #727094 - Attachment is obsolete: true
Attachment #727521 - Flags: feedback?(jAwS)
Comment on attachment 727521 [details] [diff] [review]
Double-tap gesture intergration v0.2

Review of attachment 727521 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsChildView.mm
@@ +2071,5 @@
> +
> +    touchInterval = 0.0;
> +    // Set up the touch interval
> +    if (touchInterval > [NSEvent doubleClickInterval]) {
> +      touchInterval = ((long) [NSEvent doubleClickInterval]) / 5.0;

This code is a little confusing. It seems that the only case the if-condition will not be accepted is if doubleClickInterval is zero or negative. Dividing zero by five will still result in zero, so it seems that you could unconditionally set touchInterval to doubleClickInterval/5. Is it possible for doubleClickInterval to have a negative value?

I do have one further question. Why is the doubleClickInterval being divided by 5 here?

@@ +2952,5 @@
> +      mAcceptSecondTap = true;
> +    }
> +  } else if([touches count] == 1) {
> +    if(mIsTouched) {
> +      NSTimeInterval deltaTouchTime = [NSDate timeIntervalSinceReferenceDate] - mFirstTouchTime;

Is `timeIntervalSinceReferenceDate` the best choice here? The documentation, https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSDate_Class/Reference/Reference.html, says that dates prior to 1 Jan 2001 are negative. I worry that this would break in unexpected ways if the user changed their system clock.

If you can find a monotonically increasing, non-negative introducing, timer to use here that would be preferred. You may be able to find one in Gecko, since `window.performance.now()` has these characteristics.

@@ +3039,5 @@
> +}
> +
> +- (void)tapWithEvent:(NSEvent *)anEvent
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;

I'm not familiar with this. Can you explain why the body of this function needs to be wrapped in a try-abort block? Is this the standard practice within other parts of this file?
Attachment #727521 - Flags: feedback?(jAwS) → feedback+

Comment 8

5 years ago
For reference, I'm attaching the email which Bill sent:
-------------------------------------------------------
I am currently working with a group of students to add to the current gesture features available to Firefox OS X. Currently, I have been trying to get the double tap gesture recognition implemented for OS X because it is not a standard gesture that is recognized by OS X. Therefore, I have had to create a custom touch gesture to identify a double tap occurrence. I have submitted a few patches on to Bugzilla but had been told by jaws [Jared Wein] to contact you both and explain my patch in detail.

Here is what I am doing:

- Allow for the handling of Multi-Touch Events.
- Recognize a double tap gesture by recognizing a two individual touch events that occur either simultaneously or that occur within the touchInterval (a value that is arbitrarily set to be a fifth of the value of doubleClickInterval, which can be altered in the system settings.) If the following touch event is repeated within the doubleClickInterval a double tap event is recognized and the function tapWithEvent is triggered.

     * NOTE: The reason we chose to include the recognition of a two touch event that occurs within a        touchInterval; that is, two one touch events that occur within a very short time lapse (a touchInterval,) is to accommodate for the fact that it is difficult to reproduce a two touch event because that requires the fingers to touch at the exact same time. Therefore, we chose to include this touch event in addition, so that we could recognize a double tap event with greater ease.
-------------------------------------------------------

If I understand correctly, you add both single two-fingers-tap detection, and also double two-fingers-tap (which is what you actually want here) detection? Also, from your explanation I understand that cocoa does support a single two-fingers-tap event, but it's not clear to me from your explanation whether Gecko already uses/hooks it.

I would think that if Gecko already has single two-fingers-tap event on OSX, then you should just detect two of those which happen within cocoa's double-click interval. If Gecko doesn't already hook the single two-fingers-tab event, then it should be added first directly from the cocoa event, and then follow up with the other part (detect two of those within normal double-click interval).
Created attachment 729152 [details] [diff] [review]
Double-tap gesture intergration v0.3

jaws:

::: widget/cocoa/nsChildView.mm
@@ +2071,5 @@
> +
> +    touchInterval = 0.0;
> +    // Set up the touch interval
> +    if (touchInterval > [NSEvent doubleClickInterval]) {
> +      touchInterval = ((long) [NSEvent doubleClickInterval]) / 5.0;

I do have one further question. Why is the doubleClickInterval being divided by 5 here?

    I am not sure how else to relate touchInterval to something that can be 
    modified in system setting (doubleClickInterval). I divide it by five so 
    that I get one millisecond, by default, which seems to be a fair margin 
    for a lag in time between the fingers touching the trackpad.

@@ +2952,5 @@
> +      mAcceptSecondTap = true;
> +    }
> +  } else if([touches count] == 1) {
> +    if(mIsTouched) {
> +      NSTimeInterval deltaTouchTime = [NSDate timeIntervalSinceReferenceDate] - mFirstTouchTime;

Is `timeIntervalSinceReferenceDate` the best choice here? The documentation, https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSDate_Class/Reference/Reference.html, says that dates prior to 1 Jan 2001 are negative. I worry that this would break in unexpected ways if the user changed their system clock.

    I am only using it to find the differences, which should be positive; 
    useless the user changes the time and date on his computer. Assuming that 
    specific scenario occurs, isn't better is I just check that the differences 
    are also greater than zero?

@@ +3039,5 @@
> +}
> +
> +- (void)tapWithEvent:(NSEvent *)anEvent
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;

I'm not familiar with this. Can you explain why the body of this function needs to be wrapped in a try-abort block? Is this the standard practice within other parts of this file?

     I use NS_OBJC_BEGIN_TRY_ABORT_BLOCK because that seems to be the standard
     practice with other parts of the bug that accept gestures (e.i. 
     tapWithEvent, swipeWithEvent…).


avih:

If I understand correctly, you add both single two-fingers-tap detection, and also double two-fingers-tap (which is what you actually want here) detection? Also, from your explanation I understand that cocoa does support a single two-fingers-tap event, but it's not clear to me from your explanation whether Gecko already uses/hooks it.

   No, unfortunately cocoa does not support a single two-finger-tap event. The 
   gestures that are supported are: 
   
    - Pinch gesture (zoom in or out)
    - Rotate gesture (clockwise or counterclockwise)
Three fingers brushing across the trackpad surface in a common direction is a swipe gesture.

    Two fingers moving vertically or horizontally is a scroll gesture.

I would think that if Gecko already has single two-fingers-tap event on OSX, then you should just detect two of those which happen within cocoa's double-click interval. If Gecko doesn't already hook the single two-fingers-tab event, then it should be added first directly from the cocoa event, and then follow up with the other part (detect two of those within normal double-click interval).
Attachment #727521 - Attachment is obsolete: true
Attachment #729152 - Flags: feedback?(masayuki)
Attachment #729152 - Flags: feedback?(jAwS)
Attachment #729152 - Flags: feedback?(avihpit)
Comment on attachment 729152 [details] [diff] [review]
Double-tap gesture intergration v0.3

Review of attachment 729152 [details] [diff] [review]:
-----------------------------------------------------------------

jaws:

***CORRECTION: should say 100 milliseconds not 1 millisecond.

avih (Please ignore your part in the last comment):

If I understand correctly, you add both single two-fingers-tap detection, and also double two-fingers-tap (which is what you actually want here) detection? Also, from your explanation I understand that cocoa does support a single two-fingers-tap event, but it's not clear to me from your explanation whether Gecko already uses/hooks it.

   No, unfortunately cocoa does not support a single two-finger-tap event. 
   The gestures that are supported on OS X are: 
   
    - Pinch gesture (zoom in or out.)
    - Rotate gesture (clockwise or counterclockwise.)
    - Swipe gesture (up or down.)
    - Scroll gesture (right or left.)

   Source: http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/EventOverview/HandlingTouchEvents/HandlingTouchEvents.html#//apple_ref/doc/uid/10000060i-CH13-SW10   

   Because of this I had to enable multi-touch events, whereby I recognized how many 
   fingers where touching the trackpad at a specific time. Doing this enabled to 
   recognize a tap event and through the method I explained, allow us to recognize a
   tap gesture that was not exactly in sync.
 
I would think that if Gecko already has single two-fingers-tap event on OSX, then you should just detect two of those which happen within cocoa's double-click interval. If Gecko doesn't already hook the single two-fingers-tab event, then it should be added first directly from the cocoa event, and then follow up with the other part (detect two of those within normal double-click interval).

    My understanding is that a single tap event is a equivalent to a right click; for 
    this reason I choose to ignore a single double tap event and only pass a double
    tap event.
(In reply to Bill de Araujo [:spartanfire] from comment #10)
>     My understanding is that a single tap event is a equivalent to a right
> click; for 
>     this reason I choose to ignore a single double tap event and only pass a
> double
>     tap event.

So if you try to detect double right-click events, would that actually detect (also) double two-fingers-tap events? If yes, then wouldn't it be easier to detect these existing events, and then (maybe) apply some logic to disregard those which don't originate from the touchpad/magic-mouse?
avih:

It is only by coincidence that a two-finger tap gesture is equivalent to a right click. This is handled by the operating system and cannot be caught as an event or can it be overriden. It is not a gesture which the OS shares with other applications. Therefore, a right-click event is not a gesture that we can recognize as half of our double-tap.

Moreover, it would not make sense to implement the gesture recognition as two right clicks in a row--that is not intuitive.  It seems unlikely that the operating system would include the source of a right-click (mouse button vs. touchpad), and even if it did, it still seems inappropriate to hijack this behavior.
Attachment #729152 - Flags: feedback?(jAwS)
I'm trying to understand what are you doing. However, I cannot test the patch with following testcase:

data:text/html;charset=utf-8,<script>function log(e) { var text = document.createTextNode(e.type); var li = document.createElement("li"); li.appendChild(text); document.getElementById("log").appendChild(li); } window.addEventListener("MozTapGesture", log, true);</script><ul id="log"></ul>

My environment is 10.7, do I need 10.8 for that?

nit:

You're using some coding styles for {}.

You usually use:

if ()
{

}
else {

}

but our coding rule is, you have to use:

if () {

} else {

}

style. And also, don't omit {} even if the content of if block is only one line.
masayuki:

I'm not sure how to generate the error you had. Could you please tell me how I could reproduce it?

In addition, the development environment I use is 10.8, but I believe that should not be a problem because NSTouch is available on 10.6 and later.
Hmm, I loaded the test case mentioned in comment 13 (the data URI) and did tap/double-tap/two-finger tap on its body. However, I didn't see any MozTabGesture event log in the testcase.
(Assignee)

Comment 16

5 years ago
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> Hmm, I loaded the test case mentioned in comment 13 (the data URI) and did
> tap/double-tap/two-finger tap on its body. However, I didn't see any
> MozTabGesture event log in the testcase.

My suspicion is that it has to do with this line:  https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#771

This prevents the event from reaching content (it is desired that these remain chrome-only events), which in your case, might be preventing that quick script you were using from receiving the events.  Could you try putting a logging event into browser/base/content/browser.js and re-making obj-.../browser/ ?
Hmm, thank you. But how did you test it? I'll try to check it with your approach or putting printf() into the C++ part.

Sigh, I think that browser.js should have a pref for easy to test the gesture event generation. I'll file a bug for that.
I filed bug 855916. And test your patch again with the patch. However, I feel that it's difficult to fire the event. Could you check it? I'm not sure if it's intentional behavior.
(Assignee)

Comment 19

5 years ago
Here's a copy of an email I recently sent that explains in some detail why this bug exists and how (at a high level) we propose to fix it (i.e. implement double-tap recognition):

We're working on a double-tap (aka Smart Zoom) for gesture support on OS X.  Since it's not an officially supported event in Cocoa (http://developer.apple.com/library/mac/#documentation/cocoa/conceptual/EventOverview/HandlingTouchEvents/HandlingTouchEvents.html), unlike pinch zoom and rotate, we've got to write recognition for it on our own.

Essentially, what we're calling "double tap" is this:  two fingers land at approximately the same time (allowing for human imprecision), and then immediately after, release at approximately the same time (half of a double tap); and then touch and release again.  This is four total touches and releases.

We're experiencing a lot of difficulty getting the gesture to be recognized reliably.  One major issue is the aforementioned human imprecision.  Relative to the trackpad, the touches almost always land at different times, so some sort of tolerance must be established to account for this.  We need to set up the functions "touchesBeganWithEvent" and "touchesEndedWithEvent" so that they can effectively and reliably recognize a two-finger tap (half of a double-tap) even with human imprecision, and then have a simple counter/state to determine whether 0, 1, or 2 two-finger taps have come in--and if 2, then send the gesture event to the application.

We also need a timer of sorts, so that if 1 two-finger tap comes in and too much time passes (e.g. the double click time for the mouse is a good option), it will reset the counter back to 0.
Created attachment 733512 [details] [diff] [review]
Double-tap gesture intergration v0.4
Attachment #733512 - Flags: review?(masayuki)
(Assignee)

Updated

5 years ago
Attachment #729152 - Attachment is obsolete: true
Attachment #729152 - Flags: feedback?(masayuki)
Attachment #729152 - Flags: feedback?(avihpit)
(Assignee)

Updated

5 years ago
Attachment #733512 - Flags: review?(masayuki) → feedback?(masayuki)
Sorry for the delay.

I tested with the new patch. But I still feel that it's difficult to fire MozTapGesture.

I was thinking that the time for double click isn't enough for double tap. However, it's not so, perhaps. When I test it with the latest your patch, sometimes:

1. MozTapGesture fired as expected
2. MozTabGesture fired at third tap, sometimes
3. MozTapGesture fired twice during three taps, sometimes

I'm still trying to understand what makes this bad feel.
Comment on attachment 733512 [details] [diff] [review]
Double-tap gesture intergration v0.4

Okay, I misunderstand the patch. [touches count] means number of fingers. Reading your patch again with this knowledge, I feel your patch very strange. Do you understand this correctly?

First, your patch may dispatch only one finger taps. I feel it's unexpected behavior of your concept.

Next, if you touch one-finger first, then, touch second-finger, and release them, then, first two finger tap causes firing MozTapGesture event. That's wired.

Finally, if my first second two-finger tap is detected as second tap but it's already timed out, then, it doesn't handled as first tap. Then, I need 3 times two-finger taps for firing MozTapGesture event.

So, I think that you need to redesign the patch with following points:

1. You don't need to dispatch the event at one-finger tap because I don't see two-finger tap causes one-finger tap event after two-finger tap event.
2. You shouldn't make mAcceptSecondTap true at one-finger tap.
3. You must not reset mAcceptSecondTap when two-finger tap is occurred with mAcceptSecondTap=true after it's timed out.

So, I feel this is enough:

if ([touches count] != 2) {
  return;
}

if (mAcceptSecondTap) {
  NSTimeInterval deltaTapTime =
    [NSDate timeIntervalSinceReferenceDate] - mFirstTapTime;
  if (deltaTapTime <= [NSEvent doubleClickInterval] &&
      deltaTapTime > 0.00) {
    [self tapWithEvent: anEvent];
    return;
  }
}
mAcceptSecondTap = true;
mFirstTapTime = [NSDate timeIntervalSinceReferenceDate];

isn't it?
Attachment #733512 - Flags: feedback?(masayuki) → feedback-
(Assignee)

Comment 23

5 years ago
Created attachment 734811 [details] [diff] [review]
Double-tap gesture integration v0.5

This version uses Masayuki's method along with some cleanup of unused variables and a bit of refactoring.

Overall, it's simpler and seems to recognize better than the previous version, and doesn't get stuck in intermediate states.

Please note:  this seems to work fairly well, but it still feels like Safari's recognition is better.  We'd also like to hear from Josh Aas and see if he's had any success in his research.
Attachment #733512 - Attachment is obsolete: true
Attachment #734811 - Flags: feedback?(masayuki)
Comment on attachment 734811 [details] [diff] [review]
Double-tap gesture integration v0.5

Almost looks good for me.

>+- (void)touchesBeganWithEvent:(NSEvent *)anEvent
>+{
>+  if (!anEvent) {
>+    return;
>+  }
>+
>+  // Set up for recognition of a double tap gesture
>+  NSSet* touches = [anEvent touchesMatchingPhase:NSTouchPhaseTouching inView:self];

nit: This line is over 80 characters.

>+
>+  if ([touches count] != 2) {
>+    return;
>+  }

I'm sorry. My suggestion isn't enough for here. I realized that 3 finger double top sometimes causes MozTapGesture event. The reason is probably events are fired following order:

1. 2 finger tap
2. 3 finger tap
3. 2 finger tap
4. 3 finger tap

So, at #2, we need to reset the double tap gesture state.

I.e., this should be:

  NSUInteger fingers = [touches count];
  if (fingers != 2) {
    // Cancel double tap if three or more fingers tap is detected
    if (mGestureState == eGestureState_TapGesture && fingers > 2) {
      mGestureState = eGestureState_None;
    }
    return;
  }

With this change, I don't see any problems.
Attachment #734811 - Flags: feedback?(masayuki) → feedback+
(Assignee)

Comment 25

5 years ago
Created attachment 735258 [details] [diff] [review]
Double-tap gesture integration v1.0

This patch includes all previous feedback.  Hopefully this is a final version, I included the standard patch headers if it's acceptable as a final version. :)
Attachment #734811 - Attachment is obsolete: true
Attachment #735258 - Flags: review?(masayuki)
Comment on attachment 735258 [details] [diff] [review]
Double-tap gesture integration v1.0

Looks good for me.

However, I suggested a lot of thing for this. So, another review should review this too. Steven, are you a good person for reviewing the gesture part of Cocoa? If not so, please fwd the review to Josh or another person.
Attachment #735258 - Flags: review?(smichaud)
Attachment #735258 - Flags: review?(masayuki)
Attachment #735258 - Flags: review+
Comment on attachment 735258 [details] [diff] [review]
Double-tap gesture integration v1.0

I've looked through this and can't find any problems with the code (with one caveat, which I'll discuss below).

I've also done some testing -- not (unlike Masayuki) by trying to consume (or otherwise make use of) these events in cross-platform code, but by adding logging to nsChildView.mm (particularly -[ChildView tapWithEvent:]).  I've found that the code works reasonably well, but probably not as well as it should.  About two thirds of my double-tap-with-two-fingers taps get recognized.  But sometimes things get into a state where no further events get recognized until I either wait 20-30 seconds or click on the desktop and then back on a browser window.

I have no idea what's causing these problems, and I suspect it'll take serious reverse engineering of the OS to find out.  I'm very unlikely to have time to do this myself in the foreseeable future.

So I'm willing to r+ this patch, but it's a pretty tepid r+.

Here's the "caveat" I mentioned above:

When (in -[ChildView tapWithEvent:]) an nsSimpleGestureEvent is created and dispatched to Gecko, its clickCount doesn't get set (it's left at the default value of '0').  I suspect this isn't correct.  But I know nothing about tap events, so I can't say for sure.

And here's a nit:

 - (void)magnifyWithEvent:(NSEvent *)anEvent;
+- (void)tapWithEvent:(NSEvent *)anEvent;
 - (void)rotateWithEvent:(NSEvent *)anEvent;

The declaration of tapWithEvent: shouldn't be listed togther with the other "...WithEvent" methods, but separately and below.  The other methods are genuine instance methods of NSResponder, but tapWithEvent: (of course) isn't.
Attachment #735258 - Flags: review?(smichaud) → review+
(Following up comment #27)

On further thought:  I think it's worth taking this patch on the trunk for now, to give it wider testing.  But if no way can be found to improve its reliability, it should probably be backed out again at some point.

And to repeat what I said above, I'm not going to have any time to devote to making this patch more reliable.  So the work will need to be done by others.
I should mention that all my tests were performed on OS X 10.7.5.  Shortly I'll retest on OS X 10.8.3.
(Assignee)

Comment 30

5 years ago
Created attachment 735876 [details] [diff] [review]
Double-tap gesture integration v1.1

This patch is the same as the previous, but incorporates Steven's feedback:

--Since all other gesture events send clickCount = 0, I decided to leave this event as it was
--Fixed the nit, moving tapWithEvent down a bit and adding a comment to note that it's not a genuine nsResponder method.
Attachment #735258 - Attachment is obsolete: true
Attachment #735876 - Flags: review?(smichaud)
(Assignee)

Comment 31

5 years ago
(In reply to Steven Michaud from comment #29)
> I should mention that all my tests were performed on OS X 10.7.5.  Shortly
> I'll retest on OS X 10.8.3.

We've been working on both 10.8.2 and 10.8.3 and have had fairly good recognition.  

Also, we've not encountered a state where it refuses to recognize anything--could it be that you accidentally had a finger resting on the trackpad?
> Also, we've not encountered a state where it refuses to recognize
> anything--could it be that you accidentally had a finger resting on
> the trackpad?

No.

You guys really should be testing on all the versions of OS X
supported by Firefox (currently back to 10.6).  You don't need to test
all the minor releases (the point releases).  But you do need to test
the last point release for both 10.7 (10.7.5) and 10.6 (10.6.8).

And (once this patch lands) you should encourage people to test your
addon on those versions of OS X.
Comment on attachment 735876 [details] [diff] [review]
Double-tap gesture integration v1.1

> --Since all other gesture events send clickCount = 0, I decided to
>   leave this event as it was

When Windows sends a NS_SIMPLE_GESTURE_TAP event, it sets clickCount
to '1':

http://hg.mozilla.org/mozilla-central/annotate/ee5ca214e87c/widget/windows/nsWinGesture.cpp#l395
Attachment #735876 - Flags: review?(smichaud) → review+
(Assignee)

Comment 34

5 years ago
I tried it with MozRotateGesture, MozRotateGestureUpdate, MozMagnifyGesture, and MozMagnifyGestureUpdate, all yielded clickCount = 0 on OS X.  I couldn't get MozSwipeGesture to propagate through to content (where I was testing it) but a quick look through the swipe code seems to indicate that it also doesn't set click count.
(Assignee)

Comment 35

5 years ago
...however, looking through the Windows code, it looks like it doesn't set click count for Magnify / Rotate either, so for the sake of parity w/ Windows, I'll set click count to 1.  Will re-upload patch shortly.
> I'll set click count to 1

It may turn out that '0' is right.  (And like I said, I know nothing about tap events.)  I just wanted to make sure you're aware that the value of clickCount might need to be changed.
(Assignee)

Comment 37

5 years ago
Created attachment 735900 [details] [diff] [review]
Double-tap gesture integration v1.2

This version sets click count to 1 for parity w/ Windows.
Attachment #735876 - Attachment is obsolete: true
Attachment #735900 - Flags: review?(smichaud)
Comment on attachment 735900 [details] [diff] [review]
Double-tap gesture integration v1.2

OK.  But as I said above this may need to change.
Attachment #735900 - Flags: review?(smichaud) → review+
I've now also tested on OS X 10.6.8 and 10.8.3, on two different
MacBook Pros, with reasonably good results.

I'm no longer so sure the following is true:

> But sometimes things get into a state where no further events get
> recognized until I either wait 20-30 seconds or click on the desktop
> and then back on a browser window.

But only about 2/3 to 3/4 of my double-taps-with-two-fingers get
recognized.  If enough others have the same difficulty, you guys will
need to do something about it.

Comment 40

5 years ago
Brandon, if you can get a build I can test it for you? I am currently on a 2012 MPB with Mountain Lion.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/29a5fd2889f3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

5 years ago
Depends on: 864053
Created attachment 740372 [details] [diff] [review]
Backout to fix bug 864053

We need to backout this patch to fix bug 864053.

I hope that a future patch for bug 863841 will restore support for double-tap gestures without breaking other things.  But that may take a while.
Attachment #740372 - Flags: review?(bgirard)
Brandon, please try to figure out (by trial and error) which part(s) of your patch caused bug 864053.

If NSEventTypeSmartMagnify is (as I suspect) only supported on Mountain Lion and up, we may need to keep using some version of this patch to support double-tap events on Lion.
(Assignee)

Comment 45

5 years ago
I'd like to see the NSEventTypeSmartMagnify tested on Lion before I spend time nit picking through my patch. I strongly suspect it's the [self acceptTouch Events] or whatever that line is. Without that line, none of the patch works so it's not worth testing.
> I strongly suspect it's the [self acceptTouch Events]

Please test that!  It should be easy for you.

I'd do it myself, but I don't have the time.
(Assignee)

Comment 47

5 years ago
If I removed that line, not a single line of the remaining code will execute, so it will determine nothing that we don't already know.  However, we may not need any of those other lines, if smartMagnifyWithEvent works on Lion, in which case, all that time I spent testing would be wasted anyway. That said, since testing can't yield any interesting results and if it works on Lion those results will be unnecessary, I'd like to wait to hear if this works on Lion.

Also, FWIW, after May 3 I will no longer have any access to any OS X machines, so if we can't land anything by then, I won't be able to do it.
As I expected, NSEventTypeSmartMagnify is only supported on OS X 10.8 and up.  See Apple's NSEvent class reference.
This seems to have also caused a regression in bug 862417.
(Assignee)

Comment 50

5 years ago
Backing this patch out should fix both bug 862417 and bug 864053. Then, assuming bug 863841 is as easy to fix as I'm hoping it is (but won't know until further testing), we should be able to fix this bug without re-breaking the others...
Comment on attachment 740372 [details] [diff] [review]
Backout to fix bug 864053

r+ for backing out patch causing regressions.
Attachment #740372 - Flags: review?(bgirard) → review+
I'll reopen this bug after the backout lands on mozilla-central.
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/d813280d88f3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 55

5 years ago
Now that bug 863841 is fixed, this can probably be marked as fixed, since that bug was essentially a duplicate of this one since this one got backed out.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.