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

Support Pointer Events #12507

Merged
merged 2 commits into from
May 16, 2018
Merged

Support Pointer Events #12507

merged 2 commits into from
May 16, 2018

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Mar 31, 2018

Closes #499

This PR adds support for Pointer Events as discussed in #499. It is heavily based on previous work in #1389 and will add most pointer events to the SimpleEventPlugin and enter/leave events to EnterLeaveEventPlugin.

I added a new DOM fixture to test all pointer events and make sure my implementation does indeed work. I tested on Chrome 65 and Firefox 59 without seeing any issues. If you think the fixtures is not necessary for future changes, I'm happy to remove them as well.

The only open question is if we want to add a polyfill. For the sake of simplicity, I opted against a polyfill for this PR. However, this work is compatible with PEP (I've verified this behavior in Safari 11 by loading PEP in fixtures/dom/public/index.html).

@jquense
Copy link
Contributor

jquense commented Mar 31, 2018

Nice! I think a polyfill is probably not needed here. In general we are trying to less of that to simplify and shrink react-dom

@philipp-spiess
Copy link
Contributor Author

philipp-spiess commented Mar 31, 2018

@jquense I absolutely agree. It’s not worth the maintenance costs if there are well-established alternatives like PEP that work (although with limitations on their own). It would also add a lot of code for a feature that, based on #499, is not requested by a lot of people.

philipp-spiess added a commit to philipp-spiess/reactjs.org that referenced this pull request Apr 11, 2018
This PR adds a section about the state of Pointer Events in React.

This should be merged only if facebook/react#12507 is accepted as well.
@pull-bot
Copy link

pull-bot commented Apr 16, 2018

ReactDOM: size: 🔺+0.9%, gzip: 🔺+0.7%

Details of bundled changes.

Comparing: de84d5c...5f3167d

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.4% +0.3% 642.04 KB 644.49 KB 147.47 KB 147.96 KB UMD_DEV
react-dom.production.min.js 🔺+0.9% 🔺+0.7% 100.22 KB 101.14 KB 32.13 KB 32.36 KB UMD_PROD
react-dom.development.js +0.4% +0.3% 626.41 KB 628.86 KB 143.41 KB 143.86 KB NODE_DEV
react-dom.production.min.js 🔺+0.9% 🔺+0.8% 98.63 KB 99.55 KB 31.22 KB 31.46 KB NODE_PROD
react-dom-test-utils.development.js 0.0% 0.0% 45.03 KB 45.04 KB 12.36 KB 12.37 KB UMD_DEV
react-dom-test-utils.development.js 0.0% +0.1% 39.89 KB 39.9 KB 10.93 KB 10.94 KB NODE_DEV
ReactDOM-dev.js +0.4% +0.4% 651.6 KB 654.33 KB 146.38 KB 146.91 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.7% 🔺+0.7% 299.69 KB 301.89 KB 54.56 KB 54.92 KB FB_WWW_PROD
ReactTestUtils-dev.js 0.0% 0.0% 41.29 KB 41.29 KB 11.13 KB 11.14 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2018

I don’t think we can afford to increase the bundle size with more events, at least not now.

If we want to do this we need to find something to cut.

@philipp-spiess
Copy link
Contributor Author

@gaearon Thanks for looking into that! Any ideas where/how we can loose some weight?

@jquense
Copy link
Contributor

jquense commented Apr 16, 2018

No more adding any events? Or polyfill? I think react DOM has a responsibility to support the new things in the platform as much as it has one to web perf. Not mention adding new events without bundle increases will likely require a rewrite of the event system. The latter is going to take a long time and involve a lot of input across renderers and FB in general, I hate for RD to fall behind in platform support in the meantime.

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2018

No more adding any events? Or polyfill?

I’m not saying to never add them. But this is adding a lot.

Not mention adding new events without bundle increases will likely require a rewrite of the event system.

I think that might be an overstatement. To drop the 1K pre-gzip, we need to find a way to drop the 1K pre-gzip. Not necessarily rewrite the event system. It could be composed of small improvements anywhere.

We get a lot of PRs with features. I appreciate this but I think it's time more people start hunting for opportunities to cut the size if we want to ship those features.

Any ideas where/how we can loose some weight?

I tend to start by running yarn build core,dom --type=UMD_PROD --pretty. Read the bundles in build/dist. Look for opportunities to cut the code. Repeat.

Here are some examples of me doing it: #11912 #11800 #10803 #10802 #10671 #10227. There was no specific pattern other than I looked through all the bundle code, found dubious logic, and deleted or refactored it.

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2018

To clarify the previous comment. I don’t mean we need to find a way to add events without increasing the whitelist right now (although that would be nice). I mean that if we want to add something, we need to start being more proactive at removing something. Even if those things are not directly related. This applies both to the core team and to the outside contributions.

@jquense
Copy link
Contributor

jquense commented Apr 16, 2018

I think that might be an overstatement.

Sorry, it's an overstatement if the idea is we should find anything to remove. I do think this PR illustrates that long term, the current system is not great, if we don't want to increase the bundle size as the platform grows. I'd love to start making incremental improvements to the events, (as well as @nhunzaker tho we've both been short on time :/ ). I would say tho, that our explorations there, indicate tho that incremental improvements are hard there, as the whole system sort of neatly rests on itself. Maybe on that front we could do another small call and debrief a bit out it and chart a way forward.

This applies both to the core team and to the outside contributions.

Ya that's great, We should and definitely start building an environment of trimming down the code. I'm just a bit worried about the added outside cost. This adds an even steeper hill to outside contributors, in already hard-to-contribute-to code base. Finding things that are safe to delete requires a lot of experience in the codebase, and as a point of contribution, it's a lot harder to get reviews for those sort of changes. I totally understand the sentiment i just want to make sure we aren't pushing folks away from submitting good additions, because they can't cut an equal amount out somewhere.

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2018

I do think this PR illustrates that long term, the current system is not great,

Agreed! Rewriting it is a very ambitious goal that also has downstream effects (e.g. react-native-web currently strongly relies on it). So I don't think it's feasible to look at this holistically yet.

as a point of contribution, it's a lot harder to get reviews for those sort of changes

I’m always happy to review bundle size improvements outside of the normal queue. I agree it's not easy but it's necessary.

@aweary
Copy link
Contributor

aweary commented Apr 16, 2018

I think identifying if we can move forward with #11894 would be a good opportunity to reduce bundle size.

@gaearon
Copy link
Collaborator

gaearon commented Apr 16, 2018

I won't have time to do it but happy to review.

@necolas
Copy link
Contributor

necolas commented Apr 19, 2018

Is the idea that any bundle size increase at all is a blocker? Or only above a certain absolute size, or is there a % threshold?

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2018

I'm uncomfortable adding non-core features that increase size at the same time as we're working on core features that will also increase the size.

It's not that there's any particular change or a size limit, but the fact that we get very few pull requests that remove code. So every non-core feature contributes to a death by a thousands cuts.

There was a lot of hard work to get the bundle size down with 16 (partially to “free the space” for new features) and the bar for taking unplanned changes that increase the size is becoming higher now. That can be mitigated with proactive work on getting it down again, but it requires some investment.

@gaearon
Copy link
Collaborator

gaearon commented May 15, 2018

@philipp-spiess Can you please rebase so we can land this? Thanks!

@philipp-spiess
Copy link
Contributor Author

🎉 The PR is now rebased against my changes from #12629. I've updated the DOM fixture as well to reflect the example we also use for the PanResponder in RN.

Pointer Events DOM Fixture

I tested the using the DOM fixture against the following browsers to make sure everything works as expected:

  • Chrome 66
  • Firefox 60
  • Edge 41
  • IE 11

However I did find an issue while testing with PEP in Safari 11. According to MDN, gotpointercapture and lostpointercapture should not bubble and that is the behavior I'm seeing when using PEP as well. This means that those events won't fire in React as they can't be listened to on the top level. This is, however, seemingly not an issue in all of the browsers tested above.

