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

[react-ui] Remove event object warnings #16822

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Sep 18, 2019

This might be a bit controversial, but one that I've spent the last few weeks considering quite a bit. Today, we don't allow for the new event system to supply methods on the event object to do preventDefault and stopPropagation. The reasoning for this was based a few important points:

  1. Events are used in hooks, and hooks aren't meant to be able to affect if another event hook triggers or doesn't.
  2. We want to be able to determine during server-side render what events should be prevented and propagated when it comes to replaying the events during client-side hydration.
  3. For RN, the idea is that we can tell the native side this information declaratively ahead of time.

Propagation

We already have a form of stopPropagation in the current event system. The Keyboard responder, which has a way of basically doing stopPropagation (the return value can allow for propagation). Given that we've not run into that many issues with keyboard, this kind of already means that point 1 above was probably not needed. The hooks themselves still work, it's just their callbacks that are affected, and that doesn't actually invalidate the Rules of Hooks after-all.

Preventing native behaviour

There is a very important use-cases for where we want to be able to conditionally preventDefault at runtime – when handling user focus management. In the cases where we want to "control" where focus goes using keyboard input, we preventDefault on the key that is being pressed so native focus doesn't fire. However, what if we don't want to control the focus? There's no way to conditionally switch of preventing the native event part way through a key press. I also noticed that we end up having all these complex abstractions to try and work around the fact that we have these constraints in place, often ending up generating far more code in the process.

What about event replaying?

It would be ideal to know ahead of time if something gets prevent defaulted or not, but it feels like we're building all this complex architecture just to handle a bunch of event replaying cases. Keyboard input probably shouldn't even be replayed, and that's one of the main cases where you need conditional propagation and preventing of default behaviour. Pointer events should probably always prevent default, or at least always be declaratively known – so at least they make sense when replaying. Things like scroll should ideally never be prevented, as it leads to a terrible UX.

What about RN?

Handling of keyboard on RN is another tricky case. I'm not sure of the best way forward here. It gets even more complex because it would be nice in an ideal world for this problem space to be handled in part with Fabric, where we can yield to JS without causing as much of an issue as there is today without Fabric.

@sizebot
Copy link

sizebot commented Sep 18, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 32d1728

@necolas
Copy link
Contributor

necolas commented Sep 18, 2019

I think some of the issues you brought up also manifest in the pointer-based gestures. For example, when using Tap you might need to prevent the native behavior associated with onTap (browser navigation) but not that associated with onSecondaryTap and when modifier keys are held (browser open in new-tab). Or when using ContextMenu, you might want to prevent the native behavior when the pointerType is touch (i.e., long-press) but not when it's mouse (i.e., right-click).

Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

Approving to unblock the keyboard/focus work

@trueadm trueadm merged commit 924a305 into facebook:master Sep 19, 2019
@trueadm trueadm deleted the keyboard-prevent-default branch September 19, 2019 10:46
@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 19, 2019

To prevent "native" behavior in a cross-platform way, you need to define what "native" behavior is. We can't just say that it's the same as what the browser's native behavior is because that doesn't apply to native and native has other native behavior that might need to be prevented or not.

For the example where we "don't want to control the focus", how do you know that you don't if we don't define it cross platform.

Otherwise any such user code has to do preventDefault (browser), preventIOS, preventAndroid... under different conditions.

I really think we need to raise the level of abstraction in the responders to allow this to be defined. I still think it's doable declaratively for some definition of declaratively such as for example using combinators to describe a condition. But it doesn't necessarily have to be but we need to at least define what the behavior is so many it's more like preventReactBuiltInBehavior and then that can be implemented with native or not. I'm skeptical that this built-in behavior will map 1:1 with browser default behavior.

I think we need to define what the high level interactions are that we want to support and ensure that those work in a reasonable way across all platforms regardless if that behavior is utilizing native to do it or overrides it.

None of these concerns are strictly related to event replaying but a signal that we've done the right thing architecturally and as a result it'll probably help event replaying as well.

@necolas
Copy link
Contributor

necolas commented Sep 19, 2019

For now we need to get something that works in practice for the web and F8 use cases.

@sebmarkbage
Copy link
Collaborator

In that case should we name it DO_NOT_USE_preventDefault()?

I’m concerned about the surface area we’re exposing in Flare since it ties into core and it’s hard to get rid of it once it relies on core.

Workarounds are fine in user space.

@trueadm
Copy link
Contributor Author

trueadm commented Sep 19, 2019

In that case should we name it DO_NOT_USE_preventDefault()?

Keyboard events needs preventDefault that much is clear for what we need to build here - so having a DO_NOT_USE suffix doens't really do much here as we probably want to to be exposed in the case of keyboard (which shouldn't work via the replay event system anyway).

Ulatimetly, I think we should look at the picture differently here. Let's build out the infra for what we need today, as it's blocking progress internally, then come back later and refine it to make it more declarative. There's a solid time constaint on the former and getting that right directly impacts how the API forms later. It's all experimental and thus we have tight control over it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants