Restore DOM selection and suppress events #8353

sophiebits · 2016-11-18T23:23:36Z

This makes Draft mostly work.

sebmarkbage · 2016-11-19T03:00:41Z

I think that this just happens because we currently always reset the root. Otherwise it is usually to remove and insert the selected node. We can't keep doing that. We will instead use the append/remove child methods for the root too.

However, even in that case this could be needed when a child that is selected is the one that needs to move. It needs to be fixed somewhere else though.

I suspect we need something in the beginning and end of commitAllWork.

sebmarkbage

We should do this in a way that covers all mutations in a commit instead of just the top which happens to work right now but won't soon.

sophiebits · 2016-11-23T00:01:41Z

How about this? Sucks that I had to change all the other files.

gaearon · 2016-11-23T01:09:59Z

Can you add a test to ReactIncrementalErrorHandling verifying that environment state gets reset after an uncaught error?

sebmarkbage · 2016-11-23T01:24:46Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    return {
+      eventsEnabled,
+      selectionInformation: ReactInputSelection.getSelectionInformation(),
+    };


Any reason we need to allocate instead of just mutating something in the closure here?

sebmarkbage · 2016-11-23T04:01:55Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+    if (!isCommitting) {
+      isCommitting = true;
+      config.prepareForCommit();
+    }


It realized that this is not actually the correct behavior because if a life-cycle focuses a different thing we don't want to restore it back again.

Since these transaction wrappers are before the DOM ready queue and they execute in order it seems to me that the restore should actually happen before the life-cycles get invoked. So this whole block needs to move up to line 214.

sebmarkbage · 2016-11-23T04:03:14Z

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js

@@ -275,4 +275,24 @@ describe('ReactIncrementalErrorHandling', () => {
    expect(ReactNoop.getChildren('e')).toEqual(null);
    expect(ReactNoop.getChildren('f')).toEqual(null);
  });
+
+  it('restores environment state despite error during commit', () => {


Ideally we'd have a DOM specific test for this that tests the observable intent.

sebmarkbage · 2016-11-23T04:06:55Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-          break;
+    if (!isCommitting) {
+      isCommitting = true;
+      config.prepareForCommit();


These should both be imported as constants like these

react/src/renderers/shared/fiber/ReactFiberCommitWork.js

Lines 35 to 41 in e3131c1

const updateContainer = config.updateContainer;

const commitUpdate = config.commitUpdate;

const commitTextUpdate = config.commitTextUpdate;

const appendChild = config.appendChild;

const insertBefore = config.insertBefore;

const removeChild = config.removeChild;

sophiebits · 2016-11-23T07:56:03Z

Better, maybe. Now that we don't run user code before the restoration it's not totally clear to me that we need the try/catch but I guess it's safer to keep it.

sophiebits · 2016-11-23T07:56:53Z

I added a test that fails if you restore state after lifecycles.

sebmarkbage · 2016-11-23T19:02:12Z

It is still possible to fail if a ref callback which receives null is invoked. For example: ref={n => n.nodeName}. Those are invoked in the first phase.

sebmarkbage · 2016-11-23T19:14:10Z

Interestingly that case doesn't seem covered in the same way tryCallComponentWillUnmount is doing. I think we can just remove the try/finally and treat this as a bug in the ref impl.

This makes Draft mostly work.

sebmarkbage · 2016-11-23T21:43:31Z

src/renderers/noop/ReactNoop.js

@@ -255,6 +270,8 @@ var ReactNoop = {

  syncUpdates: NoopRenderer.syncUpdates,

+  isCommitting: () => isCommitting,


I guess we can remove this now since we're not testing this.

sebmarkbage · 2016-11-23T22:09:03Z

src/renderers/dom/stack/client/__tests__/ReactDOM-test.js

+    // something that could happen when manipulating DOM nodes (but is hard to
+    // deterministically force without relying intensely on React DOM
+    // implementation details)
+    var div = container.firstChild;


This caused a lint error.

Sorry, I turned off my precommit hook yesterday and forgot to reenable it.

This makes Draft mostly work.

sophiebits assigned sebmarkbage Nov 18, 2016

facebook-github-bot added GH Review: review-needed CLA Signed labels Nov 18, 2016

sebmarkbage added GH Review: needs-revision and removed GH Review: review-needed labels Nov 22, 2016

sebmarkbage requested changes Nov 22, 2016

View reviewed changes

sophiebits force-pushed the fiber-sel branch from 9cf5de5 to 5d037a0 Compare November 23, 2016 00:01

sophiebits changed the title ~~Restore DOM selection and suppress events in updateContainer~~ Restore DOM selection and suppress events Nov 23, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 23, 2016

sebmarkbage reviewed Nov 23, 2016

View reviewed changes

sophiebits force-pushed the fiber-sel branch from 5d037a0 to 7838575 Compare November 23, 2016 03:26

sebmarkbage requested changes Nov 23, 2016

View reviewed changes

sebmarkbage reviewed Nov 23, 2016

View reviewed changes

sebmarkbage added GH Review: needs-revision and removed GH Review: review-needed labels Nov 23, 2016

sophiebits force-pushed the fiber-sel branch from 7838575 to de9824c Compare November 23, 2016 07:55

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 23, 2016

sebmarkbage approved these changes Nov 23, 2016

View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Nov 23, 2016

Restore DOM selection and suppress events

3448450

This makes Draft mostly work.

sophiebits force-pushed the fiber-sel branch from de9824c to 3448450 Compare November 23, 2016 21:00

sophiebits merged commit 0c885af into facebook:master Nov 23, 2016

sebmarkbage reviewed Nov 23, 2016

View reviewed changes

sophiebits added a commit to sophiebits/react that referenced this pull request Nov 23, 2016

Cleanup after facebook#8353

3310f2b

sophiebits added a commit to sophiebits/react that referenced this pull request Nov 23, 2016

Cleanup after facebook#8353

7af9af6

sophiebits added a commit to sophiebits/react that referenced this pull request Nov 23, 2016

Cleanup after facebook#8353

8d3635a

sophiebits added a commit that referenced this pull request Nov 23, 2016

Cleanup after #8353 (#8403)

53cdc4b

sebmarkbage mentioned this pull request Nov 24, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017

Restore DOM selection and suppress events (facebook#8353)

6e8ac00

This makes Draft mostly work.

acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017

Cleanup after facebook#8353 (facebook#8403)

2dec591

acusti mentioned this pull request Oct 31, 2017

Support iframes (nested browsing contexts) in selection event handling for React 16 #11410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore DOM selection and suppress events #8353

Restore DOM selection and suppress events #8353

sophiebits commented Nov 18, 2016 •

edited

sebmarkbage commented Nov 19, 2016

sebmarkbage left a comment

sophiebits commented Nov 23, 2016

gaearon commented Nov 23, 2016

sebmarkbage Nov 23, 2016

sebmarkbage Nov 23, 2016

sebmarkbage Nov 23, 2016

sebmarkbage Nov 23, 2016

sophiebits commented Nov 23, 2016

sophiebits commented Nov 23, 2016

sebmarkbage commented Nov 23, 2016

sebmarkbage commented Nov 23, 2016 •

edited

sebmarkbage Nov 23, 2016

sebmarkbage Nov 23, 2016

sophiebits Nov 23, 2016

sophiebits Nov 23, 2016

	const updateContainer = config.updateContainer;
	const commitUpdate = config.commitUpdate;
	const commitTextUpdate = config.commitTextUpdate;

	const appendChild = config.appendChild;
	const insertBefore = config.insertBefore;
	const removeChild = config.removeChild;

		@@ -255,6 +270,8 @@ var ReactNoop = {

		syncUpdates: NoopRenderer.syncUpdates,

		isCommitting: () => isCommitting,

Restore DOM selection and suppress events #8353

Restore DOM selection and suppress events #8353

Conversation

sophiebits commented Nov 18, 2016 • edited

sebmarkbage commented Nov 19, 2016

sebmarkbage left a comment

Choose a reason for hiding this comment

sophiebits commented Nov 23, 2016

gaearon commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits commented Nov 23, 2016

sophiebits commented Nov 23, 2016

sebmarkbage commented Nov 23, 2016

sebmarkbage commented Nov 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits commented Nov 18, 2016 •

edited

sebmarkbage commented Nov 23, 2016 •

edited