Skip to content

Prevent focus loss after removing a block from List View#39031

Merged
alexstine merged 8 commits into
WordPress:trunkfrom
delowardev:fix/list-view-focus-loss
Mar 15, 2022
Merged

Prevent focus loss after removing a block from List View#39031
alexstine merged 8 commits into
WordPress:trunkfrom
delowardev:fix/list-view-focus-loss

Conversation

@delowardev

@delowardev delowardev commented Feb 23, 2022

Copy link
Copy Markdown
Contributor

Fixes: #39026

Description

Removing a block from List View is supposed to select its previous block, but currently, it doesn't select any block after removing.

Testing Instructions

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@delowardev delowardev requested a review from ellatrix as a code owner February 23, 2022 17:06
@Mamaduka Mamaduka requested review from gwwar and talldan February 25, 2022 11:42
@alexstine

Copy link
Copy Markdown
Contributor

@delowardev Thanks for the PR.

Focus is still lost when I gave this a local test.

To reproduce:

  1. Insert some blocks.
  2. Open the list view.
  3. Select a block.
  4. Use the options menu to remove the block.
  5. Focus is placed at the bottom of the page and not in the block.

The block is likely getting selected but focus is not changing. Can you look in to it?

Thanks.

@delowardev

delowardev commented Feb 25, 2022

Copy link
Copy Markdown
Contributor Author

@alexstine thanks so much for the feedback.

The original goal of this PR is to select the previous block after the selected block gets removed;
Here focus looks working for me, after removing a block when the previous block gets selected, it focuses on that block too. (Please check the attached video)

Thanks.

focus-loss.mov

@alexstine

Copy link
Copy Markdown
Contributor

@delowardev Sorry, I just realized you were running a fork so my checkout command did not work. 😮

Here are my thoughts:

  1. I enjoy that the focus is not lost.
  2. If you remove the first block in the list, focus is still lost. In this situation, you should focus the next block from current position.
  3. I am not sold on this being the best solution. If I open the List View to interact with blocks, I would not want to focus the block content after removing. I would want to focus the previous item in the List View. Does this make sense?

Thanks.

@alexstine alexstine added [a11y] Keyboard & Focus [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Package] Block editor /packages/block-editor [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility labels Feb 25, 2022
@alexstine alexstine requested a review from MarcoZehe February 25, 2022 17:06
@MarcoZehe

Copy link
Copy Markdown
Contributor

I agree with @alexstine, if the block is deleted from the list view, focus must stay in the list view and focus must be put on the item that corresponds to the newly selected/focused block. Otherwise, this would be very inefficient.

@alexstine

Copy link
Copy Markdown
Contributor

@talldan I am sure can speak further on this but I bet this won't be easy after thinking about it. I am sure the delete action probably has nothing to do with the List View so I am not sure there will be an easy way to focus the List View itself. Certainly something worth exploring though.

@delowardev

Copy link
Copy Markdown
Contributor Author

I am not sold on this being the best solution. If I open the List View to interact with blocks, I would not want to focus the block content after removing. I would want to focus the previous item in the List View. Does this make sense?

For ListView, It makes more sense than current behavior; but it won't be something easy to achieve I think.
It's not related to the Remove block action, It's actually how current Block Selection works; after selecting a block it focuses on the correspondent block element no matter how we select the block. For example, if someone duplicates a block or clicks on a block name from the List View, it will move focus to the block from List View.

@alexstine

@alexstine

Copy link
Copy Markdown
Contributor

@delowardev Yeah, figured as much. This is so component based that it applies to each block vs. the list view itself. Not sure what the answer is here without a ton of custom dev work. Might want to get a tracking issue and at least get this merged for the fix for focus loss. I'll play with it some more over the weekend and try to give it a more solid review.

I also recommend expanding the E2E test to cover jumping forward to the next block just in case this is broken in the future.

@MarcoZehe MarcoZehe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I think the tests cover all scenarios at least I would also have thought of. But please don't merge until code owners also reviewed this.

@talldan talldan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I think this is as good as we can get right now given the constraints that @alexstine mentioned.

I think it'd be good to consider opening a bug report for the focus being moved outside of list view so that it doesn't get forgotten.

@alexstine

Copy link
Copy Markdown
Contributor

@delowardev Sorry for the delay on this. Can you fix the merge conflicts? Then I'll do final review and try to merge this by end of today.

@alexstine alexstine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good. Will merge when checks complete.

@alexstine

Copy link
Copy Markdown
Contributor

@delowardev Found another bug. :(

  1. Open a page/post.
  2. Insert some blocks.
  3. Select the last block inserted.
  4. Open the list view.
  5. select a middle block that is not selected. E.g. if I have 5 blocks, I'll make sure the 5th block is selected for editing. Then I will select Options on the 3rd block.
  6. Select Remove from the Options menu.
  7. The focus loss occurs.

It seems it only works if you remove a block that is currently selected for editing.

Thanks.

@delowardev

delowardev commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

I think it's expected that if someone clicks outside, it will focus on clicked HTML element instead 🤔 But yeah, I got your point here, I think there are some other situations too where it would be challenging to keep the focus on the selected block, such as:

  • Select multiple blocks and then press Tab >> focus is not in the block
  • Select 3rd block; then click options dropdown menu on 1st block ( or any block which is not select); then press Esc >> focus will not be in the selected block

@alexstine Thoughts?

Also, here I tried to select prev/next after removing the selected block; previously, it wasn't updating the selection after removing a block.

Thanks

@alexstine

Copy link
Copy Markdown
Contributor

@delowardev It seems to only update the selection if you remove a selected block. In other cases, it still fails. Do you have any idea to work around that or maybe that should be for a future PR?

@delowardev

Copy link
Copy Markdown
Contributor Author

@alexstine, I think that should be a separate ticket. It will require much more effort to achieve those block/element focus in all the mentioned cases.

@alexstine alexstine merged commit 05b8a7d into WordPress:trunk Mar 15, 2022
@alexstine

Copy link
Copy Markdown
Contributor

@delowardev Sounds good. We can look in to it in future PR.

@github-actions github-actions Bot added this to the Gutenberg 12.9 milestone Mar 15, 2022
@delowardev delowardev deleted the fix/list-view-focus-loss branch March 15, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Package] Block editor /packages/block-editor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List View: Removing a block doesn't update selection.

4 participants