Use Hyperlink for pressable virtual text#8335
Conversation
We had previously created a custom component called HyperlinkTextViewManager that derived from VirtualTextViewManager to enable focus on pressable text, get the correct cursor behavior for pressable text, and to work around the issue with text hit testing that is being addressed in microsoft#7553). Rather than adding a new component that has no meaning on other platforms, we can use Hyperlink when virtual text meets certain conditions. This is really just a proposal for now, we should discuss if this change is warranted and, if so, under what conditions the Span should be swapped out for a Hyperlink. Please note this diff is dependent on microsoft#7553 and microsoft#7813 as we want to use the TouchEventHandler implementation to handle clicks in a way that they leverage the existing bubble / capture mechanisms of React Native. Also, the Hyperlink::Click event does not given you important details like the pointer position (see microsoft/microsoft-ui-xaml#4730). Remaining TODOs are: 1. Enable handled keyboard events on Hyperlink to flow so `usePressability` can handle keyboard invocation events. 2. Decide under what conditions we want to swap Span with Hyperlink. This PR uses `accessibilityRole` settings as an example, but we could also send an `isPressable` prop. 3. Add focus-related props to virtual text, so Hyperlink can set properties like TabIndex, TabStop, and UseSystemFocusVisuals. One remaining bit is how to enable "handled" keyboard events (as the `Enter` and `Space` keys are handled by Hyperlink component
rozele
force-pushed
the
virtualTextHyperlink
branch
from
7d77fe6 to
3396cc3
Compare
| if (isHyperlink && !wasHyperlink) { | ||
| winrt::Hyperlink hyperlink; | ||
| // Underline should be handled by base class using 'textDecorationLine' prop | ||
| hyperlink.UnderlineStyle(winrt::UnderlineStyle::None); |
There was a problem hiding this comment.
I'm more inclined to let components behave natively as they do in the OS, and let properties override those defaults, e.g. textDecorationLine: 'none'. Thoughts?
| // invoke action is triggered? Does View suffer the same limitation where | ||
| // a double tap for accessibility invoke will fire both the `onTouch*` | ||
| // events and the `onClick` / `onAccessibilityTap` events? | ||
| if (UiaClientsAreListening()) { |
There was a problem hiding this comment.
This seems to return true even when Narrator is turned off...
There was a problem hiding this comment.
So we're seeing the onPress event fire twice.
| }); | ||
|
|
||
| hyperlink.GotFocus([=](auto &&...) { | ||
| DispatchEvent("topFocus", folly::dynamic::object("target", m_tag)); |
There was a problem hiding this comment.
Can we generalize this a bit so it can share code with ViewViewManager?
| } | ||
|
|
||
| void VirtualTextShadowNode::dispatchCommand(const std::string &commandId, winrt::Microsoft::ReactNative::JSValueArray &&commandArgs) { | ||
| if (commandId == "focus") { |
There was a problem hiding this comment.
Can we generalize this a bit so it can share code with ViewViewManager?
|
This approach is getting pretty complex. I may have a new proposal to just add a |
@rozele What's your take on this now? Originally you had said this:
What's your recommendation at this point and would it be helpful to have a focused discussion on this? |
@chrisglein I think at this point, given the complexity and UIA limitations of Hyperlink (it’s always read as a “link” type) I’ll likely move forward with a product code / 3rd party view manager implementation via @asklar’s suggestion to use IViewManagerCreateWithProperties. Recent WIP changes to how background color is handled (#8408) have made it more trivial for that code to “play nice” with non-RN core TextElements, and the two other text tree traversal pieces: textTransform and the WIP approach to multi line text hit testing (#7553) either already work with non-core TextElements or would be equally trivial to modify to work with non-core spans. In parallel, I’ll socialize a component that satisfies the focusability and UIA requirements for Windows and macOS for true semantic links, and we can revisit for RNW if/when such a component lands upstream. |
|
I've started a new PR that changes the approach for all the tree recursion algorithms: #8454 This will ensure that if we want to add Hyperlink via |
3 participants
We had previously created a custom component called HyperlinkTextViewManager that derived from VirtualTextViewManager to:
Rather than adding a new component that has no meaning on other platforms, we can use Hyperlink when intermediate text nodes meet certain conditions.
This is really just a proposal for now, we should discuss if this change is warranted and, if so, under what conditions the Span should be swapped out for a Hyperlink.
Please note this diff is dependent on #7553 and #7813 as we want to use the TouchEventHandler implementation to handle clicks in a way that they leverage the existing bubble / capture mechanisms of React Native. Also, the Hyperlink::Click event does not given you important details like the pointer position (see microsoft/microsoft-ui-xaml#4730).
Remaining TODOs are:
usePressabilitycan handle keyboard invocation events.accessibilityRolesettings as an example, but we could also send anisPressableprop and use Hyperlink for all pressable inlines.Hyperlink.
onClickandonAccessibilityTapwork via theHyperlink::ClickeventUiaClientsAreListeningcheck, but I think we only want to fireonClickoronAccessibilityTapwhen narrator is attached, and only when the regularonTouch*events would not fire (e.g., when CapsLock+Enter is pressed or the view is double tapped). However, I think double tap suffers a similar limitation on View.❌ Ensure Narrator reports accessibilityRole correctly
Microsoft Reviewers: Open in CodeFlow