adds selection persistence on sidebar tab switch

[draganescu]

Description

Fixes #13309

When a block is selected or multi selected switching the settings sidebar to document deselects everything but switching back to block selects everything back.

How has this been tested?

There is an e2e test in the a11y suite.

Screenshots

Types of changes

Non breaking changes.
Added a new action in the core/block-editor store, restoreSelectedBlock.
Updated the blockSelection reducer of the core/block-editor store.

Checklist:

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

[draganescu]

@gziolo using the page.$ selection engine from puppeteer I couldn't get 'is-selected' no matter what I did, hence the eval solution in the test. Not sure why, but the selector was always null, even though visual inspection contradicted that.

The state update in this PR is generic enough, the only required implementation is to dispatch the new action whenever we want to return the previous selection after clearing.

[gziolo]

The state update in this PR is generic enough, the only required implementation is to dispatch the new action whenever we want to return the previous selection after clearing.

Yes, it looks very clean in the current shape. I like this proposal 👍
There is some more work required on this branch as noted in code comments. Travis isn't happy about the following as well:

1. There are failing unit tests with these changes applied:
   https://travis-ci.com/WordPress/gutenberg/jobs/199430106#L5455

I think it's expected that there are going to be actions where the previous selection will have to be cleared as well.

2. There are new public API methods added so you will have to run npm run docs:build to refresh docs.

3. There is one e2e test failing which looks completely unrelated:
   https://travis-ci.com/WordPress/gutenberg/jobs/199430115#L1092

FAIL packages/e2e-tests/specs/editor-modes.test.js (11.732s)
  ● Editing modes (visual/HTML) › the code editor should unselect blocks and disable the inserter
    expect(received).not.toBeNull()
    Received: null
      112 | 		await page.click( '.edit-post-sidebar__panel-tab[data-label="Block"]' );
      113 | 		const noBlocksElement = await page.$( '.block-editor-block-inspector__no-blocks' );
    > 114 | 		expect( noBlocksElement ).not.toBeNull();
          | 		                              ^
      115 | 
      116 | 		// The inserter is disabled
      117 | 		const disabledInserter = await page.$( '.block-editor-inserter > button:disabled' );
      at toBeNull (specs/editor-modes.test.js:114:33)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:45:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:271:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:97:21)
      at asyncGeneratorStep (specs/editor-modes.test.js:11:103)
      at _next (specs/editor-modes.test.js:13:194)

[draganescu]

fixed two tests, made a new one fail! yay! :)

[draganescu]

I had to add a way to clear previousSelection destructively, that is without saving previous selection, so that restoreBlock wouldn't find anything to restore when invoked. The case was needed when selecting text mode, and called restoreBlock it looked in the UI as if there were some blocks selected, even though none existed.

[draganescu]

we should wipe the selection when the blocks are intentionally unselected and write a test about that.

[draganescu]

I have added a wipeBlockSelection as a way to destructively clear block selection and called it on visual/text editor mode switch and also when all blocks are manually deselected.

[gziolo]

I will retest later today, code wise it looks good 👍

[draganescu]

@gziolo everything should be up to date and conform to the requested changes :)

[gziolo]

Thank you for addressing this issue. This is a really great improvement! Code looks great 💯

[aduth]

Coming to this late, but a few brief observations in passing:

• Why should changing between "Document" and "Blocks" have any impact on selection, either visually or in data? From what I can discern from the animation, it seems we still visually show the blocks as deselected when using the "Document" tab. I'm unsure if this is what motivated some of the new implementation.
• It wasn't immediately obvious to me what the new terminology meant with "wipe" a selection. If the idea is that clearing a selection can be reverted, I wonder if we could have instead done this by some alternative of (a) using/modifying state history (e.g. withHistory), (b) using redux-optimist (leveraged also in editor's reducer.js) to have this apply as a commit/revert operation, or (c) just as an argument of the existing clearSelectedBlock to reflect this effect.
  • Personally I think we should be wary to introduce new actions when existing ones can serve just as well with minor (sensible) revisions
  • Depending on the answer to my first point, I wonder if we'd need any of this at all
• I see a warning now when viewing the editor with SCRIPT_DEBUG. I assume this prop is being passed down somewhere it's not meant to be:

react-dom.165d5c53.js:500 Warning: React does not recognize the `wipeSelectedBlock` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `wipeselectedblock` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
    in div (created by BlockSelectionClearer)
    in BlockSelectionClearer (created by WithDispatch(BlockSelectionClearer))
    in WithDispatch(BlockSelectionClearer)
    in Unknown (created by WithSelect(WithDispatch(BlockSelectionClearer)))
    in WithSelect(WithDispatch(BlockSelectionClearer)) (created by VisualEditor)
    in VisualEditor (created by Layout)
    in div (created by Layout)
    in div (created by FocusReturnProvider)
    in FocusReturnProvider (created by Layout)
    in Layout
    in Unknown (created by WithViewportMatch(Layout))
    in WithViewportMatch(Layout) (created by NavigateRegions(WithViewportMatch(Layout)))
    in div (created by NavigateRegions(WithViewportMatch(Layout)))
    in NavigateRegions(WithViewportMatch(Layout)) (created by WithDispatch(NavigateRegions(WithViewportMatch(Layout))))
    in WithDispatch(NavigateRegions(WithViewportMatch(Layout)))
    in Unknown (created by WithSelect(WithDispatch(NavigateRegions(WithViewportMatch(Layout)))))
    in WithSelect(WithDispatch(NavigateRegions(WithViewportMatch(Layout)))) (created by Editor)
    in ErrorBoundary (created by Editor)
    in div (created by DropZoneProvider)
    in DropZoneProvider (created by BlockEditorProvider)
    in SlotFillProvider (created by BlockEditorProvider)
    in BlockEditorProvider (created by WithDispatch(BlockEditorProvider))
    in WithDispatch(BlockEditorProvider)
    in Unknown (created by Context.Consumer)
    in WithRegistryProvider(WithDispatch(BlockEditorProvider)) (created by EditorProvider)
    in EditorProvider (created by WithDispatch(EditorProvider))
    in WithDispatch(EditorProvider)
    in Unknown (created by WithSelect(WithDispatch(EditorProvider)))
    in WithSelect(WithDispatch(EditorProvider)) (created by Editor)
    in StrictMode (created by Editor)
    in Editor
    in Unknown (created by WithSelect(Editor))
    in WithSelect(Editor)

[draganescu]

oh boy :) so many things.

First the decision to deselect blocks when using the document inspector is an older design decision a while ago .

Second, I had no idea about redux-optimist, I was trying to solve the issue using a straight forward approach: make the thing reversible :O) by storing it in a place.

Indeed, clear vs wipe, maybe it's not the best :) Naming is hard. Initially I wanted to pass an object as an argument to clearSelectedBlock like { destructively: true } just to make it clearer.

I will create an issue for the error on SCRIPT_DEBUG. I think this should default to ON in the dev Gutenberg docker image?

[aduth]

First the decision to deselect blocks when using the document inspector is an older design decision a while ago .

Thanks for sharing the context. I hadn't seen @jasmussen 's comment. It helps frame this pair of tabs as something of being mutually exclusive to there being / not being a selection of blocks, which I wasn't aware of.

Aside from this point, one of the motivators in my prior comment is in limiting what we add to our own maintenance burden if existing concepts could apply. withHistory and redux-optimist may or may not work well for this, but conceptually it seemed like the idea proposed here aligned with some behavior of a reversal of some prior action (the deselection). Ultimately, going this route may prove to add more complexity than what you've proposed, so I don't mean for it to come across as a challenge, but rather as constructive contemplation. Similarly, if we could account for this behavior in the existing clearSelectedBlock or by just changing the UI visually (while retaining the selection state), there could be some middle ground. I was prompted to make my comment because it wasn't entirely clear to me what was meant by "wipe", at least in contrast with the existing "clear" mechanic. I expect a future maintainer may have similar difficulty in discerning them.

I will create an issue for the error on SCRIPT_DEBUG. I think this should default to ON in the dev Gutenberg docker image?

I noticed after making my comment that @afercia had commented similarly at #13309 (comment) . Since this issue has been reopened, it may be enough to use that one for tracking?

I thought we did have SCRIPT_DEBUG as the default for the Docker development environment (#14371). It isn't yet enabled for end-to-end testing, since there are a number of existing React warnings (fewer as of #11360) which can trip the global error capturing. The task to enable this is tracked at #14845.

[jasmussen]

Thanks for sharing the context. I hadn't seen @jasmussen 's comment. It helps frame this pair of tabs as something of being mutually exclusive to there being / not being a selection of blocks, which I wasn't aware of.

Currently you can customize the document loaded, and you can customize each block in that document. One of the things that may be explored in the future, is to allow you to customize also the site itself (i.e. Site | Document | Block) as tabs. The argument made in the past, and thanks for considering it, is that by leveraging breadcrumbs instead of tabs, we make the hierarchy clear (Site → Document → Block). It also has the benefit that breadcrumbs can help you indicate this hierarchy even further (Site → Document → Columns → Column → Quote → Paragraph), and finally that both in cases of translation length and for the longer breadcrumb, it can wrap to two lines, whereas tabs can't. But at the same time, it is critical to such breadcrumbs that they are directly tied to what is selected. If you have a block selected and click the "Site" breadcrumb, that's what should get selected.

[youknowriad]

Based on the fact that there are some technical uncertainties and that the original issue is not really fixed by that PR, I'm wondering if we should revert this for this release and give this more time to mature and address these concerns.