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

Pressable Text substrings do not work properly when they're selectable #1679

Closed
rigdern opened this issue Feb 28, 2018 · 6 comments
Closed

Comments

@rigdern
Copy link
Contributor

rigdern commented Feb 28, 2018

Problem

You can create a pressable Text substring by nesting a Text component with an onPress handler inside of another Text component. If the outer Text component marks the text as selectable, then the press functionality of the inner Text component is broken. Here's an example:

<Text selectable={true}>
  Outer text
  <Text onPress={this._handlePress} style={{color: 'red'}}>
    Inner text
  </Text>
  More outer text
</Text>

There are a couple of issues with this:

  1. When you click on "Inner text" _handlePress is not called.
  2. When you hover over "Inner text" the cursor is the I-beam selection cursor. Instead, the cursor should be a hand indicating that you are hovering over a clickable element.

Potential Solutions

I have a few ideas for how to solve this but none are ideal so far:

  1. Manually do hit testing on Text to see if a pressable region was hit. To do this, we'd have to sign up for the RichTextBlock's pointer events using AddHandler so we can pass true for the handledEventsToo parameter. A major limitation is apparently we are stuck with the I-beam selection cursor when hovering over text. When trying to set it to something else like an arrow in the PointerMoved handler, the cursor rapidly flickers between I-beam and arrow. To fix this, we'd likely need a feature from the XAML team. Another concern is around the performance of the code that would do the hit testing.
  2. Every nested Text component with an onPress handler is rendered as a Hyperlink. This solution has some issues due to limitations with Hyperlink:
  3. Every nested Text component with an onPress handler is rendered as an inline view. The issue with this approach is that when your selection includes the inline view, the copy command doesn't work.

The best option might be (2). We translate a <Text> with an onPress handler to a XAML Hyperlink. There will be a lot of limitations in styling but a hyperlink scenario is probably the most common use of onPress on Text nodes anyway. Also, I can't think of a more general way to solve this nicely given the APIs XAML exposes.

@rozele rozele added the .NET label Feb 26, 2019
@chrisglein chrisglein added the .NET Archive Issue opened against deprecated .NET implementation, will be closed (label drives bot activity) label Oct 25, 2019
@ghost
Copy link

ghost commented Oct 25, 2019

We are not investing in new features or lower priority bug fixes on vCurrent. The bulk of investment is now in vNext as we prepare to make that the default choice. If this issue is still relevant on the vNext implementation please open a vNext issue. If this issue is of significant severity for a vCurrent app and vNext is not an option, re-open with justification.

@ghost ghost removed the .NET Archive Issue opened against deprecated .NET implementation, will be closed (label drives bot activity) label Oct 25, 2019
@chrisglein chrisglein added the .NET Archive Issue opened against deprecated .NET implementation, will be closed (label drives bot activity) label Oct 25, 2019
@ghost ghost removed the .NET Archive Issue opened against deprecated .NET implementation, will be closed (label drives bot activity) label Oct 25, 2019
@ghost ghost closed this as completed Oct 25, 2019
@rozele
Copy link
Collaborator

rozele commented Mar 31, 2021

Reviving an oldie-but-goodie. This remains to be true in vnext.

@rozele rozele reopened this Mar 31, 2021
@rozele
Copy link
Collaborator

rozele commented Mar 31, 2021

For now, we're using option 2 from the description above, but it's limiting that the ABIViewManager derives from FrameworkElementViewManager (so we need to keep the HyperlinkViewManager implementation in a fork of react-native-windows). Honestly option 2 wouldn't need to be a core feature if we fixed #5539. Ideally we would be able to create ViewManagers that returned DependencyObjects as opposed to only FrameworkElements.

I experimented with option 1 (using AddHandler with handledEventsToo=true) in TouchEventHandler.cpp rather than the routed event handlers, but that doesn't seem to work entirely because of pointer capture inside TextBlock (so PointerCaptureLost is invoked before PointerReleased, canceling the pointer).

@ghost ghost added the Invalid Triage https://github.com/microsoft/react-native-windows/wiki/Triage-Process (label applied by bot) label Apr 1, 2021
@rozele rozele added Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) and removed Invalid Triage https://github.com/microsoft/react-native-windows/wiki/Triage-Process (label applied by bot) labels Apr 2, 2021
@chrisglein chrisglein added Area: Text and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Apr 5, 2021
@chrisglein chrisglein added this to the Backlog milestone Apr 5, 2021
@chrisglein chrisglein added the bug label Apr 5, 2021
@chrisglein
Copy link
Member

@rectified95 You've got a change to restore the pressable events on nested text. See if selectable is still affected.

@rozele
Copy link
Collaborator

rozele commented Apr 7, 2021

@chrisglein @rectified95 - selectable is still affected, it's an issue with TextBlock capturing the pointer and handling events before they can reach TouchEventHandler.

A while back, we used the UIElement.AddHandler approach with handledEventsToo set to true: #1646

However, this has it's own limitations, as PointerCaptureLost fires whenever the TextBlock handles an event, which effectively cancels the React Native touch sequence. It might also interfere with other workarounds that were added for pointer capture behavior on ScrollViewer like this one: facebook/react-native#30374

I have a WIP approach that takes a more targeted approach, subscribing TouchEventHandler directly on TextBlocks when selectable is set:
https://github.com/rozele/react-native-windows/tree/handledEventsToo

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
@rozele
Copy link
Collaborator

rozele commented Nov 4, 2021

Fixed by #7813

@rozele rozele closed this as completed Nov 4, 2021
@ghost ghost added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Nov 4, 2021
@asklar asklar removed the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Nov 4, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants