Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writing Flow: Fix all the broken flows #5513

Merged
merged 8 commits into from
Mar 9, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 8, 2018

This pull request seeks to remedy a number of issues with writing flow, some of which have existed for some time, some of which have been introduced (blaming myself) in recent refactorings. This includes...

  • Inserting a block should only shift focus to a text field, otherwise focusing the block's "focus stop"
  • Pressing backspace or enter from the block focus stop should respectively delete or insert a subsequent paragraph block
  • Pressing escape in a block should do nothing
  • Pressing arrow down from a non-text-field should proceed with tab transition as expected.
    • Example: Add an image placeholder block and some text blocks following it. Arrow back to the image block. Once black border appears, press tab so focus is placed at the "Upload" button. Then press down. The text caret should be in the next text block (in master, it skips to the end of the block list).
    • This has never worked correctly.
  • Multiselection at last text field in a block now accounts for non-contenteditable text fields
    • Not aware of any existing blocks which are structured this way, but there was an inconsistency between how we treated arrow tab transitions and the multi-selection shift-to-expand-only-at-last-available-field-in-block
  • Better identification of text fields for writing flow transitions
    • Previously, if a block contained a checkbox, radio, or other non-text input tags, they would be erroneously included in the writing flow sequence.
    • This has never worked correctly.

Implementation notes:

I'm hopeful the changes here provide more stability by consolidating like behaviors and isolating typing behavior outside the block itself.

Managing isTyping was moved to a separate component, largely because there was conflicts with IgnoreNestedEvents when both the block's inner and outer wrappers listen on the same block. I had started to explore providing a label to identify block handled events per block UID, but instead decided to refactor the typing logic altogether. The intention here is that isTyping is a global behavior, not specific to individual blocks (especially as we'd previously had handling re-setting isTyping after a separate block becomes selected after merge/split, and had separately handled logic of "is typing within block").

Because I'm guilty of causing much of these regressions in refactoring, I am committing myself to writing end-to-end tests for these behaviors.

Testing instructions:

Verify that all the writing flows work correctly. Notably all the examples above plus:

  • Inserting paragraph block (quote, etc; those with text fields) via autocomplete moves focus to cursor
  • Inserting image block (or others without text fields) via autocomplete causes focus to stay at block wrapper with black border
  • Shift-arrow from an text field incurs multiselection, but not if there are other text fields in the intended direction in the same block
    • Shift-up from quote's text causes multi-selection upward
    • Shift-down from quote's text causes caret to move into citation
    • Shift-down from quote's citation causes multi-selection downward
  • Block splitting and merging moves caret to correct position, and causes isTyping flag to be set if not arleady
  • Block controls disappear when typing (either input, arrow keys, enter/backspace/delete)
    • Block controls reappear when mouse moved or shift-selection (uncollapsed selection)

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Mar 8, 2018
@aduth aduth added this to the 2.4 milestone Mar 8, 2018
@aduth aduth requested a review from jasmussen March 8, 2018 22:10
@aduth aduth force-pushed the fix/tabbable-writing-flow branch from 4391553 to 9425112 Compare March 9, 2018 03:00
Escape doesn't clear focus, so causes problems that block is not selected but retains focus (since isSelected state is synced with focus)
Previously only closed on esc when editing link, not adding new link

TODO: Consolidate editing state
@jasmussen
Copy link
Contributor

Thank you so much for working on this. This is a 🔥PR and a masterclass in code. 👏

Inserting a block should only shift focus to a text field, otherwise focusing the block's "focus stop"
Example: Inserting image should focus the top-level placeholder and show a black border

Works really well 🌟

Pressing backspace or from the block focus stop should respectively delete or insert a subsequent paragraph block
Example: Pressing enter or delete on an image placeholder

🌟

Pressing escape in a block should do nothing
Arguable whether this is the desired behavior, but it is consistent with native inputs. Master is in a worse state because the block becomes deselected but the text caret is not removed, causing issues because the block never again becomes selected (because isSelected is synced to its focus event, but focus has never left).

🌟

