Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Commit

Permalink
try disabling 'blockSelectEvents' flag
Browse files Browse the repository at this point in the history
Summary:
In the past Draft.js set a 'blockSelectEvents' flag on 'componentWillUpdate'
and unset it in 'componentDidUpdate'.

The reason for this, according to comments in the code, was that IE will fire
a 'selectionChange' event when we programmatically change the selection,
meaning it would trigger a new select event while we are in the middle of
updating.

**Edit:**
We found that the 'selection.addRange' was what triggered the stray `selectionchange` event in IE, for the record.[1] Thanks sophiebits for the tip about that.

When manually testing, I saw no bugs or inconsistencies introduced when the extra 'selectionchange' event fired in IE. The event would go into React, and yet it was never logged as triggering a handler in Draft.js.

So either that `selectionchange` event was not one that Draft.js. listened to, or React ignores/defers it because it's in the middle of a commit.

This diff does two things:
- Add logging to verify if we ever actually use the `blockSelectEvents` flag in practice. Is it actually blocking anything??? And if not, as I suspect, then;
- We can just remove it. This also removes it under a GK, so we can test on 50% employees first and see if they experience any bugs.

Lastly - I do want to add tests for this. It is error-prone and time consuming to set up and repeately do manual testing. Going to put together an outline of what I think the test should look like in a follow-up diff.

 ---
[1]: Here are the logs I recorded, for posterity
```
// inside of DraftEditor#focus
calling forceSelection
calling udpate with new editor state and new selection
componentWillUpdate fired!
This is where blockSelectEvents would be set to true~~~
a leaf is about to call setDraftEditorSelection
removing all ranges from the selection
calling range.setStart
**calling selection.addRange**
**React got an event of type topSelectionChange**
**window got an event of 'selectionchange'**
componentDidUpdate fired
This is where blockSelectEvents would be set to false~~~
React got an event of type topBlur
React got an event of type topFocus
Draft got an event of onFocus
Draft returned early because currentSelection already matches the updated selection
```

Reviewed By: sophiebits

Differential Revision: D6866079

fbshipit-source-id: 01034c6404df892224f5c18a45cba744a64d6e38
  • Loading branch information
flarnie authored and facebook-github-bot committed Feb 5, 2018
1 parent 558352c commit d144883
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/component/base/DraftEditor.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,11 @@ class DraftEditor extends React.Component<DraftEditorProps, State> {
* of browser interaction, not re-renders and forced selections.
*/
componentWillUpdate(nextProps: DraftEditorProps): void {
this._blockSelectEvents = true;
if (!gkx('draft_js_stop_blocking_select_events')) {
// We suspect this is not actually needed with modern React
// For people in the GK, we will skip setting this flag.
this._blockSelectEvents = true;
}
this._latestEditorState = nextProps.editorState;
}

Expand Down Expand Up @@ -414,6 +418,7 @@ class DraftEditor extends React.Component<DraftEditorProps, State> {
editorNode instanceof HTMLElement,
'editorNode is not an HTMLElement',
);

editorNode.focus();

// Restore scroll position
Expand Down
11 changes: 11 additions & 0 deletions src/component/handlers/edit/editOnSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import type DraftEditor from 'DraftEditor.react';

const DraftJsDebugLogging = require('DraftJsDebugLogging');
var EditorState = require('EditorState');
var ReactDOM = require('ReactDOM');

Expand All @@ -26,6 +27,16 @@ function editOnSelect(editor: DraftEditor): void {
editor._blockSelectEvents ||
editor._latestEditorState !== editor.props.editorState
) {
if (editor._blockSelectEvents) {
const editorState = editor.props.editorState;
const selectionState = editorState.getSelection();
DraftJsDebugLogging.logBlockedSelectionEvent({
// For now I don't think we need any other info
anonymizedDom: 'N/A',
extraParams: '',
selectionState: JSON.stringify(selectionState.toJS()),
});
}
return;
}

Expand Down
1 change: 1 addition & 0 deletions src/stubs/DraftJsDebugLogging.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@
'use strict';

module.exports = {
logBlockedSelectionEvent: () => null,
logSelectionStateFailure: () => null,
};

0 comments on commit d144883

Please sign in to comment.