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

Commit d144883

Browse files
flarniefacebook-github-bot
authored andcommitted
try disabling 'blockSelectEvents' flag
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
1 parent 558352c commit d144883

File tree

3 files changed

+18
-1
lines changed

3 files changed

+18
-1
lines changed

src/component/base/DraftEditor.react.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,11 @@ class DraftEditor extends React.Component<DraftEditorProps, State> {
378378
* of browser interaction, not re-renders and forced selections.
379379
*/
380380
componentWillUpdate(nextProps: DraftEditorProps): void {
381-
this._blockSelectEvents = true;
381+
if (!gkx('draft_js_stop_blocking_select_events')) {
382+
// We suspect this is not actually needed with modern React
383+
// For people in the GK, we will skip setting this flag.
384+
this._blockSelectEvents = true;
385+
}
382386
this._latestEditorState = nextProps.editorState;
383387
}
384388

@@ -414,6 +418,7 @@ class DraftEditor extends React.Component<DraftEditorProps, State> {
414418
editorNode instanceof HTMLElement,
415419
'editorNode is not an HTMLElement',
416420
);
421+
417422
editorNode.focus();
418423

419424
// Restore scroll position

src/component/handlers/edit/editOnSelect.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import type DraftEditor from 'DraftEditor.react';
1717

18+
const DraftJsDebugLogging = require('DraftJsDebugLogging');
1819
var EditorState = require('EditorState');
1920
var ReactDOM = require('ReactDOM');
2021

@@ -26,6 +27,16 @@ function editOnSelect(editor: DraftEditor): void {
2627
editor._blockSelectEvents ||
2728
editor._latestEditorState !== editor.props.editorState
2829
) {
30+
if (editor._blockSelectEvents) {
31+
const editorState = editor.props.editorState;
32+
const selectionState = editorState.getSelection();
33+
DraftJsDebugLogging.logBlockedSelectionEvent({
34+
// For now I don't think we need any other info
35+
anonymizedDom: 'N/A',
36+
extraParams: '',
37+
selectionState: JSON.stringify(selectionState.toJS()),
38+
});
39+
}
2940
return;
3041
}
3142

src/stubs/DraftJsDebugLogging.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,5 +12,6 @@
1212
'use strict';
1313

1414
module.exports = {
15+
logBlockedSelectionEvent: () => null,
1516
logSelectionStateFailure: () => null,
1617
};

0 commit comments

Comments
 (0)