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

DOM Selections cleared when <select>ing outside editor #2123

Closed
holloway opened this issue Jul 8, 2019 · 2 comments
Closed

DOM Selections cleared when <select>ing outside editor #2123

holloway opened this issue Jul 8, 2019 · 2 comments
Assignees

Comments

@holloway
Copy link
Contributor

holloway commented Jul 8, 2019

Do you want to request a feature or report a bug?

Feature.

I'm happy to open a PR with this change however I first wanted to see if there was interest in this from the Draft team.

What is the current behavior?

DOM Selections are cleared on blur.

What is the expected behavior?

DOM Selections are maintained.

I hoped to find a config option to disable this behaviour but there was no none.

Which versions of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of Draft.js?

All versions.


In the Draft source there's this function and code comment,

function editOnBlur(editor, e) {
  // In a contentEditable element, when you select a range and then click
  // another active element, this does trigger a `blur` event but will not
  // remove the DOM selection from the contenteditable.
  // This is consistent across all browsers, but we prefer that the editor
  // behave like a textarea, where a `blur` event clears the DOM selection.
  // We therefore force the issue to be certain, checking whether the active
  // element is `body` to force it when blurring occurs within the window (as
  // opposed to clicking to another tab or window).
  if (getActiveElement() === document.body) {
    var _selection = global.getSelection();
    var editorNode = editor.editor;
    if (_selection.rangeCount === 1 && containsNode(editorNode, _selection.anchorNode) && containsNode(editorNode, _selection.focusNode)) {
      _selection.removeAllRanges();
    }
  }

In our web app we have a toolbar outside the <Editor> for formatting, and this toolbar has both <button>s and <select>s.

The <select>s are the problem in Draft.js because clicking them visibly deselects any text. Have a look at https://jsfiddle.net/y4Lszjv5/ and type some text, select it (while making sure that the selection would be visible with the native <select> expanded) and the selection will disappear while the <select> is expanded. This makes it harder to see which text would be formatted.

Note that Quill and most other editors don't have this problem as they leave the consistent browser behaviour as-is (as your code comment notes "This is consistent across all browsers").

So I guess there are two potential solutions,

  1. Offer a config prop to disable _selection.removeAllRanges();, or
  2. Handle <select> specifically and don't run _selection.removeAllRanges(); when focus has moved to that type of element.
@mrkev mrkev self-assigned this Jul 9, 2019
@mrkev
Copy link
Contributor

mrkev commented Jul 9, 2019

I think the best move here is to make this configurable. I can't promise I'll look into this right away since I'm quite busy at the moment (work and personal todos piling up 😅), but I'd be happy to either review a PR or look into this in a couple of weeks.

@claudiopro
Copy link
Contributor

I'm also fine with the proposal as long as we preserve the current behavior as default. This could also have significant accessibility impact so this change would have to be verified to be friendly to screen readers.

facebook-github-bot pushed a commit that referenced this issue Jul 16, 2019
Summary:
**Summary**

This adds a `preserveSelectionOnBlur` prop as per #2123

**Test Plan**

Tests were added for the two possible scenarios. I'm not sure how much to mock in this test because it is just testing a specific branch of code, so I've faked certain details like the rangeCount.

What do you think mrkev claudiopro ?
Pull Request resolved: #2128

Reviewed By: claudiopro

Differential Revision: D16270879

Pulled By: mrkev

fbshipit-source-id: 304af92c1211b8ff95741bff434b4fe3c4b6dd7d
@mrkev mrkev closed this as completed Jul 17, 2019
jdecked pushed a commit to twitter-forks/draft-js that referenced this issue Oct 9, 2019
Summary:
**Summary**

This adds a `preserveSelectionOnBlur` prop as per facebookarchive#2123

**Test Plan**

Tests were added for the two possible scenarios. I'm not sure how much to mock in this test because it is just testing a specific branch of code, so I've faked certain details like the rangeCount.

What do you think mrkev claudiopro ?
Pull Request resolved: facebookarchive#2128

Reviewed By: claudiopro

Differential Revision: D16270879

Pulled By: mrkev

fbshipit-source-id: 304af92c1211b8ff95741bff434b4fe3c4b6dd7d
mmissey pushed a commit to mmissey/draft-js that referenced this issue Mar 24, 2020
Summary:
**Summary**

This adds a `preserveSelectionOnBlur` prop as per facebookarchive#2123

**Test Plan**

Tests were added for the two possible scenarios. I'm not sure how much to mock in this test because it is just testing a specific branch of code, so I've faked certain details like the rangeCount.

What do you think mrkev claudiopro ?
Pull Request resolved: facebookarchive#2128

Reviewed By: claudiopro

Differential Revision: D16270879

Pulled By: mrkev

fbshipit-source-id: 304af92c1211b8ff95741bff434b4fe3c4b6dd7d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants