FIX: After saving, ability to save isn't available until you exit a block

[tg-ephox]

fixes #1240

Throttle TinyMCE Change event so that changes are captured as soon as made.

[codecov]

Codecov Report

Merging #2418 into master will decrease coverage by <.01%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #2418      +/-   ##
==========================================
- Coverage   31.38%   31.38%   -0.01%     
==========================================
  Files         177      177              
  Lines        5413     5420       +7     
  Branches      949      949              
==========================================
+ Hits         1699     1701       +2     
- Misses       3139     3144       +5     
  Partials      575      575

Impacted Files	Coverage Δ
blocks/editable/index.js	11.02% <28.57%> (+0.49%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78dd8b6...269800e. Read the comment docs.

[youknowriad]

I was expecting more troubles to be introduced by this :). It works reasonably well but I'm seeing some "dirty detection" issues: After saving a draft, the "Save draft" link remains visible even if the post is not dirty anymore.

[aduth]

To clarify, Change is not expected to fire on every individual change (i.e. keystroke) but rather on what I presume is changes to the Undo history?

Demo: http://fiddle.tinymce.com/GXfaab

[tg-ephox]

@youknowriad I tried to replicate this but was unable to. When does this happen for you? Are you clicking the 'Save draft' link or when the auto save runs? I can see that if one was to click 'Save draft' less than 500ms after typing this could occur...

@aduth I was under the impression that the Change event works like this also however when I console.log in the handler I get every key stroke... This might be a peculiarity of how TinyMCE works in inline mode?

[nitrajka]

Hi,

I found the case when "Save draft" button remains visible but shouldn't. (in branch try/1240-improve-block-saving)
Here are steps to reproduce:

1. write something
2. save it
3. delete the whole text, you've written (not only a part of it because in that case, it behaves well) and don't click anywhere (stay with the cursor in the text)
4. Notice that "Saved" button still remains visible, but shouldn't

@aduth Could we add an 'onKeyPress' event listener instead?

[aduth]

I'm a little wary of the performance impact if we start to keep TinyMCE content more strongly in sync with editor state, since it requires that we call the block parser (correction: nodeListToReact) on every keypress, a very non-trivial task.

[tg-ephox]

@nitrajka after investigating this looks like it could be a bug TinyMCE UndoManager (doesn't fire the Change event if block starts empty!). Have changed to keyup event.
@aduth is the 500ms timeout going to cause performance issues or should we look a different way to achieve fix?

[aduth]

A throttle would be an improvement, yes. It's still potentially invoked very often, but I think ultimately we'll want some some change events firing while the block has focus. This can also help when we start to store local unsaved copies of the post for recovery (see #2410). Whether that needs to be every half second vs... maybe even every 10 seconds, I'm not sure the user cares as much? Especially if we're firing on the leading edge which is likely to flag the change immediately.

In any case, I'm on board with the direction and the implementation seems sound 👍

[tg-ephox]

@aduth I think this is as good as it gets without making a change to 'pull' content from blocks. The problem with setting the throttle to 10 seconds is that if i start typing then click 'Save Draft' it takes the remainder of the 10 seconds from first typing for the post to be marked dirty again...

[aduth]

The problem with setting the throttle to 10 seconds is that if i start typing then click 'Save Draft' it takes the remainder of the 10 seconds from first typing for the post to be marked dirty again...

This is a good point to raise, and one which we should consider even for a half-second delay. When the user blurs the Editable, should we be canceling any delayed saves, since the save will occur immediately upon blur? Or does it not really matter since the content would presumably be the same? How is this any different between a 0.5 second and 10 second delay?

[tg-ephox]

@aduth have changed this to a 10100ms delay it needs to be > 10000ms as auto save uses this and we need to trigger after auto save. One problem I did find is that if the UPDATE_BLOCK_ATTRIBUTES action is fired during the save operation the post wont be marked dirty as the RESET_POST action clears the history.past state key. This could obviously happen with the 500ms throttling but it will be corrected quickly... This will also be corrected when focus is lost from the TinyMCE instance and the focusout event is fired.

[aduth]

I've been thinking about this some more, particularly as it relates to the value of Editable / children (#771 and related work at #2463). In its current form, the cost of creating a new editable value is the effort it takes to convert the DOM tree to a tree of React elements. There might be some overhead here, less-so if we move toward simpler data structures like the array structures discussed in #771 and #2463.

One thing I'd not considered previously which is worth noting is that frequent changes were important to avoid when using a single TinyMCE instance for an entire post, because it wasn't unrealistic to expect the DOM of the editor to become very very large, and therefore very non-performant to frequently walk. Since we made the conscious decision to create many smaller instances of TinyMCE in Gutenberg, this is not as much of a problem, because recalculating the value only needs to walk a single, smaller instance of TinyMCE.

I toyed with a few proofs-of-concept to see how we might generate and represent content as an array:

• ...fully recalculating tree on any change: https://codepen.io/aduth/pen/brypKz
• ...updating subset of the value using MutationObserver: https://codepen.io/aduth/pen/ayrNew

As described in #2463, there's a few other challenges in our way to be able to represent the value this way, but if we were, I think it could be reasonable to even make change handling synchronous / immediate, which avoids some of the headaches around handling throttled handlers (tangentially, I spent my entire day yesterday debugging such issues 😩 ).

[tg-ephox]

@aduth I like the approach of having a model to represent content (not HTML) as you could throw this anywhere and have it render as intended. Also have some ideas around how to create non-react blocks. Will throw together a PoC when I have some time.

[adamsilverstein]

have changed this to a 10100ms delay it needs to be > 10000ms as auto save uses this and we need to trigger after auto save.

Please note, this will not be an issue when we enable heartbeat based autosaves. autosave dirty state is tracked separately from editor dirty state after #4218

[aduth]

Superseded by #4955