Agree that it's unclear what is the ideal behavior here. It sort of conflicts with #4382 (comment), but there isn't wide consensus on the approach outlined there, so we can always revisit if and when that becomes necessary.

Pressing arrow down from a non-text-field should proceed with tab transition as expected.
Example: Add an image placeholder block and some text blocks following it. Arrow back to the image block. Once black border appears, press tab so focus is placed at the "Upload" button. Then press down. The text caret should be in the next text block (in master, it skips to the end of the block list).

🌟

Multiselection at last text field in a block now accounts for non-contenteditable text fields
Not aware of any existing blocks which are structured this way, but there was an inconsistency between how we treated arrow tab transitions and the multi-selection shift-to-expand-only-at-last-available-field-in-block

🌟

Better identification of text fields for writing flow transitions
Previously, if a block contained a checkbox, radio, or other non-text input tags, they would be erroneously included in the writing flow sequence.

🌟 🔥

Inserting paragraph block (quote, etc; those with text fields) via autocomplete moves focus to cursor

🌟

Inserting image block (or others without text fields) via autocomplete causes focus to stay at block wrapper with black border

🌟

Shift-arrow from an text field incurs multiselection, but not if there are other text fields in the intended direction in the same block

🌟 CC: @iseulde

I think this is desired behavior, it feels right, but I feel like Ella and I discussed in the past what happened if you were in a multi input block, held shift, and arrowed. Can't recall the full details.

Shift-up from quote's text causes multi-selection upward
Shift-down from quote's text causes caret to move into citation
Shift-down from quote's citation causes multi-selection downward

🌟🌟🌟

Block splitting and merging moves caret to correct position, and causes isTyping flag to be set if not arleady

🌟🔥🚒

Block controls disappear when typing (either input, arrow keys, enter/backspace/delete)
Block controls reappear when mouse moved or shift-selection (uncollapsed selection)

🌟

Amazing work! This is suuuch a vast improvement to the writing flow. Really really dig this. 👍 👍

@aduth
Copy link
Member Author

aduth commented Mar 9, 2018

Thanks for the glowing review @jasmussen ! And for running through all the steps.

It's a risky move forward with the code changes, but I want this one to have some time to sit in master to let any issues surface before the pending release, so I'm going to go ahead and merge.

@aduth aduth merged commit 07f6ec6 into master Mar 9, 2018
@aduth aduth deleted the fix/tabbable-writing-flow branch March 9, 2018 21:10
@ellatrix
Copy link
Member

I think this is desired behavior, it feels right, but I feel like Ella and I discussed in the past what happened if you were in a multi input block, held shift, and arrowed. Can't recall the full details.

Yes, this is still a to-do. See #3232.


Thanks for all the work on this @aduth. I've been testing shift-arrow-multi-selection a bit, and it seems broken if you go in one direction for a while, and then the other. Not sure if this ever worked as expected but I think it did. I'm expecting shift+arrow in the opposite direction to deselect the blocks again one at the time. I'll see if I can fix this.

@aduth
Copy link
Member Author

aduth commented Mar 10, 2018

I've been testing shift-arrow-multi-selection a bit, and it seems broken if you go in one direction for a while, and then the other.

Yes, this was identified as a "Known bug" of the nesting implementation (#3745 (comment), #3745 (comment)), but good to have an explicit issue for it (#5548). Thanks!

@mcsf
Copy link
Contributor

mcsf commented Mar 13, 2018

This is awesome.

*/
toggleEventBindings( isBound ) {
const bindFn = isBound ? 'addEventListener' : 'removeEventListener';
document[ bindFn ]( 'selectionchange', this.stopTypingOnSelectionUncollapse );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to recall if there was a specific reason to make this bound to the document, vs. just an onSelectionChange event on the div wrapper. It was inherited from how this was applied previously, originally from #4872, where the selection handling moved from RichText to BlockListBlock.

The document-based mousemove event makes sense because we want to unset typing even when the mouse is moved outside the descendents of this component. But selectionchange? I'm not sure. That seems inconsistent with the rest of the key handling that happens here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this would probably explain why:

facebook/react#5785

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants