Skip to content
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.

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

NSTabView doesn't send mouseUp event because it doesn't inherit from NSControl #2111

Closed
eablokker opened this issue Apr 19, 2024 · 7 comments
Labels
bug Something isn't working Needs: Triage 🔍

Comments

@eablokker
Copy link

eablokker commented Apr 19, 2024

Environment

react-native -v: command not found (Metro says v0.73.2)
npm ls react-native-macos: react-native-macos@0.73.15
node -v: v18.19.0
npm -v: 10.3.0
yarn --version: 3.6.1
xcodebuild -version: Xcode 13.2.1 Build version 13C100

Steps to reproduce the bug

  1. Create a custom native tabView component
// MyTabViewManager.m

#import <AppKit/AppKit.h>
#import <React/RCTViewManager.h>

@interface MyTabView : NSTabView
@end

@interface MyTabViewManager : RCTViewManager<NSTabViewDelegate>
@end

@implementation MyTabViewManager

RCT_EXPORT_MODULE(MyTabView)

- (MyTabView *)view {
  MyTabView *tabView = [[MyTabView alloc] init];
  tabView.delegate = self;

  // Insert some tabs
  NSTabViewItem *tabViewItem = [[NSTabViewItem alloc] init];
  tabViewItem.label = @"Tab One";

  // Add empty view
  tabViewItem.view = [NSView new];

  [tabView addTabViewItem:tabViewItem];

  // Insert another tab
  NSTabViewItem *tabViewItem2 = [[NSTabViewItem alloc] init];
  tabViewItem2.label = @"Tab Two";

  // Add empty view
  tabViewItem2.view = [NSView new];

  [tabView addTabViewItem:tabViewItem2];

  return tabView;
}
  1. Create a TabView React component
// MyTabView.tsx

import React from 'react';
import {requireNativeComponent} from 'react-native';

const TabView = () => {
  return <MyTabView style={{ height: 300 }} />;
};

const MyTabView = requireNativeComponent('MyTabView');

module.exports = TabView;
  1. Click on tab label buttons.

Expected Behavior

The tab buttons should activate on click.

Actual Behavior

The first click activates the button but the second click logs the error "Touch is already recorded. This is a critical bug." On the first click, the mouseDown event fires but mouseUp does not. On the second click the mouseDown does not fire but mouseUp does.

Reproducible Demo

No response

Additional context

The bug is located in RCTTouchHandler

// 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];
} else if ([targetView isKindOfClass:[NSText class]]) {
  _shouldSendMouseUpOnSystemBehalf = [(NSText*)targetView isSelectable];
}
else if ([targetView.superview isKindOfClass:[RCTUITextField class]]) {
  _shouldSendMouseUpOnSystemBehalf = [(RCTUITextField*)targetView.superview isSelectable];
} else {
  _shouldSendMouseUpOnSystemBehalf = NO;
}

NSTabView does not inherit from NSControl. According to the view hierarchy inspector this is the class inheritance hierarchy:

  • NSTabView
  • NSView
  • NSResponder
  • NSObject

This code change seems to work

} else if ([targetView isKindOfClass:[NSTabView class]]) {
  _shouldSendMouseUpOnSystemBehalf = YES;
}

However this by itself seems to still have click locations in the wrong place. If I combine this with a hitTest on the custom NSTabView then it works:

- (NSView *)hitTest:(NSPoint)aPoint {
  if (!self.isHidden && [self mouse:aPoint inRect:self.bounds]) {
    // Convert point to each subview and check
    for (NSView *subview in [self.subviews reverseObjectEnumerator]) {
      NSPoint convertedPoint = [self convertPoint:aPoint toView:subview];
      NSView *hitTestView = [subview hitTest:convertedPoint];
      if (hitTestView) {
        return hitTestView;
      }
    }
    // If no subview should handle this point, return self or nil based on whether this view should handle clicks
    return self;
  }
  return nil;
}
@eablokker eablokker added the bug Something isn't working label Apr 19, 2024
@eablokker eablokker changed the title NSTabView's NSTabViewButtons don't send mouseUp event because they don't inherit from NSControl NSTabView doesn't send mouseUp event because it doesn't inherit from NSControl Apr 19, 2024
@Saadnajmi
Copy link
Collaborator

