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

Block-type changes can affect adjacent sibling block #86

Closed
gscottolson opened this issue Feb 25, 2016 · 5 comments
Closed

Block-type changes can affect adjacent sibling block #86

gscottolson opened this issue Feb 25, 2016 · 5 comments
Assignees
Labels

Comments

@gscottolson
Copy link
Contributor

Repro:

  • Navigate to the Draft.js homepage
  • Add a few blocks of text
  • Triple-click a block of text
    • This will select the entire block
  • Change the block type to H1

Expected:

  • The block type for the selected line changes to a heading

Actual:

  • The block type for the selected line and the following line changes to a heading

triple-click

As I typed this bug with Github Markdown editor, I noticed it also demonstrates this behavior (triple-clicking a line and applying bold will drop the trailing ** at the beginning of the next line). Maybe this is out of the hands of Draft. However, I do believe that fixing the issue would lead to more reasonable UX.

@gscottolson
Copy link
Contributor Author

I should also add…

Killer work on this project, team. I have been exploring contentEditable-n-React solutions for a few weeks now. This project is a huge leap forward. Thanks for sharing it!

@hellendag
Copy link

Thanks @gscottolson!

I think the problem here is that the SelectionState is wrapping into the next line, and probably looks something like:

SelectionState {
  anchorKey: 'a',
  anchorOffset: 0,
  focusKey: 'b',
  focusOffset: 0,
  ...
}

One option would be to change getDraftEditorSelectionWithNodes to be more aware of this possibility and normalize the DOM selection to a SelectionState that just includes block a. Another would be to make block-type and inline-style changes not apply to the second block, within modifier code.

I'm thinking the first option might be a little cleaner, though it means that there would be no state distinction between selecting an entire line of text and selection the text plus the newline. Given that we pretty much ignore the newline as content, though, that might be fine.

What do you think?

@gscottolson
Copy link
Contributor Author

I think the first solution seems reasonable. Thanks for the quick reply.

FWIW, Facebook Notes does not exhibit this behavior. How is this selection case handled in that product?

Edit: You are right about Notes.

@hellendag hellendag self-assigned this Feb 26, 2016
@hellendag
Copy link

Looks like Notes does have this problem. :)

@hellendag hellendag added the bug label Feb 27, 2016
hellendag pushed a commit to hellendag/draft-js that referenced this issue Feb 27, 2016
Resolves facebookarchive#86.

In cases where the user triple-clicks on a block, then toggles the block type, it feels weird to have the trailing block also change block type.

The `SelectionState` resulting from the triple-click is correct, but to improve the interaction here, let's discard the trailing block for this modifier.

Test Plan:

View rich.html, add three lines of text.

- Triple-click first line, toggle H1. Verify that only the first line toggles.
- Repeat with second line, verify that only the second line toggles.
- Repeat with third line (no trailing block), verify that only the third line toggles.

Also select across multiple blocks by dragging, verify that toggling block types properly affects all visibly selected blocks.
@ghost ghost closed this as completed in b213d5b Feb 28, 2016
@gscottolson
Copy link
Contributor Author

Excellent. Thanks for the quick fix! 🎩

This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants