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

Support custom native components that are not FrameworkElement #5539

Closed
aschultz opened this issue Jul 16, 2020 · 9 comments
Closed

Support custom native components that are not FrameworkElement #5539

aschultz opened this issue Jul 16, 2020 · 9 comments

Comments

@aschultz
Copy link
Member

aschultz commented Jul 16, 2020

Summary

Microsoft.ReactNative exposes the IViewManager interface to enable export of custom native UI components into the React environment. However, the interface has the notable restriction that the views it creates and manages must be based on FrameworkElement. This prevents its use with a number of unique XAML elements, such as those derived from TextElement or FlyoutBase.

The IViewManager interfaces should instead use DependencyObject as the common "view" type.

Example

I wish to create a block of text with a stylized and clickable inline link embedded in it. My React might look like:

render() {
    return <Text>This sentence includes a <Link url={...}>link</Link></Text>
}

Here the Text element is built-in to RNW as a XAML TextBlock control. The Link element would ideally be a XAML Hyperlink element that gets added as a child. Since Link is not built-in to RNW, we might create a custom view manager to provide this functionality. However, Hyperlink is not derived from FrameworkElement, so we cannot currently surface it through an IViewManager implementation.

Details

Some of the changes I expect will be needed:

  • IViewManager, IReactContext, and XamlUIService should switch to using DependencyObject instead of FrameworkElement/UIElement
  • ABIViewManager stops deriving from FrameworkViewManager and instead applies base logic depending on the views being managed.
    • View managers for FrameworkElement-derived views should get access to the same rich defaults as before (handling of layout, accessibility, and input event props).
    • View managers for other views should probably be marked as not affecting layout. FlyoutBase may need a special case here that allows it to register and host a root Yoga node.
  • Address RNW shouldn't use Tag property internally. #4187, as Tag is specific to FrameworkElement
@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Jul 16, 2020
@chrisglein
Copy link
Member

Should this go all the way to IInspectable? What has us stop specifically at DependencyObject? If we're no longer looking at what's in the XAML tree, it doesn't need to be a XAML type (outside of some root that links up). Is there a scenario for that? Not saying this is a requirement, but worth exploring the needs here. Not needed for the TextElement scenario, since that's a DO.

Some design work needed here to figure out what's the minimal set of operations needed. But seems like we can unblock the Text scenario.

@chrisglein chrisglein added Partner: Xbox (label may be applied by bot based on author) and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Jul 20, 2020
@chrisglein chrisglein added this to the 0.64 milestone Jul 20, 2020
@chrisglein
Copy link
Member

@aschultz Do you have a timeline for when you'd need this?

@aschultz
Copy link
Member Author

aschultz commented Jul 20, 2020

@chrisglein One advantage of stopping at DependencyObject is that it provides an easy means to stash/retrieve the React tag value as an attached property (instead of using FrameworkElement.Tag). If you go up to IInspectable you'd have to find another solution.

As for timeline -- I'm in the process of upgrading our app to 0.62 and just ran into this as a blocker for upgrading a Hyperlink component that's used in chat messages. It appears to be used in only that one place, so we might be able to craft a workaround in the short term. Otherwise, I was hoping to have us upgraded by the end of this month.

@chrisglein chrisglein modified the milestones: 0.64, 0.65 Nov 12, 2020
@ladebo
Copy link

ladebo commented Jan 15, 2021

@chrisglein Hello! From @aschultz 's last comment -- we're now using inline links in more locations (like FAQs and descriptions that are controlled content). Having this functionality to create a link, surrounded by plain text, moving forward would really help us. We've been running into issues with our workaround control and updating it to new scenarios.

Wanted to note in case this bug could be bumped up in priority.

@asklar
Copy link
Member

asklar commented Mar 15, 2021

@ladebo @aschultz I wanted to reach out and let you know about a module that has some intersection with the work described here: https://github.com/asklar/react-native-xaml/

We added a IViewManagerCreateWithProperties interface in 0.64 which is IInspectable-based, but the framework still expects objects returned by the view manager to be framework elements. react-native-xaml works around this by wrapping non-FrameworkElements into a ContentControl, then unwrapping them when they're added to the tree and doing the right thing to attach them to a parent (e.g. setting the flyout property if the child is a flyout, etc.).

The main purpose of the react-native-xaml module is to allow using XAML controls directly from a React Native Windows app. Let me know if this is useful to you guys, maybe the wrapping/unwrapping technique works for you too : )

@chrisglein chrisglein modified the milestones: 0.65, 0.66 Apr 24, 2021
@asklar asklar changed the title Support custom native components that derive from DependencyObject Support custom native components that are not FrameworkElement Jul 31, 2021
@asklar
Copy link
Member

asklar commented Jul 31, 2021

Renaming issue since we'll want to avoid repeating the same mistake of tying the API surface to a xaml dialectal type

@rozele
Copy link
Collaborator

rozele commented Sep 16, 2021

We discussed offline recently that it likely makes sense to continue tying interfaces for Paper to a XAML dialect type until Fabric (cc @acoates-ms). Here is a proposal for what interfaces we would need:

interface IDependencyObjectViewManager {
  String Name { get; };
  DependencyObject CreateView(int tag, int rootTag, IJSValueReader propertyReaderMap);
}

interface IDependencyObjectViewManagerWithUnmount {
  void DropView(DependencyObject view);
}

interface IDependencyObjectViewManagerWithChildren {
  IMapView<String, ViewManagerPropertyType> NativeProps { get; };
  void UpdateProperties(DependencyObject view, IJSValueReader propertyMapReader);
}

interface IDependencyObjectViewManagerWithCommands {
  IVectorView<String> Commands { get; };
  void DispatchCommand(DependencyObject view, String commandId, IJSValueReader commandArgsReader);
}

interface IDependencyObjectViewManagerWithChildren {
  void AddView((DependencyObject parent, DependencyObject child, Int64 index);
  void RemoveAllChildren(DependencyObject parent);
  void RemoveChildAt(DependencyObject  parent, Int64 index);
  void ReplaceChild(DependencyObject parent, DependencyObject oldChild, DependencyObject newChild);
}

@asklar
Copy link
Member

asklar commented Sep 18, 2021

I have a strong preference for not tying any new API to a xaml dialect/version.

@chrisglein chrisglein modified the milestones: 0.66, 0.67 Oct 4, 2021
rozele added a commit to rozele/react-native-windows that referenced this issue Oct 6, 2021
Summary:
When `selectable={true}` on the Text component in react-native-windows, the TextBlock native component captures and handles all pointer events, thus none of the pointer events are sent to JS. We could just allow the TouchEventHandler to respond to all events, even handled ones, but I'm concerned that this may raise other issues with other components that are expected to handle events (e.g., getting onPress events before ScrollViewer gets a chance to capture the pointer). There are other limitations to that approach as well, including issues with the mouse cursor and how the control would interoperate with accessibility.

Adding Hyperlink works around this limitation as we can emit direct events that can be tied to a JS `onPress` callback via the `Click` event, which "plays nicely" with `selectable={true}`. Also, Hyperlink has expected behavior out of the box for focusability and pointer cursor.

The reason this needs to be implemented in our fork and not in Zeratul is because the `IViewManager` interface exposed from the ABI for react-native-windows uses `FrameworkElement` as a base class, so we could not create a ViewManager for `Hyperlink`, which derives from `DependencyObject` and not `FrameworkElement`.

Here are the GitHub issues tracking that this is working around:
microsoft#1679
microsoft#5539



Test Plan:
- Can we invoke onPress for FocusableText when selectable=true?

https://pxl.cl/1G90F

- Does the mouse cursor change from the selection IBeam to signal that the FocusableText can be clicked?

{F564338487}

- Can I still set selection on link text?

{F564343437}

- Can we tab focus to FocusableText?
Not currently, but I believe this is not a limitation of the Hyperlink approach, something else seems to be trapping focus in the chat window.

- Can we use the Enter key to invoke onPress for FocusableText?
Not currently because the hyperlink cannot get focus.

- Stretch: Is there any feedback when onPressIn for FocusableText?
No, and it's likely that if someone adds it using `onPressIn` or `onPressOut` it will be broken.

Reviewers: skyle, lyahdav, mylando

Reviewed By: lyahdav

Subscribers: necolas, eliwhite

Differential Revision: https://phabricator.intern.facebook.com/D27474588

Tasks: T87521248

Signature: 27474588:1617305556:de80de6759c483628f86d3453533eacf360ce234
@asklar asklar removed this from the 0.67 milestone Oct 22, 2021
@asklar asklar added this to the 0.68 milestone Oct 22, 2021
@chrisglein chrisglein added this to Pending Evaluation in Planning Poker via automation Jan 21, 2022
@chrisglein chrisglein modified the milestones: 0.68, Backlog Feb 24, 2022
rozele added a commit to rozele/react-native-windows that referenced this issue Oct 23, 2022
Summary:
When `selectable={true}` on the Text component in react-native-windows, the TextBlock native component captures and handles all pointer events, thus none of the pointer events are sent to JS. We could just allow the TouchEventHandler to respond to all events, even handled ones, but I'm concerned that this may raise other issues with other components that are expected to handle events (e.g., getting onPress events before ScrollViewer gets a chance to capture the pointer). There are other limitations to that approach as well, including issues with the mouse cursor and how the control would interoperate with accessibility.

Adding Hyperlink works around this limitation as we can emit direct events that can be tied to a JS `onPress` callback via the `Click` event, which "plays nicely" with `selectable={true}`. Also, Hyperlink has expected behavior out of the box for focusability and pointer cursor.

The reason this needs to be implemented in our fork and not in Zeratul is because the `IViewManager` interface exposed from the ABI for react-native-windows uses `FrameworkElement` as a base class, so we could not create a ViewManager for `Hyperlink`, which derives from `DependencyObject` and not `FrameworkElement`.

Here are the GitHub issues tracking that this is working around:
microsoft#1679
microsoft#5539

Test Plan:
- Can we invoke onPress for FocusableText when selectable=true?

https://pxl.cl/1G90F

- Does the mouse cursor change from the selection IBeam to signal that the FocusableText can be clicked?

{F564338487}

- Can I still set selection on link text?

{F564343437}

- Can we tab focus to FocusableText?
Not currently, but I believe this is not a limitation of the Hyperlink approach, something else seems to be trapping focus in the chat window.

- Can we use the Enter key to invoke onPress for FocusableText?
Not currently because the hyperlink cannot get focus.

- Stretch: Is there any feedback when onPressIn for FocusableText?
No, and it's likely that if someone adds it using `onPressIn` or `onPressOut` it will be broken.

Reviewers: skyle, lyahdav, mylando

Reviewed By: lyahdav

Subscribers: necolas, eliwhite

Differential Revision: https://phabricator.intern.facebook.com/D27474588

Tasks: T87521248

Signature: 27474588:1617305556:de80de6759c483628f86d3453533eacf360ce234
@chrisglein
Copy link
Member

Investment focus is on Fabric where this consideration is no longer relevant. Won't fix for Paper.

@chrisglein chrisglein closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Planning Poker
Pending Evaluation
Development

No branches or pull requests

6 participants