Skip to content

Send mouse up notifications for all enabled NSControls #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 25, 2020
Merged

Send mouse up notifications for all enabled NSControls #263

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4676b1
remove a stale event from the native tracking queue that prevents eve…
HeyImChris Mar 25, 2020
ab70f20
comment change inline
HeyImChris Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion React/Base/RCTTouchHandler.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,10 @@ - (void)_recordNewTouches:(NSSet *)touches
#else // [TODO(macOS ISS#2323203)
CGPoint touchLocation = [self.view convertPoint:touch.locationInWindow fromView:nil];
NSView *targetView = [self.view hitTest:touchLocation];
if ([targetView isKindOfClass:NSText.class]) {
// Pair the mouse down events with mouse up events so our _nativeTouches cache doesn't get stale
if ([targetView isKindOfClass:[NSControl class]]) {
_shouldSendMouseUpOnSystemBehalf = [(NSControl*)targetView isEnabled];
Copy link

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 •
edited
Loading

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.

Copy link

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

Copy link

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.

} else if ([targetView isKindOfClass:[NSText class]]) {
_shouldSendMouseUpOnSystemBehalf = [(NSText*)targetView isSelectable];
} else {
_shouldSendMouseUpOnSystemBehalf = NO;
Expand Down