[Windows] Add onContextMenu callback to GestureView #599

thezero · 2018-04-17T13:44:33Z

When using RX.GestureView as an interaction handler, it's impossible to detect right mouse button clicks right now in UWP. This info is already propagated from native code in the event received by RX, but it's hidden by GestureView. This PR passes the information further so that user can detect right clicks.

erictraut · 2018-04-17T15:30:32Z

src/common/Types.ts

    clientY: number;
    pageX: number;
    pageY: number;
+    isRightButton?: boolean; // UWP only, for desktop context menu


For consistency with MouseEvent, this should be called "button", it should return the integer corresponding to the button, and it should be implemented for all mouse-based platforms (not just UWP).

Also, since this is an addition to the public interface, it will need to be documented (in GestureView.md) and added to the ReactXP test app (GestureViewTest.tsx).

thezero · 2018-04-23T13:18:15Z

Changes:

Removed isRightButton property from original PR
Added onContextMenuGesture(gestureState: Types.TapGestureState) to GestureView with multiplatform support
Added test

erictraut · 2018-04-23T14:46:38Z

src/common/Types.ts

 export interface MouseEvent extends SyntheticEvent {
    altKey: boolean;
-    button: number;
+    button: MouseButton;


This is a breaking interface change. While I like the use of an enumeration here, it's not worth the pain associated with breaking existing code. Please revert back to "number".

erictraut · 2018-04-23T14:47:41Z

src/common/Types.ts

    pageX?: number;
    pageY?: number;
    touches: TouchList;
+    button?: number; // Mac, web native events


We shouldn't reflect this differently on Mac/web/UWP. We should use "button" on all three platforms. Please remove isRightButton and isMiddleButton.

That's not up to me - there are attributes coming directly from native code. While I agree it would be great to have them unified, it would require changes in react-native-windows.

It's the job of ReactXP to eliminate differences between the underlying RN implementations. Why can't you simply translate the isRightButton and isMiddleButton into common "button" values?

erictraut · 2018-04-23T14:50:51Z

src/common/Types.ts

 export type FocusEvent = SyntheticEvent;

+export enum MouseButton {
+    Primary,


Please remove this enumeration since its use in MouseEvent will result in a breaking change.

erictraut · 2018-04-23T14:52:15Z

src/native-common/GestureView.tsx

-        if (this.props.onTap) {
+        const button = EventHelpers.toMouseButton(e);
+        if (button === Types.MouseButton.Secondary) {
+            // always handle secondary button, even if context menu is not set - it shouldn't trigger onTap


Nit - for consistency, capitalize comment and end in period.

erictraut · 2018-04-23T14:52:19Z

src/native-common/GestureView.tsx

    }

    private _sendDoubleTapEvent(e: Types.TouchEvent) {
+        // if user did a double click with different mouse buttons, eg. left (50ms) right


Nit - for consistency, capitalize comment and end in period.

erictraut · 2018-04-23T14:59:15Z

src/web/GestureView.tsx

+            const clientRect = this._getGestureViewClientRect();
+
+            if (clientRect) {
+                const tapEvent: Types.TapGestureState = {


The TapGestureState doesn't derive from SyntheticEvent, so it doesn't offer the standard event methods preventDefault and stopPropagation. I think that's a problem here. I think we have two options:

Change the TapGestureState to inherit from SyntheticEvent and implement these two methods - and fill in the other fields that go along with SyntheticEvent

Always call e.preventDefault() and e.stopPropagation() for onContextMenu in a gesture view and document it as such.

Option #1 is more general and arguably more consistent, but it's a big change. For that reason, I am leaning to option #2. Let's discuss.

I think #2 is consistent with all the other events GestureView is handling - none of them bubble down. CMIIW, but you can't get onPress on a parent. So the only thing missing here is possibly e.stopPropagation() as e.preventDefault is a few lines below already.

Yes, that's a good argument. Please add the call to e.stopPropagation.

…atting

erictraut · 2018-04-23T21:45:31Z

src/common/Types.ts

    onPanHorizontal?: (gestureState: PanGestureState) => void;
    onTap?: (gestureState: TapGestureState) => void;
    onDoubleTap?: (gestureState: TapGestureState) => void;
+    onContextMenuGesture?: (gestureState: TapGestureState) => void;


This should be "onContextMenu" for consistency. We don't add the word "Gesture" in any other cases.

This can't be onContextMenu because of conflicting interface (ViewPropsShared). As discussed in the chat you created:

onContextMenu is defined as (e: MouseEvent) which 1) doesn't make sense for touch interface 2) would be hard to fake due to how GestureView works now and would require serious refactoring of the component.

All the other callbacks for GestureView are (gestureState: TapGestureState) so my proposal is to keep up with this and add onContextMenuGesture(gestureState: TapGestureState). Thoughts?

Yeah, I realize I used the name "onContextMenuGesture" in the chat, but now that I'm reviewing the change, I think "onContextMenu" is better for consistency. There's no conflict with ViewPropsShared because GestureProps has no inheritance relationship with ViewPropsShared.

The GestureView documentation also needs to be updated to include the new prop.

Yes, good point about documentation. I'll add it.

There's no conflict with ViewPropsShared because GestureProps has no inheritance relationship with ViewPropsShared.

src/native-common/GestureView.tsx(42,52): error TS2344: Type 'GestureViewProps' does not satisfy the constraint 'ViewProps'.
Types of property 'onContextMenu' are incompatible.
Type '((gestureState: TapGestureState) => void) | undefined' is not assignable to type '((e: MouseEvent) => void) | undefined'.

GestureView extends ViewBase<Types.GestureViewProps, {}> -> ViewBase<P extends Types.ViewProps, S> -> ViewProps extends ViewPropsShared. Therefore GestureViewProps need to be interface compatible with ViewPropsShared.

Yikes, that's a bug. GestureView shouldn't be extending ViewBase like that. Neither should ScrollView. I'm surprised that the TypeScript compiler didn't complain about that because ViewBase requires that "P extends Types.ViewProps", and GestureViewProps doesn't properly extend this interface. I'm working on a fix, and I'll let you know when I'm done.

OK, I just pushed a fix for the bug. Turns out it was a problem for some implementations of GestureView, ScrollView and WebView. I've fixed all of them.

erictraut · 2018-04-23T21:47:46Z

src/native-common/utils/EventHelpers.ts

+        const nativeEvent = e as any;
+        if (nativeEvent.button !== undefined) {
+            return nativeEvent.button;
+        } else if (nativeEvent.isRightButton) {


Are you sure "isRightButton" shouldn't be capitalized? It was in the old code. Same with IsMiddleButton. Was the old code incorrect?

I believe so (look right below at method isRightMouseButton, it suddenly checks camelCase isRightButton). Mac sends button property, RNUWP sends camelCase isRightButton. isMiddleButton is not implemented right now at all, I added it for future compatibility with RNUWP (unless they switch to button too).

erictraut · 2018-04-23T21:48:38Z

src/web/GestureView.tsx

+            const clientRect = this._getGestureViewClientRect();
+
+            if (clientRect) {
+                const tapEvent: Types.TapGestureState = {


Yes, that's a good argument. Please add the call to e.stopPropagation.

…o web

thezero · 2018-04-25T09:43:57Z

src/native-common/utils/EventHelpers.ts

+        const nativeEvent = e as any;
+        if (nativeEvent.button !== undefined) {
+            return nativeEvent.button;
+        } else if (nativeEvent.isRightButton || nativeEvent.IsRightButton) {


As mentioned in previous PR, I tested current implementations and it seems that UpperCase IsRightButton is not produced by any native code, but I'd rather keep backwards compatibility.

thezero · 2018-04-25T09:45:38Z

src/web/GestureView.tsx

+    private _sendContextMenuEvent = (e: React.MouseEvent<any>) => {
+        if (this.props.onContextMenu) {
+            e.preventDefault();
+            e.stopPropagation();


I moved these a bit earlier to keep this behavior on par with the rest on reactxp - events are being cancelled before their synthentic handlers. At the same time if onContextMenu is not defined, I don't think we should prevent the event from bubbling down.

* add isRightButton property to GestureView tap * generic handling of mouse tap on GestureView * add onContextMenu handling * add conversion for Mac * cleanup whitespace changes * PR fixes: change MouseEvent.button back to number, unify comment formatting * rename onContextMenuGesture to onContextMenu; add e.stopPropagation to web

add isRightButton property to GestureView tap

9a55aab

erictraut suggested changes Apr 17, 2018

View reviewed changes

Jakub Tomanek added 2 commits April 18, 2018 12:01

generic handling of mouse tap on GestureView

9c193b0

add onContextMenu handling

0b59cdb

thezero changed the title ~~[Windows] Add isRightButton property to GestureView tap~~ [Windows] Add onContextMenuCallback to GestureView Apr 23, 2018

Jakub Tomanek added 2 commits April 23, 2018 15:29

add conversion for Mac

b006cb4

cleanup whitespace changes

15c433c

erictraut suggested changes Apr 23, 2018

View reviewed changes

PR fixes: change MouseEvent.button back to number, unify comment form…

b1ea334

…atting

thezero force-pushed the windows-right-button-gestureview branch from 5add235 to b1ea334 Compare April 23, 2018 15:46

erictraut suggested changes Apr 23, 2018

View reviewed changes

Merge branch 'master' into windows-right-button-gestureview

ce6fb5f

thezero changed the title ~~[Windows] Add onContextMenuCallback to GestureView~~ [Windows] Add onContextMenu callback to GestureView Apr 25, 2018

rename onContextMenuGesture to onContextMenu; add e.stopPropagation t…

ba766e5

…o web

thezero commented Apr 25, 2018

View reviewed changes

erictraut merged commit c9a8af0 into microsoft:master Apr 25, 2018

thezero mentioned this pull request May 9, 2018

Add onContextMenu to RX.Link #621

Merged

[Windows] Add onContextMenu callback to GestureView #599

[Windows] Add onContextMenu callback to GestureView #599

Uh oh!

Conversation

thezero commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thezero commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thezero Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thezero commented Apr 23, 2018 •

edited

Loading

thezero Apr 24, 2018 •

edited

Loading