Backspace doesn't (always) delete images

[Trapsta]

What?

This PR sets focus to the parent block when merging via backspace into a block's caption field.

Why?

This fixes an issue where the cursor gets stuck in the block's caption when backspacing from a paragraph into an image block. See #7422

How?

In the block's onfocus handler, I am setting focus to the block if I detect that the target of a "reverse" merge is a caption by checking if the nodeName is a figcaption. I am not very sure if this is the most reliable way to detect captions so any suggestions here would be much appreciated. This also fixes the bug in other blocks that have captions i.e. embed, video etc

Testing Instructions

1. Create a post with an image and add a bit of text after it
2. Place the cursor at the end of the text and backspace until the paragraph is empty
3. Backspace once to remove the paragraph
4. Backspace again from the image block

Fixes #7422

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Trapsta! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[alexstine]

@Trapsta I'll give this a review but I might have actually caused this bug indirectly from an accessibility fix.

#37934

Essentially, this will find the first element to focus when block receives focus. This had to be done because it is confusing to users to have focus placed on an element that gives them 0 context such as a div that doesn't have contenteditable attribute set. I had been thinking about how to fix this for some time now and really never came to a solid solution.

[Trapsta]

Great, thanks @alexstine for the background and the review (in advance). That gives more clarity and it makes a lot of sense to set focus on the more useful elements in a block. Would be happy to bounce ideas since I feel like my solution is a bit narrow and too specific so let me know if there is any better way to accomplish it or if we need to discuss the issue further

[alexstine]

@Trapsta Added a thought.

@tellthemachines @talldan Any ideas here? I knew this would come at some point but just not sure how to work around UX and A11Y.

[alexstine]

For this code to work, it likely needs to be placed after my code on this line.

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-list/use-block-props/use-focus-first-element.js#L115

If not, it will likely just add focus back to the caption field.

I am not sure this is the best way to handle this either. Seems like putting focus on the last element traveling backwards is the right thing to do.

[Trapsta]

In that case , do you think if we go with @tellthemachines 's suggestion in this comment , would that be more in line with putting focus on the last element traveling backwards

[tellthemachines]

Thanks for addressing this issue!

From the discussion on #7422, it looks like the preferred solution is to keep moving focus to the caption on pressing Backspace from the block underneath, and then select the block only once the caption content is deleted. So, backspace from an empty caption should select the block, instead of Backspace from the previous block. I think this makes sense, because if we just want to select and delete all the blocks, we can easily do so with Shift+Arrow Up and then Backspace.

I might have actually caused this bug indirectly from an accessibility fix.

@alexstine , your PR didn't cause the bug. It's an old one - that issue has been open since 2018 😅

[Trapsta]

@tellthemachines @alexstine Thanks a lot for the feedback. Made some changes to address some the things that came up. Also fixed a bug that was resulting in the caret being inserted at the beginning of the caption instead of at the at the end when backspacing. The bug was on this line in the useFocusFirstElement hook.

// If reversed (e.g. merge via backspace), use the last in the set of
// tabbables.
const isReverse = -1 === initialPosition;

isReverse was always returning false because we were not setting initialPosition when we were trying to merge via backspace.
Next , I moved the boilerplate code for setting up captions to a new component and added code to set focus to the block when backspacing from an empty caption since the block was already selected in most cases.

[Trapsta]

added an initialPosition param to denote the direction of merge when backspacing to fix a bug in useFocusFirstElement hook where it was not detecting reverse merging via backspace due to initialPosition not being set

[Trapsta]

Added a BlockCaption component to avoid duplicate and redundant code in all blocks that have a caption

[alexstine]

This seems to not be working. 😞 After deleting text from the paragraph block, the block disappears. After this, focus moves to the caption field. Pressing Backspace again does not remove the block.

[ndiego]

@Trapsta just checking in to see what the progress of this PR is. Thanks!

[alexstine]

A lot has changed for this block since this PR started. Good to close?

[alexstine]

This can always be re-opened in the future. Just trying to do a little housekeeping. 👍