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: Consider horizontal handled by stopPropagation in RichText #6712

Merged
merged 6 commits into from Jun 29, 2018

Conversation

Projects
None yet
2 participants
@aduth
Member

aduth commented May 11, 2018

Extracted from #6467
Fixes #5095

This pull request seeks to stop propagation of horizontal arrow key down events when it's inferred that this is to be handled by TinyMCE in traversing out of an inline boundary. This avoids an issue where TinyMCE moves the caret, then the WritingFlow proceeds to consider the arrow navigation, leading to it being impossible to arrow-navigate out of an inline boundary at the end of a paragraph (since TinyMCE moves the caret out, then WritingFlow moves the caret to the next block).

Testing instructions:

Repeat steps to reproduce from #5095, noting that you can escape out of the italicized text by pressing ArrowRight at the end of the paragraph.

@tofumatt tofumatt self-requested a review Jun 28, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 28, 2018

I'm taking a look at this and fixing the merge conflict, so I'll be pushing a rebased branch in a bit.

@tofumatt

I tested this locally and it works great. I updated the code to use the keycodes package and fixed the merge conflict. If you're happy with my changes I say :shipit:

2018-06-28 17 34 26

// [WORKAROUND]: When in a new paragraph in a new inline boundary node,
// while typing the zero-width space occurs as the first child instead
// of at the end of the inline boundary where the caret is. This should

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

I don't quite follow the sentence "This should only be exempt when focusNode is not only the ZWSP, which occurs when caret is placed on the right outside edge of inline boundary."

Could you rephrase it to explain what this is saying? I don't quite follow.

This comment has been minimized.

@aduth

aduth Jun 28, 2018

Member

Heh, I was reading this again earlier and was having a hard time following my own explanation, so yes, I definitely agree it could do for some rephrasing.

This comment has been minimized.

@aduth

aduth Jun 28, 2018

Member

Aside: Created issue for this in TinyMCE at tinymce/tinymce#4472

@aduth

This comment has been minimized.

Member

aduth commented Jun 28, 2018

Thanks for the review and updates @tofumatt . I'm not sure I agree with moving the ZWSP constant to the keycodes module, for two reasons:

  • It isn't a key code, it's a character
  • The keycodes module should be generic and not have any built-in awareness of dependencies like TinyMCE

That said, this sadly isn't the only occasion where we reference the value, so I can certainly appreciate the desire to have a single source of truth for it. Internally, TinyMCE does expose some utilities, though in scouring their public API I don't think it is made available for us to consume.

Alas, my intuition is to move this back into RichText, though in general I am very motivated to try to consolidate / simplify these "cleaning" behaviors (see also #6467, #7583).

Thoughts?

In the meantime, I also want to add a simple end-to-end test here. I should be able to salvage something from #6467.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 28, 2018

Oh, fair enough on it not really being a keycode. RichText works for me.

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

Updates:

  • Moved constant back to RichText in a79442b
  • Clarified the workaround comment in 48da235
  • Added an end-to-end test for inline boundary traversal in 81a1b7d

My only remaining hesitation is a reluctance against stopping propagation. For example, this could inadvertently prevent ObserveTyping from doing its thing. A few alternatives I explored (unsuccessfully):

  • Adding a property to the event object to flag it as specifically handling a horizontal navigation, which is then inspected by WritingFlow. This requires a shared knowledge of the property between both components, and further doesn't work, since there's an incompatibility between TinyMCE events / DOM events / React events where the assigned property is lost by the time it reaches WritingFlow.
  • Letting TinyMCE's horizontal navigation take effect before determining whether caret has moved. This is more promising, though based on the order of events firing, might not be possible for us in KeyDown and would require binding to an event like KeyUp instead, where there'd be a noticeable and undesirable (unnatural) delay for the user in traversing from one paragraph to the next.
@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 29, 2018

From what you've written here, the second option sounds less complex and easier to implement. I'd be curious to see how "unnatural" the delay in changing paragraphs was–maybe it's not so bad? Or maybe it's quite bad and not worth pursuing. 🤷‍♂️

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

I was so very close to an amazingly simple solution, which would be to rely on TinyMCE preventing the default on the event when it's handled internally, which could be inspected from WritingFlow to determine whether a transition should be aborted early.

https://github.com/tinymce/tinymce/blob/0f7a0f12667bde6eae9377b50b797f4479aa1ac7/src/core/main/ts/keyboard/ArrowKeys.ts#L38

However, this seems to not work in all cases, namely at the outside edge of an inline boundary, where it falsely reports as handling an ArrowRight. Reported issue at tinymce/tinymce#4476.

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

In retrospect though, I like the idea of using preventDefault as the indicator of whether WritingFlow should abort its normal flow, rather than being so forceful with stopPropagation in allowing it to see the event at all. It'll be challenging to get it to work this way with TinyMCE's false reporting though. Continuing to investigate...

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

Ultimately I landed at a solution which is like the first idea in #6712 (comment) , and with full expectation that eventually we could drop all of this and rely on Event#defaultPrevented. The solution required binding to the native DOM document, rather than through TinyMCE's dispatched event, so that the property could be assigned.

@aduth aduth merged commit 16edac4 into master Jun 29, 2018

2 checks passed

codecov/project 47.01% (-0.09%) compared to 9eb1e8f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/5095-horizontal-navigation-propagation branch Jun 29, 2018

@aduth

This comment has been minimized.

Member

aduth commented Jun 29, 2018

Oof, I forgot to account for the fact that this.editor.dom.doc in the context of our inline editor is the entire page document, not just the scope of the editor node. I'll have to fix this up, as I expect it could lead to either unintended behaviors or at least wasted effort in monitoring events. It also needs to be unbound when unmounted. 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment