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 shouldn't bind wheel or touch events to the document. #1254

Open
joshduck opened this Issue Mar 12, 2014 · 33 comments

Comments

Projects
None yet
@joshduck
Contributor

joshduck commented Mar 12, 2014

React binds touchmove, touchstart, touchend, touchcancel and wheel handlers to the document. When the user tries to scroll the browser needs to execute these event handlers to ensure event.preventDefault() was not called. This means scrolling will stall while JavaScript is executing.

Chrome has a fast-path that checks whether the coordinate where the touch event happens has touch listeners (https://plus.google.com/+RickByers/posts/cmzrtyBYPQc). If there are no listeners Chrome can allow the scroll to happen even if the main thread is blocked on JavaScript.

We should bind our listeners for these events directly to the node which requires it. That event handler can then dispatch the event to the standard React top level event system. Then it will bubble/capture just like everything else and be visible to all event plugins.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 12, 2014

Member

This would also definitely fix our initializeTouchEvents problem. I don't know the perf impacts of not bubbling to document though.

Member

sophiebits commented Mar 12, 2014

This would also definitely fix our initializeTouchEvents problem. I don't know the perf impacts of not bubbling to document though.

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao
Member

zpao commented Mar 12, 2014

make-it-so-captain

@SanderSpies

This comment has been minimized.

Show comment
Hide comment
@SanderSpies

SanderSpies Mar 13, 2014

Contributor

Anyone picking this one up? Otherwise, I would like to give it a go :-).

Contributor

SanderSpies commented Mar 13, 2014

Anyone picking this one up? Otherwise, I would like to give it a go :-).

@possibilities

This comment has been minimized.

Show comment
Hide comment
@possibilities

possibilities Mar 14, 2014

It would also fix the problem I'm having here:

http://jsfiddle.net/BSJPd/1/

Arguably not a bug but unexpected and undesirable for me.

possibilities commented Mar 14, 2014

It would also fix the problem I'm having here:

http://jsfiddle.net/BSJPd/1/

Arguably not a bug but unexpected and undesirable for me.

@petehunt

This comment has been minimized.

Show comment
Hide comment
@petehunt

petehunt Mar 14, 2014

Contributor

@SanderSpies you're exactly who I had in mind for this! :)

Contributor

petehunt commented Mar 14, 2014

@SanderSpies you're exactly who I had in mind for this! :)

@SanderSpies

This comment has been minimized.

Show comment
Hide comment
@SanderSpies

SanderSpies Mar 14, 2014

Contributor

petehunt: great :)

Potential scenario's I'm seeing:

  • elements with listeners at the same level.

So for these event types there will be more then one event listener

  • child element listens to same event as parent element.

I guess we always want to use the parent element

  • during lifetime the dom changes, and therefore also the element which needs to listen.

Which would imply we would also need to remove/add event listeners on the fly, not sure if we already do that

I'm thinking of adding something like 'bindLocally' at the event plugins level like:

onTouchMove: {
bindLocally: true,
phasedRegistrationNames: {
bubbled: keyOf({onDragOver: true}),
captured: keyOf({onDragOverCapture: true})
}
}

Good idea? Bad idea? Missing stuff here?

Contributor

SanderSpies commented Mar 14, 2014

petehunt: great :)

Potential scenario's I'm seeing:

  • elements with listeners at the same level.

So for these event types there will be more then one event listener

  • child element listens to same event as parent element.

I guess we always want to use the parent element

  • during lifetime the dom changes, and therefore also the element which needs to listen.

Which would imply we would also need to remove/add event listeners on the fly, not sure if we already do that

I'm thinking of adding something like 'bindLocally' at the event plugins level like:

onTouchMove: {
bindLocally: true,
phasedRegistrationNames: {
bubbled: keyOf({onDragOver: true}),
captured: keyOf({onDragOverCapture: true})
}
}

Good idea? Bad idea? Missing stuff here?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 14, 2014

Member

It's really a function of the topLevelType topTouchMove, not the event that you wrote there. For naming, delegate: false might be clearer.

Right now I don't believe we ever remove listeners.

Member

sophiebits commented Mar 14, 2014

It's really a function of the topLevelType topTouchMove, not the event that you wrote there. For naming, delegate: false might be clearer.

Right now I don't believe we ever remove listeners.

@SanderSpies

This comment has been minimized.

Show comment
Hide comment
@SanderSpies

SanderSpies Mar 14, 2014

Contributor

@spicyj I wrote that code a bit too hastily I guess. Should have been touchMove, also notice the onDragOver + onDragOverCapture that I forgot to correct...

Contributor

SanderSpies commented Mar 14, 2014

@spicyj I wrote that code a bit too hastily I guess. Should have been touchMove, also notice the onDragOver + onDragOverCapture that I forgot to correct...

@SanderSpies

This comment has been minimized.

Show comment
Hide comment
@SanderSpies

SanderSpies Apr 1, 2014

Contributor

Currently the event listeners are attached before React inserts the nodes into the DOM. So I was wondering how I should handle this.

I was thinking of letting ReactEventEmitter.listenTo create a 'bag' for local events, that is later used by a new function (addLocalEventListeners?) which is called by ReactComponent._mountComponentIntoNode.

Thoughts on this?

Contributor

SanderSpies commented Apr 1, 2014

Currently the event listeners are attached before React inserts the nodes into the DOM. So I was wondering how I should handle this.

I was thinking of letting ReactEventEmitter.listenTo create a 'bag' for local events, that is later used by a new function (addLocalEventListeners?) which is called by ReactComponent._mountComponentIntoNode.

Thoughts on this?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Apr 3, 2014

Member

Are you sure it's before the DOM nodes are created? Regardless, probably you want to tie it to the transaction -- you can see how ReactReconcileTransaction stores a queue of componentDidMount callbacks (ON_DOM_READY_QUEUEING/ReactMountReady) and a queue of listeners to put (PUT_LISTENER_QUEUEING/ReactPutListenerQueue). If the put listener queue can't easily be modified to do what you want (though I'm guessing it can), you can make a new transaction wrapper and add it to the list there.

Member

sophiebits commented Apr 3, 2014

Are you sure it's before the DOM nodes are created? Regardless, probably you want to tie it to the transaction -- you can see how ReactReconcileTransaction stores a queue of componentDidMount callbacks (ON_DOM_READY_QUEUEING/ReactMountReady) and a queue of listeners to put (PUT_LISTENER_QUEUEING/ReactPutListenerQueue). If the put listener queue can't easily be modified to do what you want (though I'm guessing it can), you can make a new transaction wrapper and add it to the list there.

@u9520107

This comment has been minimized.

Show comment
Hide comment
@u9520107

u9520107 Aug 5, 2015

Is this still being investigated?
In my project, I have a component that uses scrollable div's with roughly 300 direct child, which nests maybe 6 to 7 children. While putting translateZ(0) to force composition layer seems to be able to boost scroll performance to 60 fps. As soon as the document wheel event is attached, the performance is crippled. Even after the component that relies on wheel events is dismounted, the wheel listener on document would live on, reducing the scroll performance to around 30-40 fps.

u9520107 commented Aug 5, 2015

Is this still being investigated?
In my project, I have a component that uses scrollable div's with roughly 300 direct child, which nests maybe 6 to 7 children. While putting translateZ(0) to force composition layer seems to be able to boost scroll performance to 60 fps. As soon as the document wheel event is attached, the performance is crippled. Even after the component that relies on wheel events is dismounted, the wheel listener on document would live on, reducing the scroll performance to around 30-40 fps.

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Sep 1, 2016

Contributor

This problem was also noticed by former Edge/IE engineer Justin Rogers, who now works with y'all at Facebook. 😃 React's technique of adding a global wheel listener on the document defeats optimizations that certain browsers (notably Safari and Edge) have implemented to avoid jank that only needs to occur for inner elements, not for the entire document.

To demonstrate, I put together a little sample application based on create-react-app. There's a setInterval() in there that janks the main thread for 1000 milliseconds every 2000 milliseconds. This is to simulate a very active page with lots of JS. (Update: bumped it to 3000/4000 to make it more obvious.)

There are three versions of the app: 1) no wheel events, 2) a React onWheel event on the inner scrollable div (which adds a global wheel event to the document), and 3) a regular DOM wheel event on the inner scrollable div. In all cases, the listener itself is just lodash.noop.

In Firefox and Chrome, there is no difference between (2) and (3). However, in Edge and Safari, React's technique of adding the global wheel listener has the unfortunate side-effect of causing the entire page to jank while scrolling, rather than just when you're scrolling the inner div.

Note that this optimization only applies to certain kinds of scroll events like mousewheel scrolling, not necessarily two-finger touchpad scrolling, clicking-and-dragging the scrollbar, or keyboard-scrolling. So you may need to use a real mouse to reproduce. It also may apply to scroll, touchstart, touchmove, etc., but I only tested wheel, since the others have their own performance characteristics and may only be impacted on touchscreen devices. It's worth investigating, but for this demo I focused on wheel.

It's kind of a subtle issue, so I also made a quick video demonstration: https://youtu.be/6Ckepx3wPPE

Contributor

nolanlawson commented Sep 1, 2016

This problem was also noticed by former Edge/IE engineer Justin Rogers, who now works with y'all at Facebook. 😃 React's technique of adding a global wheel listener on the document defeats optimizations that certain browsers (notably Safari and Edge) have implemented to avoid jank that only needs to occur for inner elements, not for the entire document.

To demonstrate, I put together a little sample application based on create-react-app. There's a setInterval() in there that janks the main thread for 1000 milliseconds every 2000 milliseconds. This is to simulate a very active page with lots of JS. (Update: bumped it to 3000/4000 to make it more obvious.)

There are three versions of the app: 1) no wheel events, 2) a React onWheel event on the inner scrollable div (which adds a global wheel event to the document), and 3) a regular DOM wheel event on the inner scrollable div. In all cases, the listener itself is just lodash.noop.

In Firefox and Chrome, there is no difference between (2) and (3). However, in Edge and Safari, React's technique of adding the global wheel listener has the unfortunate side-effect of causing the entire page to jank while scrolling, rather than just when you're scrolling the inner div.

Note that this optimization only applies to certain kinds of scroll events like mousewheel scrolling, not necessarily two-finger touchpad scrolling, clicking-and-dragging the scrollbar, or keyboard-scrolling. So you may need to use a real mouse to reproduce. It also may apply to scroll, touchstart, touchmove, etc., but I only tested wheel, since the others have their own performance characteristics and may only be impacted on touchscreen devices. It's worth investigating, but for this demo I focused on wheel.

It's kind of a subtle issue, so I also made a quick video demonstration: https://youtu.be/6Ckepx3wPPE

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Sep 1, 2016

Contributor

Update: added onTouchStart/touchstart to the demo as well, and confirmed that this causes unnecessarily janky touch-scrolling in all mobile browsers (iOS Safari, Android Chrome, Android Firefox, Edge Mobile).

I compiled the results into a table for those who want to see a browser-by-browser breakdown.

Contributor

nolanlawson commented Sep 1, 2016

Update: added onTouchStart/touchstart to the demo as well, and confirmed that this causes unnecessarily janky touch-scrolling in all mobile browsers (iOS Safari, Android Chrome, Android Firefox, Edge Mobile).

I compiled the results into a table for those who want to see a browser-by-browser breakdown.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Sep 1, 2016

Member

We just published a code dive into React event system (thanks @spicyj and @kentcdodds!)
This would be helpful to anyone who’d like to dig into this.

Member

gaearon commented Sep 1, 2016

We just published a code dive into React event system (thanks @spicyj and @kentcdodds!)
This would be helpful to anyone who’d like to dig into this.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 26, 2017

Collaborator

Just read this article and found my way here. I've got some bandwidth now that the number input issue is more or less wrapped up.

Is anyone working on this? Anything I should keep in mind before I get started?

Collaborator

nhunzaker commented Mar 26, 2017

Just read this article and found my way here. I've got some bandwidth now that the number input issue is more or less wrapped up.

Is anyone working on this? Anything I should keep in mind before I get started?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 28, 2017

Collaborator

Working on a fix here:
master...nhunzaker:nh-scroll-fix

This change allows ReactBrowserEventEmitter, which is responsible for attaching event listeners, to make the decision on a case-by-case basis as to what events should be attached to the document vs the element itself.

I still haven't gotten into touch events or serious wheel testing, but I can confirm that it gets rid of the window scroll jank in @nolanlawson v2 example

Time to find a mouse with a wheel!

Collaborator

nhunzaker commented Mar 28, 2017

Working on a fix here:
master...nhunzaker:nh-scroll-fix

This change allows ReactBrowserEventEmitter, which is responsible for attaching event listeners, to make the decision on a case-by-case basis as to what events should be attached to the document vs the element itself.

I still haven't gotten into touch events or serious wheel testing, but I can confirm that it gets rid of the window scroll jank in @nolanlawson v2 example

Time to find a mouse with a wheel!

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 29, 2017

Member

@nhunzaker We should chat about this strategy. There are several considerations here.

  1. We want to minimize the amount of work we do during initial render which includes minimizing the checks we do on events. Currently we do way too much work just to figure out that we've already attached an event.

  2. We noticed in #8117 that moving event listeners locally had many unexpected consequences on other parts of the system so it was harder than it initially seemed. E.g. it can break how the select event plugin works. We need to make sure that all that stuff plays nicely together.

Member

sebmarkbage commented Mar 29, 2017

@nhunzaker We should chat about this strategy. There are several considerations here.

  1. We want to minimize the amount of work we do during initial render which includes minimizing the checks we do on events. Currently we do way too much work just to figure out that we've already attached an event.

  2. We noticed in #8117 that moving event listeners locally had many unexpected consequences on other parts of the system so it was harder than it initially seemed. E.g. it can break how the select event plugin works. We need to make sure that all that stuff plays nicely together.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 29, 2017

Collaborator

Absolutely. Is reactiflux, the discord room, best? Really any medium is fine.

Re 1: Are there any known bottlenecks that you could point me to?

Re 2: Totally, and hard to test. It would be good to get some DOM fixtures in place as we uncover issues. That was going to be my next step here.

Collaborator

nhunzaker commented Mar 29, 2017

Absolutely. Is reactiflux, the discord room, best? Really any medium is fine.

Re 1: Are there any known bottlenecks that you could point me to?

Re 2: Totally, and hard to test. It would be good to get some DOM fixtures in place as we uncover issues. That was going to be my next step here.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 30, 2017

Member

Probably best to start here. I just wanted to highlight those issue so you're aware.

  1. ensureListeningTo and these steps in ReactBrowserEventEmitter are currently executed for every event listener in the entire page. That's several slowish DOM operations, map lookups, hasOwnProperties and property look ups PER every listener before we can show the first screen. In theory, if we attached listeners to every event at the document eagerly, we wouldn't have to do any of that for any event listener.

  2. I think wheel specifically is probably pretty safe but I believe at least one platform it might be possible to change the value in a <select /> with the wheel which might or might not be related.

Member

sebmarkbage commented Mar 30, 2017

Probably best to start here. I just wanted to highlight those issue so you're aware.

  1. ensureListeningTo and these steps in ReactBrowserEventEmitter are currently executed for every event listener in the entire page. That's several slowish DOM operations, map lookups, hasOwnProperties and property look ups PER every listener before we can show the first screen. In theory, if we attached listeners to every event at the document eagerly, we wouldn't have to do any of that for any event listener.

  2. I think wheel specifically is probably pretty safe but I believe at least one platform it might be possible to change the value in a <select /> with the wheel which might or might not be related.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Mar 31, 2017

Collaborator