I assume that proper support would need code added in ReactDOMFiberComponent similar to what we do for media events but in an even hotter path since it needs to check for onGotPointerCapture or onLostPointerCapture for every element.

I think it's a better tradeoff to not fully support PEP for now, hence I've removed the mention of PEP in the parallel guide article (reactjs/react.dev#792). It's still possible to use PEP by adding custom event handler to the DOM node. Maybe this is something we want to mention in the docs?

@gaearon gaearon merged commit 49979bb into facebook:master May 16, 2018
@gaearon
Copy link
Collaborator

gaearon commented May 16, 2018

This looks good to me.

@gaearon
Copy link
Collaborator

gaearon commented May 16, 2018

(To add context to my earlier comments, @philipp-spiess made a pretty big refactoring to save some bytes in #12629. So that's why I'm comfortable taking this now. :-)

@jquense
Copy link
Contributor

jquense commented May 16, 2018

woohoo! Thanks ya'll

@jquense jquense mentioned this pull request May 22, 2018
motiz88 added a commit to motiz88/flow that referenced this pull request Aug 12, 2018
This is based on facebook/react#12507 which is
seemingly missing a couple of fields from the official spec.
Consequently this will need an update once React supports all fields.
philipp-spiess pushed a commit that referenced this pull request Aug 12, 2018
While working on facebook/flow#6728 I noticed React's recently-added `SyntheticPointerEvent` was missing the [`tangentialPressure`](https://www.w3.org/TR/pointerevents/#dom-pointerevent-tangentialpressure) and [`twist`](https://www.w3.org/TR/pointerevents/#dom-pointerevent-twist) fields. I couldn't find any reason for their omission in #12507 (nor in the spec) so I assume they were meant to be included, like the rest of `PointerEvent`. This PR adds these two fields to `SyntheticPointerEvent`.
motiz88 added a commit to motiz88/flow that referenced this pull request Aug 12, 2018
This is based on facebook/react#12507 which is
seemingly missing a couple of fields from the official spec.
Consequently this will need an update once React supports all fields.
facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Sep 5, 2018
Summary:
Closes #6373, closes #3227.

The definition of `SyntheticPointerEvent` here was originally based on facebook/react#12507 (released in React DOM 16.4.0) which was missing a couple of fields (`twist` and `tangentialPressure`) from the official spec.

I've now updated this branch to include those fields, since facebook/react#13374 has been merged. However, for maximum correctness we should probably wait for that to be _released_ before releasing the corresponding type definitions. I'll update this notice once the relevant React DOM release happens.
Pull Request resolved: #6728

Reviewed By: fishythefish

Differential Revision: D9338459

Pulled By: mrkev

fbshipit-source-id: 263f4922e8a6765e4a2d6553b5af785bdc65a894
villesau pushed a commit to villesau/flow that referenced this pull request Mar 18, 2019
This is based on facebook/react#12507 which is
seemingly missing a couple of fields from the official spec.
Consequently this will need an update once React supports all fields.
villesau pushed a commit to villesau/flow that referenced this pull request Mar 18, 2019
This is based on facebook/react#12507 which is
seemingly missing a couple of fields from the official spec.
Consequently this will need an update once React supports all fields.
OhIAmFine pushed a commit to OhIAmFine/zh-hans.reactjs.org that referenced this pull request May 6, 2019
This PR adds a section about the state of Pointer Events in React.

This should be merged only if facebook/react#12507 is accepted as well.
OhIAmFine pushed a commit to OhIAmFine/zh-hans.reactjs.org that referenced this pull request May 6, 2019
* Add section about Pointer Events

This PR adds a section about the state of Pointer Events in React.

This should be merged only if facebook/react#12507 is accepted as well.

* Don’t recommend PEP because it lacks features
villesau pushed a commit to villesau/flow that referenced this pull request May 7, 2019
This is based on facebook/react#12507 which is
seemingly missing a couple of fields from the official spec.
Consequently this will need an update once React supports all fields.
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.

Support Pointer events specification
9 participants