Fix onPress handlers on nested Text components

[rigdern]

Notes

I would have preferred to make GetReactTagAtPoint a polymorphic method so that the code that authors the Text component could also provide an implementation for GetReactTagAtPoint. However, because we don't control the source code for RichTextBlock and it's sealed, I couldn't find a way to do this.

Perhaps you know of a way?

One other option would be to make GetReactTagAtPoint a method of the view manager and TouchHandler would have to somehow find the view manager for the view and invoke the method.

Commit Message

Currently, onPress handlers only work on outermost Text components.
This change enables you to place onPress handlers on nested Text
components. For example, this now works:

  <Text>
    <Text onPress={this._handlePress}>Press Me</Text>
  </Text>

In the fix, we need to manually do hit testing on outermost Text
components to see if any of the nested Text components got hit. This is
because the nested Text components are represented by Inlines rather than
by UIElements.

[rozele]

@rigdern I would definitely like to avoid seeing a specific reference to RichTextBlock (or any specific view type) inside TouchHandler as well, and yeah, I can see where you would have wanted to follow the pattern in ReactAndroid with the ReactCompoundView (https://github.com/facebook/react-native/blob/c91591b98792ffbd0826a4bda7304d63a52f0479/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactCompoundView.java#L19), but cannot do this due to sealed framework elements.

I can think of two things to avoid having view-specific code in the TouchHandler:

1. We could wrap the RichTextBlock in a UserControl or Canvas that is not sealed.
2. Add another method to DependencyObjectExtensions to stuff another object in the ConditionalWeakTable associated with each tagged view instance.

Honestly, I was hoping to get rid of the DependencyObjectExtensions all together at some point, but I don't see many other options because so many of the XAML elements are sealed. Maybe there are other options, but can't think of any right now.

Update: I forgot that the pointerEvents logic was already put in the ConditionalWeakTable in DependencyObjectExtensions, so touch-related behavior is already being done in this way...

[rigdern]

Does (1) concern you from a performance perspective? Apps can have lots of text views so we'd end up adding a bunch of extra UserControls or Canvases.

Can you elaborate on (2)? Are you imagining something like the following? Add a method called GetReactTagAtPoint to the DependencyObjectExtensions that calls GetTag() by default. Also add a method called SetGetReactTagAtPointImpl to DependencyObjectExtensions which takes a function which will be used instead when GetReactTagAtPoint is called. That way, the TextViewManager can provide a custom implementation of GetReactTagAtPoint for each of its views.

Did you have any thoughts on the idea of putting the GetReactTagAtPoint implementation on the view managers and then having the TouchHandler get ahold of the view manager somehow?

[rozele]

Yes, there is definitely a perf concern for 1). I would take an object-oriented approach for 2), creating an interface like ReactCompoundView, and associating React view instances with instances of the interface.

I think it would not be a good idea for now to give other components full access to view managers, feels like too much breaking of encapsulation.

[rigdern]

I pushed an implementation of (2).

I thought a little bit about a replacement for the DependencyObjectExtensions file. FrameworkElement has a property called Tag where you can stash an arbitrary object. When constructing a view, each view manager could set that to an object which implements whatever interfaces the view is interested in supporting (e.g. IReactCompoundView).

If you need this technique to work on non-FrameworkElements (e.g. Run and Span), you could have just 2 methods in DependencyObjectExtensions: one to get the object associated with the view/run/span/etc. and one to set it.

[rozele]

Agreed, would save us a couple allocations and taking the lock in CWT. In fact, it used to be that way before we introduced DependencyObect as the base class. I removed it because I wasn't sure if a case could be made where a user implementation of a view manager would want to use the Tag property, but its okay with me if you want to add it back.

---

From: Adam Comellamailto:notifications@github.com
Sent: ‎8/‎5/‎2016 6:02 PM
To: ReactWindows/react-native-windowsmailto:react-native-windows@noreply.github.com
Cc: Eric Rozellmailto:erozell@outlook.com; Commentmailto:comment@noreply.github.com
Subject: Re: [ReactWindows/react-native-windows] Fix onPress handlers on nested Text components (#560)

I pushed an implementation of (2).

I thought a little bit about a replacement for the DependencyObjectExtensions file. FrameworkElement has a property called Tag where you can stash an arbitrary object. When constructing a view, each view manager could set that to an object which implements whatever interfaces the view is interested in supporting (e.g. IReactCompoundView).

If you need this technique to work on non-FrameworkElements (e.g. Run and Span), you could have just 2 methods in DependencyObjectExtensions: one to get the object associated with the view/run/span/etc. and one to set it.

---

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#560 (comment)

[rigdern]

I'm happy enough with the approach I submitted in this PR and I don't want to do the refactoring now. We can keep it in mind for later.

[rozele]

We're probably going to want to make this public.

[rozele]