@sebmarkbage I recently had a few PRs merged that remove some of the work in BrowserEventEmitter doing support checks we don't need. Next I'm going to see what kind of savings we actually get by just assigning an event listener for every supported event.

Without changing things too much, I want to figure out what's slow, eliminate that work, then evaluate reworking ReactBrowserEventEmitter with less moving parts (and I better understand what's happening).

I'm also curious if we can reduce the number of calls to ensureListeningTo for special cases in ReactDomComponent

Is that a reasonable approach?

Collaborator

nhunzaker commented Mar 31, 2017

@sebmarkbage I recently had a few PRs merged that remove some of the work in BrowserEventEmitter doing support checks we don't need. Next I'm going to see what kind of savings we actually get by just assigning an event listener for every supported event.

Without changing things too much, I want to figure out what's slow, eliminate that work, then evaluate reworking ReactBrowserEventEmitter with less moving parts (and I better understand what's happening).

I'm also curious if we can reduce the number of calls to ensureListeningTo for special cases in ReactDomComponent

Is that a reasonable approach?

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Apr 1, 2017

Contributor

Restricting the set of "prefer local listeners" to wheel, touchstart, and touchmove is probably a good start. All of those events can block scrolling in Edge/Safari/etc. but only if done globally.

FWIW, in case you don't want to go to the trouble of buying a mouse with a wheel, I've found that two-fingered scrolling on a touchpad usually exhibits identical behavior.

Contributor

nolanlawson commented Apr 1, 2017

Restricting the set of "prefer local listeners" to wheel, touchstart, and touchmove is probably a good start. All of those events can block scrolling in Edge/Safari/etc. but only if done globally.

FWIW, in case you don't want to go to the trouble of buying a mouse with a wheel, I've found that two-fingered scrolling on a touchpad usually exhibits identical behavior.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 3, 2017

Collaborator

@nolanlawson Thank you for the guidance! I have a mouse with a wheel (thank goodness for conference swag), but I wasn't sure about two-fingered scrolling, that's really good to know.

@sebmarkbage The only hiccup I'm hitting now is that to-string rendering (at least running tests against the feature flag: useCreateElement: false throws an error because the node to attach the listener for doesn't exist yet.

ensureListeningTo, which runs to make sure a top-level event listener is attached to the document, looks something like this:

function ensureListeningTo(inst, registrationName, transaction) {
  if (transaction instanceof ReactServerRenderingTransaction) {
    return;
  }

  listenTo(registrationName, getDocument(inst), getNode(inst));
}

The problem is that getNode(inst) fails some times when the useCreateElement feature flag is set to false. You see this warning:

Invariant Violation: React DOM tree root should always have a node reference.

So I had a few questions/ideas:

  1. Is setting useCreateElement: false still the best way to test server-side rendering?
  2. What about doing enqueuing event listener attachment post mount, like:
transaction.getReactMountReady().enqueue(attachListeners, this);

If we did this, what do you think about moving all of the local listener logic out of ReactDOMComponent and into ReactBrowserEventEmitter? For example, this section here:

https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactDOMComponent.js#L254-L310

Then, possibly, we could eliminate the unmount work here:

https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactDOMComponent.js#L1107-L1159

Collaborator

nhunzaker commented Apr 3, 2017

@nolanlawson Thank you for the guidance! I have a mouse with a wheel (thank goodness for conference swag), but I wasn't sure about two-fingered scrolling, that's really good to know.

@sebmarkbage The only hiccup I'm hitting now is that to-string rendering (at least running tests against the feature flag: useCreateElement: false throws an error because the node to attach the listener for doesn't exist yet.

ensureListeningTo, which runs to make sure a top-level event listener is attached to the document, looks something like this:

function ensureListeningTo(inst, registrationName, transaction) {
  if (transaction instanceof ReactServerRenderingTransaction) {
    return;
  }

  listenTo(registrationName, getDocument(inst), getNode(inst));
}

The problem is that getNode(inst) fails some times when the useCreateElement feature flag is set to false. You see this warning:

Invariant Violation: React DOM tree root should always have a node reference.

So I had a few questions/ideas:

  1. Is setting useCreateElement: false still the best way to test server-side rendering?
  2. What about doing enqueuing event listener attachment post mount, like:
transaction.getReactMountReady().enqueue(attachListeners, this);

If we did this, what do you think about moving all of the local listener logic out of ReactDOMComponent and into ReactBrowserEventEmitter? For example, this section here:

https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactDOMComponent.js#L254-L310

Then, possibly, we could eliminate the unmount work here:

https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactDOMComponent.js#L1107-L1159

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 3, 2017

Member

useCreateElement: false is what happens when you render into a server rendered tree on the client. We're in need of imminently rewriting that whole algorithm and likely need to switch strategies for Fiber anyway.

Member

sebmarkbage commented Apr 3, 2017

useCreateElement: false is what happens when you render into a server rendered tree on the client. We're in need of imminently rewriting that whole algorithm and likely need to switch strategies for Fiber anyway.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 4, 2017

Collaborator

I took another pass:
master...nhunzaker:nh-scroll-fix

This diff further moves event listener location handling to BrowserEventEmitter. Here's how it works:

  1. createElement:false runs a post-mount hook to attach all listeners
  2. _updateDOMProperties runs BrowserEventEmitter.listenTo(eventName, document, element), more or less like normal.
  3. BrowserEventEmitter.listenTo is responsible for figuring out if a listener should be attached locally or globally.

No fiber tests fail! But we probably have a few more tests to write.

What does fail is createElement:false in the stack renderer: Because attaching event listeners to invalid nested markup means ReactDOMComponentTree can't find them.

For example:

<div>
  <tr></tr>
  <tr></tr>
</div>

When ReactDOMComponentTree goes to precache the nodes, the <tr> tags can't be found in the div, they seem to get stripped out. This causes the following validation error:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponentTree.js#L115

ReactDOMComponent › nesting validation › warns nicely for table rows
Invariant Violation: Unable to find element with ID 2.

So I'm looking for a good check to run to see if the node associated with a ReactDOMComponent instance is actually mountable. This is further complicated by the <source> tag inside a <video> element. Most of my attempts either fail this test:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L759-L779

Or this family of tests:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L1263-L1277

Or

Do we even need to do listener attachment if createElement is set to false?

Collaborator

nhunzaker commented Apr 4, 2017

I took another pass:
master...nhunzaker:nh-scroll-fix

This diff further moves event listener location handling to BrowserEventEmitter. Here's how it works:

  1. createElement:false runs a post-mount hook to attach all listeners
  2. _updateDOMProperties runs BrowserEventEmitter.listenTo(eventName, document, element), more or less like normal.
  3. BrowserEventEmitter.listenTo is responsible for figuring out if a listener should be attached locally or globally.

No fiber tests fail! But we probably have a few more tests to write.

What does fail is createElement:false in the stack renderer: Because attaching event listeners to invalid nested markup means ReactDOMComponentTree can't find them.

For example:

<div>
  <tr></tr>
  <tr></tr>
</div>

When ReactDOMComponentTree goes to precache the nodes, the <tr> tags can't be found in the div, they seem to get stripped out. This causes the following validation error:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponentTree.js#L115

ReactDOMComponent › nesting validation › warns nicely for table rows
Invariant Violation: Unable to find element with ID 2.

So I'm looking for a good check to run to see if the node associated with a ReactDOMComponent instance is actually mountable. This is further complicated by the <source> tag inside a <video> element. Most of my attempts either fail this test:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L759-L779

Or this family of tests:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L1263-L1277

Or

Do we even need to do listener attachment if createElement is set to false?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 4, 2017

Member

Do we even need to do listener attachment if createElement is set to false?

No, createElement: false is only used for validating server markup on the client. Its result is thrown away.

Member

gaearon commented Apr 4, 2017

Do we even need to do listener attachment if createElement is set to false?

No, createElement: false is only used for validating server markup on the client. Its result is thrown away.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 4, 2017

Collaborator

👍 . That solves my problem then

Collaborator

nhunzaker commented Apr 4, 2017

👍 . That solves my problem then

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 4, 2017

Collaborator

Okay. PR soon. here's a test page:
http://nh-react-scroll-fix.surge.sh

Associated code:
https://github.com/nhunzaker/react-wheel-jank-demo

(Thanks @nolanlawson!)

I believe this fixes the issues with wheel, touchstart, and touchmove events. But I need to test touchstart and touchmove on Edge. That's outside of my capabilities. Is this something someone else could take up?

Beyond that, generally, would others be able to help verify?

Collaborator

nhunzaker commented Apr 4, 2017

Okay. PR soon. here's a test page:
http://nh-react-scroll-fix.surge.sh

Associated code:
https://github.com/nhunzaker/react-wheel-jank-demo

(Thanks @nolanlawson!)

I believe this fixes the issues with wheel, touchstart, and touchmove events. But I need to test touchstart and touchmove on Edge. That's outside of my capabilities. Is this something someone else could take up?

Beyond that, generally, would others be able to help verify?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Apr 5, 2017

Member

No, createElement: false is only used for validating server markup on the client. Its result is thrown away.

Wait, I don't think that's true. We use it only when rendering on top of server markup, but when called on the client it does need to attach event handlers! Like Seb said though, server rendering attaching code needs to get rewritten soon for Fiber so we're not sure what will happen.

Member

sophiebits commented Apr 5, 2017

No, createElement: false is only used for validating server markup on the client. Its result is thrown away.

Wait, I don't think that's true. We use it only when rendering on top of server markup, but when called on the client it does need to attach event handlers! Like Seb said though, server rendering attaching code needs to get rewritten soon for Fiber so we're not sure what will happen.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 5, 2017

Collaborator

@spicyj Well shoot... okay. Any ideas? What is the best way to handle invalidly nested tags that never get created, but React is still tracking?

Would this discussion be better in #9333? Or do you want me to close that out and keep talking here?

Collaborator

nhunzaker commented Apr 5, 2017

@spicyj Well shoot... okay. Any ideas? What is the best way to handle invalidly nested tags that never get created, but React is still tracking?

Would this discussion be better in #9333? Or do you want me to close that out and keep talking here?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Apr 5, 2017

Member

Sorry, I haven't been following this whole thread – I just saw the one comment and it seemed wrong. In the div > tr example, why are we doing anything with event listeners? I must be missing something because I would have assumed we skip all the events code for that test.

We do have a public Messenger room at https://m.me/g/Abas8IcWxIvjVxHN which everyone is welcome to participate in. I'm usually responsive in close to real time and happy to answer questions.

Member

sophiebits commented Apr 5, 2017

Sorry, I haven't been following this whole thread – I just saw the one comment and it seemed wrong. In the div > tr example, why are we doing anything with event listeners? I must be missing something because I would have assumed we skip all the events code for that test.

We do have a public Messenger room at https://m.me/g/Abas8IcWxIvjVxHN which everyone is welcome to participate in. I'm usually responsive in close to real time and happy to answer questions.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Apr 5, 2017

Collaborator

Perfect, I'll hop in.

Collaborator

nhunzaker commented Apr 5, 2017

Perfect, I'll hop in.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 5, 2017

Member

We use it only when rendering on top of server markup, but when called on the client it does need to attach event handlers!

Oops, you’re right, thanks for correcting me! Still learning 😄 Sorry @nhunzaker for confusing you.

Member

gaearon commented Apr 5, 2017

We use it only when rendering on top of server markup, but when called on the client it does need to attach event handlers!

Oops, you’re right, thanks for correcting me! Still learning 😄 Sorry @nhunzaker for confusing you.

@philipp-spiess

This comment has been minimized.

Show comment
Hide comment
@philipp-spiess

philipp-spiess Aug 21, 2018

Collaborator

Mentioning #2043 here as "probably related". 🙂

Collaborator

philipp-spiess commented Aug 21, 2018

Mentioning #2043 here as "probably related". 🙂

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