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

feat(ControlWindows): Adds control component for tab stops #1411

Closed
wants to merge 1 commit into from

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Oct 5, 2017

ControlWindows is a UserControl with a single child Canvas. It has props to enable tab stop, tab index, keyboard navigation mode, and keyUp/Down events.

cc @rigdern

Fixes #886

@rozele
Copy link
Collaborator Author

rozele commented Oct 5, 2017

@rigdern the benefit of this approach is that it won't require overriding View.js for Windows and we can keep the optimization that there is no wrapper JS component around the native component for View. The downside is that in order to make Touchable components tab stops, we'll need to override TouchableWithoutFeedback or create an alternative set of Touchable components that use this ControlWindows component. Would love to get your feedback.

ControlWindows is a UserControl with a single child Canvas. It has props to enable tab stop, tab index, keyboard navigation mode, and keyUp/Down events.

Fixes microsoft#886
@codecov-io
Copy link

Codecov Report

Merging #1411 into master will decrease coverage by 0.42%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1411      +/-   ##
==========================================
- Coverage   31.78%   31.36%   -0.43%     
==========================================
  Files         259      262       +3     
  Lines       19249    19511     +262     
  Branches     1623     1633      +10     
==========================================
  Hits         6119     6119              
- Misses      12976    13238     +262     
  Partials      154      154
Impacted Files Coverage Δ
...ndows/ReactNative.Shared/Views/View/ReactCanvas.cs 0% <ø> (ø)
...Native.Net46/Views/TextInput/PlaceholderAdorner.cs 0% <ø> (ø) ⬆️
...indows/ReactNative.Net46/Shell/MainReactPackage.cs 0% <0%> (ø) ⬆️
...ows/ReactNative.Shared/Views/Control/KeyHelpers.cs 0% <0%> (ø)
.../ReactNative.Shared/Views/View/ReactViewManager.cs 0% <0%> (ø) ⬆️
...s/ReactNative.Shared/Views/Control/ReactControl.cs 0% <0%> (ø)
...Native.Shared/Views/Control/ReactControlManager.cs 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e1417...20ae297. Read the comment docs.

private void OnKeyUp(object sender, KeyEventArgs e)
#endif
{
if (sender == e.OriginalSource)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good heuristic. Even though this is a bubbling event in JavaScript, there is no concept of Handled / Unhandled, so parent components will not get onKey[Up|Down] events that a JavaScript component does not want to handle. One example of where this will probably be needed is in implementing tab and arrow key accessibility for ListView. The individual list items will likely be focusable and may have responders for Space and Enter, but they will want the arrow key events to bubble to the ListView parent so they can change the focus programmatically (unless arrow keys work by default for tab stop controls in the same parent).

Copy link
Contributor

@rigdern rigdern Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if we can learn something about how to design this from touch events. I believe React Native only listens for touch events on the RootView. Do you understand the rationale behind this design?

Also, it seems like touch events would have a similar problem regarding the Handled property. How do touch events deal with Handled and would that solution work for keyboard events as well?

Focus and blur seem similar to touch, keyboard, and mouse events. Many views can generate these kinds of events and views that generate these events can be nested inside of other views that generate these events. What are other examples of events like this and which ones listen at the RootView only vs have a listener per view?


In reply to: 143015754 [](ancestors = 143015754)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rigdern I believe touch events are sufficiently different from the focus stack and keyboard events. When I first started thinking about how to implement this, I thought we might want to do something similar to the touch responder architecture in React.js and simulate the focus stack entirely in JS. This would be a lot of work, and ultimately will always be riddled with bugs where behaviors are inconsistent with what's expected in native platform.

Currently, we don't deal with the Handled property on touch events because the events need to bubble to the root view. The only components that handle touch events are built-in gesture responders like ScrollViewer, FlipView, etc., and that's all handled via direct manipulation (not the UI thread touch events).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe touch events are sufficiently different from the focus stack and keyboard events.

Can you elaborate on how they're sufficiently different?

When I first started thinking about how to implement this, I thought we might want to do something similar to the touch responder architecture in React.js and simulate the focus stack entirely in JS. This would be a lot of work

Can you elaborate on why it's a lot of work? For focus, wouldn't we just tell React Native to dispatch the event into the JavaScript engine to the view with the appropriate tag? And then React Native would take care of the bubbling and capture phases.

Currently, we don't deal with the Handled property on touch events because the events need to bubble to the root view.

Does that mean if we listen to events on the RootView, we don't have to worry about the Handled property? An exception I could see is if we wanted to mark Handled to true immediately on the control that triggered the event to prevent one of its native ancestors from handling the event in an undesirable way.

@rigdern
Copy link
Contributor

rigdern commented Oct 5, 2017

TouchableWithoutFeedback doesn't have its own element. It clones whatever child is passed into it. So we'd have to update TouchableWithoutFeedback to introduce an extra wrapper component.

In ReactXP, RX.View would have to switch between View and ControlWindows based on its props. We'd have to do something similar for RX.Link and RX.Button.

Is ControlWindows's functionality a superset of View's? If so, maybe we should call it FocusableView. And maybe we should introduce it into the react-native repo and the iOS/Android implementations would be identical to View.

Let's briefly summarize our options:

  1. Edit View.js to use ControlWindows instead of RCTView when the View needs to be focusable. The downside is this would be introducing overhead into View which is a widely used primitive.
  2. react-native-windows can upgrade a View from a Canvas to a UserControl in C# when it needs to be focusable. When I've thought about doing things like this in the past, I've found it to be tricky. You'd have a better sense of how hard this solution would be. Aside from the trickiness, I can't think of any other downsides to this approach.
  3. Force people to use ControlWindows instead of View for focusable things. The main downside is we'd need an extra wrapping component in things like TouchableWithoutFeedback. When introducing an extra wrapping component that the consumer of the component doesn't have access to, it can be tricky to ensure the wrapper and the inner component get the correct size. In other words, what kind of layout properties do you set on the wrapper component to ensure it gets the right size regardless of the layout properties set on the inner component?

In reply to: 334535799 [](ancestors = 334535799)

@rozele
Copy link
Collaborator Author

rozele commented Oct 6, 2017

👍 we can override the TouchableWithoutFeedback to make it embed a control.

ControlWindows is not a superset of View, because it doesn't have background and border props. We could probably implement all these, but I want to talk to the XAML team about potentially exposing the BorderRadius property for the built-in Border in UserControl first. I like the idea of calling it FocusableView, we could even just call it Focusable. I don't necessarily know about upstreaming it. I know there's a concept of focusable views in tvOS, so it may be useful there, would be good to hear from @douglowder on that.

In terms of the options -

  1. This is a bad option, as you pointed out in react-native, View.js is not used in production apps because the module exports the native view directly. We wouldn't be able to do this if we added conditional logic for subbing in a control.
  2. I really don't think this is practical. For one, the view instance is instantiated before we receive any props, so we don't know at view creation time in the lifecycle if we'll need to use a Control or a Canvas. Secondly, props can change over time, so theoretically you'd have to swap a Canvas to a Control at some point, and I don't think we could unwind all the state and transfer it over to a new view; transferring children is the easy part, the hard part is transferring other props on the view and anything that's been associated with the view in conditional weak tables (we've got a few of them).
  3. Is it really tricky to size the wrapper component? Doesn't flex have plenty of options for this? I really think this is the only feasible approach to move forward with, and I really like the idea of renaming the component to Focusable and checking if it makes sense to upstream.

In reply to: 334611994 [](ancestors = 334611994,334535799)

@reseul
Copy link
Contributor

reseul commented Jan 19, 2018

@rozele I think this PR is now obsolete.

@rozele
Copy link
Collaborator Author

rozele commented Jan 19, 2018

Thanks @reseul! Closing.

@rozele rozele closed this Jan 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants