Block Editor: Add Toolbar visibility to store

[WunderBart]

Description

Allow the Toolbar visibility to be managed via the Block Editor store.

Added actions:

• hideToolbar()
• showToolbar()

Added reducers:

• isToolbarVisible()

Added selectors:

• isToolbarVisible()

How has this been tested?

• Add any block to the editor
• Focus it so the Toolbar is visible
• Go to JS console & run:
  • window.wp.data.dispatch('core/block-editor').hideToolbar() to hide the toolbar and
  • window.wp.data.dispatch('core/block-editor').showToolbar() to bring it back.

Screenshots

Visible state:

Hidden state:

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[getdave]

Nice job!

I tested this using your instructions and it works well.

I'd probably look for a confirmatory review from @ellatrix as she has recently refactored some of this logic.

Specifically, I'd be concerned about the precedence here. I know there is some forceToolbar logic. I assume it would be ok for your hidden state to be overridden by the force but I'd like to be sure.

If this goes ahead you should write some unit tests for the store changes and update any e2e tests if they exist.

[getdave]

Minor: any reason you chose hidden? I wonder whether it should be inverted isToolbarVisible because to be visible is probably the natural state of most things (both in code and in the real world) so it feels more natural to express it that way.

[WunderBart]

Sounds good to me. I chose this convention probably because explicitly hiding the Toolbar was what I was after. Inverting it makes total sense.

[WunderBart]

I actually have second thoughts about this. It's because by calling hideToolbar() we want to be sure it remains hidden, but do we want to do the exact opposite for showToolbar()? I mean showing the Toolbar in this case means just unhiding it and letting its visibility depend on other factors, like e.g. if the block is focused (show) or not (hide). So, checking if isToolbarVisible would be kinda unreliable for the true value because we don't force it as we do when false is returned.

TL;DR

isToolbarVisible:

• true: Maybe it is, maybe it isn't. It depends on other factors 🤷‍♂️
• false: It definitely is hidden.

Vs.

isToolbarHidden:

• true: It definitely is hidden.
• false: It's not hidden, which doesn't mean it's visible - check other factors.

Having said that, I'm wondering if we shouldn't use just one action for that: hideToolbar(true/false). Calling with true (default) would set the isHidden flag to true and the opposite for calling with false. What do you think?

[getdave]

isToolbarHidden

I take it back. Now you outline the other factors isToolbarHidden seems more logical because we can say "it is hidden" when true but we are not saying "it is definietely visible" when `false. Keep as is.

Having said that, I'm wondering if we shouldn't use just one action for that: hideToolbar(true/false). Calling with true (default) would set the isHidden flag to true and the opposite for calling with false. What do you think?

Not in favour of this. hideToolbar(false) feels a little cryptic. What does it do? Show the toolbar? Hide the toolbar and trigger some other unknown behaviour? I'd avoid it personally.

[WunderBart]

hideToolbar(false) feels a little cryptic

True. It's just that calling showToolbar() on the other hand sounds misleading. I was also thinking about unhideToolbar() which IMO would mean exactly what it would do, which is unset the hidden flag. How does that sound?

[ellatrix]

What will it be used for? Can it be added together with the feature?

[ellatrix]

There's already some existing state in BlockListBlock that manages some toolbar visibility, and also typing state in the store which is meant for the block toolbar to hide and show. Maybe this could be merged with the typing state? Cc @youknowriad.

[getdave]

@ellatrix It's related to this Issue. This PR is required because there was no programmatic means of hiding the Block Toolbar.

We appreciate in usual circumstances hiding toolbars on Blocks is non-standard but consider:

• Design team feel that Nav UI is overwhelming on first opening Nav Block.
• The LinkControl UI is what actually has focus and so the Nav Link Block shouldn't necessarily display the Block Toolbar as it's text not directly editing until the LinkControl is closed.

@WunderBart will no doubt provide further detail.

typing state in the store which is meant for the block toolbar to hide and show.

I understand and @WunderBart has considered this already. However, I was concerned about this because the fact that startTyping causes the Block UI to disappear feels somewhat incidental. What's to say that in the future the Design team don't decide the UX needs to change and the Block toolbar should still display when typing. This is why I suggested @WunderBart take a more explicit approach. There is no ambiguity in show/hide and it's unlikely that this will accidentally change and cause a regression. As always happy to be told I'm completely wrong 😄

[WunderBart]

Thanks for the quick response on this, @getdave. There's not much I can add, actually. Just one thing:

Specifically, I'd be concerned about the precedence here. I know there is some forceToolbar logic. I assume it would be ok for your hidden state to be overridden by the force but I'd like to be sure.

The Toolbar visibility is determined in the Toolbar component itself, so it has precedence over the forced state, which is determined locally in the BlockList component. I can see now that it would be better to place the Toolbar visibility logic inside the BlockList component though, so I will move it there.

As for the isToolbarForced flag precedence - do you think we could rename it to isToolbarFocused, @ellatrix? I can see it's set to true on the core/block-editor/focus-toolbar action, so it wouldn't look weird if the isToolbarVisible flag had precedence over it, which I think would be the right way to do it.

Edit:

I can see the shouldShowContextualToolbar already has precedence over isToolbarForced (the canFocusHiddenToolbar flag), but I'm not sure I understand the implementation. shouldShowContextualToolbar is used before it's defined so it's always returning undefined at that point. Do you know if that is intended, @ellatrix?

[youknowriad]

I'm really not certain about this API personally. This is going to expose more UI related elements as API. Ideally, the editor should be smart enough to show the UI when needed and remove it as needed automatically.

Worth noting the effort done these days related to the new design #19344 where there's difference in how the block selected state work.

My question is what's the reasoning behind the need to explicitly hide the toolbar when the link control is shown? Why is it important? and how is this specific to the Navigation block? If we can answer these questions, we may find a way to solve this in a generic way.

[gziolo]

@ellatrix and @youknowriad - whatever it's going to be decided here should align with how we end up handling the focus after applying changes in the toolbar for other attributes and format controls when available. If the focus stays in the toolbar then we shouldn't hide this toolbar as users might want to close the popover and continue their work in the toolbar. It's particularly important to have predictable behavior for users who depend on assistive technologies like screen-readers as they would have to learn every possible workflow to toolbar action buttons.

[WunderBart]

Thanks for the input @youknowriad & @gziolo.

My question is what's the reasoning behind the need to explicitly hide the toolbar when the link control is shown? Why is it important? and how is this specific to the Navigation block?

This PR is related to the #18315. The aim there is to reduce the cognitive load when editing a Navigation Link by hiding the Toolbar before typing starts. When the popup opens, it automatically focuses its search input, which makes the Toolbar a distraction at that point. In #19531, I tried to resolve the issue by calling the startTyping() editor action explicitly when the Link popup opens, but that solution was kinda pointless as the Toolbar keeps showing back before the actual typing starts anyway.

So we went a bit further and decided to keep the Toolbar hidden until the Link popup is closed, which I think presents a better UX as the Link's popup is an independent interface and is out of the Toolbar controls scope (this should also address @gziolo's comment about users that "might want to close the popover and continue their work in the toolbar").

The whole deal here is to implement a way to control the Toolbar visibility explicitly to be able to implement the described enhancement, as I believe there's no generic way to do it yet (but I might be wrong here as I've just begun working with Gutenberg).

If that all makes sense and considering we still want to make the #18315 happen, do you think we're going in the right direction implementation-wise here?

[getdave]

If adding this new prop isn't they way forward because it exposes UI related items as part of the API, then how might we best proceed?

Abusing startTyping just to get the Block toolbar to disappear seems a lot like a hack and could easily cause a regression at any time.

[WunderBart]

the editor should be smart enough to show the UI when needed and remove it as needed automatically.

It makes sense to me, @youknowriad. I can see how exposing Toolbar API like this could go the wrong way. Because of that I'm closing this PR and holding off the work on #18315 (and keeping an eye on the #19344).