Try: Manage focus through EditableProvider

[aduth]

This pull request seeks to explore options for removing the need for block implementers to manage focus state of a block, often by passing through focus and setFocus props to the Editable component.

Implementation notes:

It builds upon the EditableProvider introduced in #1943, which uses React context to communicate transparently between the editor infrastructure and Editable instances.

This is made more difficult because it requires bidirectional communication (Editable onFocus -> Editor, Editor focus -> Editable), and detecting changes in context values is not accounted in components implementing shouldComponentUpdate (facebook/react#2517). Taking cues from a project react-side-context, the workaround here is to create an event emitter in state which detects changes and emits a change event to incur a forced update.

In retrospect, I found that many blocks are making a conscious choice to use focus to determine whether block inspector or toolbar controls should be rendered. In most cases, they are primarily concerned that it is truthy ("is focussed"), and in a few cases are managing disparate focus states (quote focused in text vs. caption). The latter of these requires setFocus, whereas the former does not. In any case, there's not as much benefit to be gained here as I'd hoped, aside from simple cases where focus is managed outside the view of the block implementer. It could be argued that explicitness is actually favorable here, both to clarify actual behavior, and expose block implementers to available features.

This all said, I'm not sure if this is something we ought to move on, but thought I'd publish all the same to invite discussion on ideas of using context or other patterns to remove concerns from the responsibility of block implementers.

[codecov]

Codecov Report

Merging #2108 into master will increase coverage by 0.59%.
The diff coverage is 18.18%.

@@            Coverage Diff             @@
##           master    #2108      +/-   ##
==========================================
+ Coverage   20.34%   20.94%   +0.59%     
==========================================
  Files         135      136       +1     
  Lines        4237     4321      +84     
  Branches      722      735      +13     
==========================================
+ Hits          862      905      +43     
- Misses       2843     2873      +30     
- Partials      532      543      +11

Impacted Files	Coverage Δ
blocks/library/cover-text/index.js	30% <ø> (ø)	⬆️
blocks/editable/index.js	0% <ø> (-0.48%)	⬇️
blocks/library/preformatted/index.js	44.44% <ø> (ø)	⬆️
blocks/library/heading/index.js	16.12% <ø> (ø)	⬆️
blocks/library/button/index.js	26.66% <ø> (ø)	⬆️
blocks/library/more/index.js	30% <ø> (ø)	⬆️
blocks/library/verse/index.js	37.5% <ø> (ø)	⬆️
blocks/library/text/index.js	41.17% <ø> (ø)	⬆️
editor/index.js	0% <ø> (ø)	⬆️
blocks/library/table/index.js	36.36% <ø> (ø)	⬆️
... and 10 more

---

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 e514290...bbb05e6. Read the comment docs.

[youknowriad]

I was a bit sceptical about this but the fact that it's opt-in is a good thing IMO. If a block decides to explicitly handle the focus, it should opt-in (using a specific prop maybe).

I think if we follow this approach, we should try to avoid the necessity of the focus check before rendering the toolbars (Could be a CSS trick).

The event emitter bothers a bit though, it's hard to follow. I wonder if we can't replace it with connecting the Editable directly

[ellatrix]

To me, requiring blocks to use focus for something like the inspector feels strange. Will there ever be a case were the block should have something in the inspector when it is not focussed? IMHO, inspector and toolbar should be separate from the edit function.

This approach would also make it easier for #2296. In order to move the logic to editable, it needs to be aware of the previous and last block. With the EditableProvider I wouldn't have to pass these through all the block components.

[aduth]

@iseulde These are great points I totally overlooked. If we adopted something like this, it'd be possible in most cases for block implementers to just render the block controls and not worry about focus (also prevents from implementers forgetting to include this check).

Now, whether block controls are in fact separate from edit? I'm not sure, since they're certainly relevant only to the editing experience, not the block as a standalone thing. Where else might you imagine they live? As we had previously with a controls property?

[aduth]

A challenge here is knowing how to associate the Fill of individual blocks with the Slot for toolbar controls, since the slot is "global". One option might be to assign a unique name for the toolbar slot of each block (maybe using its uid), but this would require that the block's toolbar have awareness of the block ID. Is this passed by block implementers, or continue to abuse context to pass this information in implicitly?

[aduth]

Going to take a different approach here.