For the extra case for NSTabView, I think that's a fine PR to make. For the clicks being in the wrong spot, I actually ran into this and meant to write some docs about it: #1904

I think the hit testing can be fixed React Native macOS side so that all custom native components don't have to override hit testing.. but it's been a while.

@eablokker
Copy link
Author

@Saadnajmi Thank you, your hitTest plus isFlipped is working better than what I had.

I will make a PR for the NSTabView mouseUp soon.

I wonder if there are any other elements inheriting from NSView that need to handle mouseDown/Up, but for now all I see is NSTabView. Everything else seems to inherit from NSControl.

@Saadnajmi
Copy link
Collaborator

If you are open to it, you can also make one targeting the branch 0.73-stable so that we can release a patch release with the change

@Saadnajmi
Copy link
Collaborator

Related, I'm curious how well your NSTabView native module works. Any chance you can send a screenshot of it in an app? :)

@eablokker
Copy link
Author

I'm having another issue with it, if I add any margin or padding styles on the tabview the click positions will be off by that many pixels. Even when the style is placed on a <View> surrounding the tabview. I found this post that says you have to override insertReactSubview:atIndex:, removeReactSubview, and reactSubviews on the view in order to get the subviews to be managed by the Yoga layout system. But I haven't gotten it to work.

NSTabView is very tricky to get working because it wants to manage its own subviews based on which tab is active. But React Native also manages the subviews, so you end up with too many subviews. What I did is give each tab the exact same React Native view and let React conditionally render a single view based on the item returned from the didSelectTabViewItem event.

I'm actually making a set of AppKit components for React Native Macos. I was forced to make a TabView because the preferences window requires an NSTabViewController in order to render the toolbar buttons in the preferences style. So I went ahead and made a standalone TabView as well.

Here's a TabView inside a Preferences TabView.
Screen Shot 2024-04-22 at 12 55 22 PM

@Saadnajmi
Copy link
Collaborator

Thanks for the info. Yes, I think something like NSTabView is hard to add directly in React Native because of the conditional rendering of children. Maybe it'll get better with Fabric, but I don't know? I'm glad there are iOS guides/blogs you can work off of. You might need a custom shadow view (which I have found very little docs describing how to do so). And if your Appkit repo is open source, I'd give it a follow :)

@eablokker
Copy link
Author

Thanks, I'm not ready to make the repo public yet. I'm not sure yet how I will release it. I'm thinking to make a basic version of it free, and maybe a paid version for advanced components.

Saadnajmi added a commit that referenced this issue Apr 23, 2024
<!-- Thanks for submitting a pull request! We appreciate you spending
the time to work on these changes. Please provide enough information so
that others can review your pull request. The four fields below are
mandatory. -->

<!-- This fork of react-native provides React Native for macOS for the
community. It also contains some changes that are required for usage
internal to Microsoft. We are working on reducing the diff between
Facebook's public version of react-native and our
microsoft/react-native-macos fork. Long term, we want this fork to only
contain macOS concerns and have the other iOS and Android concerns
contributed upstream.

If you are making a new change then one of the following should be done:
- Consider if it is possible to achieve the desired behavior without
making a change to microsoft/react-native-macos. Often a change can be
made in a layer above in facebook/react-native instead.
- Create a corresponding PR against
[facebook/react-native](https://github.com/facebook/react-native)
**Note:** Ideally you would wait for Facebook feedback before submitting
to Microsoft, since we want to ensure that this fork doesn't deviate
from upstream.
-->

## Summary:

<!-- Explain the **motivation** for making this change. What existing
problem does the pull request solve? -->

Fix for issue #2111 (NSTabView doesn't send mouseUp event because it
doesn't inherit from NSControl).

NSTabView inherits from NSView, not NSControl, so its click events
aren't recorded in the _nativeTouches cache leading to mouseUp and
mouseDown not firing consistently when clicking on the tab buttons.

## Test Plan:

<!-- Demonstrate the code is solid. Example: The exact commands you ran
and their output, screenshots / videos if the pull request changes the
user interface. -->

Code for custom TabView component provided in the linked issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Needs: Triage 🔍
Projects
None yet
Development

No branches or pull requests

2 participants