Skip to content

Conversation

HeyImChris
Copy link

@HeyImChris HeyImChris commented Mar 25, 2020

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native 👍
  • [ X] I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

When controls such as NSButton don't send the mouse up signal after a mouse down click, the mouse event gets stored in our cache and becomes stale. On the next click, we fail an assert that there are no stale events in our cache and no action is executed.

What we need to do is send the mouse up signal for all controls that can receive one so we clear out our cache properly. We need to pair the mouse down events 1:1 with the mouse up events we send.

Focus areas to test

Tested in a downstream app (Polyester). Clicking every time registers and sends the correct event (dropping a menu in this case). Tested with both soft trackpad and hard trackpad clicks.

Microsoft Reviewers: Open in CodeFlow

NSView *targetView = [self.view hitTest:touchLocation];
if ([targetView isKindOfClass:NSText.class]) {
if ([targetView isKindOfClass:[NSControl class]]) {
_shouldSendMouseUpOnSystemBehalf = [(NSControl*)targetView isEnabled];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we will continuously be adding new cases and exceptions here, though this seems reasonable as a temporary unblock. How far down the path of removing _shouldSendMouseUpOnSystemBehalf and the spoof event it creates did you go and do we have a sense of what complexity makes it currently difficult to get rid of that? Would be great to add in comments so next person looking at the code can benefit from your investigation.

Copy link
Author

@HeyImChris HeyImChris Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried removing it entirely, but I also don't think there are too many other cases in the future we'd hit. Almost every control we need to send mouse events for is an NSControl or NSText object.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now probably, but you could easily imagine different types of NSViews in the future needing this. And just isEnabled/isSelected is super unlikely to cover all scenarios

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the comment, I more meant commenting on what blocks us from removing this hack altogether

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it's possible that different controls in the future require more edits here. If touch handler bugs end up taking a lot of time then I think it's worth investigating rewriting this, but that's not the case now and I'm not convinced it will be any time soon.

@tom-un tom-un linked an issue Mar 25, 2020 that may be closed by this pull request
@HeyImChris HeyImChris merged commit 2117256 into master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RCTTouchHandler assert

3 participants