Writing Flow: Move selection to block's focus handler#5289
Conversation
youknowriad
left a comment
There was a problem hiding this comment.
There's a small issue with these changes.
- create multiple text blocks
- Navigate between these blocks using arrows
- click shift + left/right arrows to select text
- The toolbar shows up in the wrong block, because the wrong block is selected in the state.
|
@youknowriad I'm not able to reproduce this, and it doesn't seem like something that ought to be happening, if the block is selecting itself in response to the focus event. |
|
Ah, reproduced in Firefox, but not Chrome. |
|
@aduth Here's a gif showing the issue (I'm using firefox)
|
|
Personally I'd find this to be a more important fix than the one addressed in #5112, particularly since the toolbar is still problematic in Safari, another solution is likely needed. I'll continue debugging for a bit longer though... |
|
I tried to demonstrate the issue with a simplistic CodePen: https://codepen.io/aduth/pen/MQxRME In master, the block's edit wrapper is focusable, but since the toolbar is outside the edit wrapper and buttons do not emit focus events on click in Firefox and Safari, the focus instead occurs on the parent block's edit wrapper. If we move I'm still trying to understand the original reason it was moved in #2934. Given original attempts at a fix to what ultimately led to the move, I'm wondering if it's even still relevant, since part of the changes proposed here is to remove this |
|
Re-testing the original issues described at #2934 (comment), they still appear to work correctly here. |
|
Discovered an issue: Arrowing to an image placeholder block skips it to the following block. |
|
Not so surprisingly, this was a result of our fragile direct selectors being fragile. Fixed in 386ac09. |
|
If these changes are solid, I suspect we might also be able to drop the gutenberg/editor/components/block-list/block.js Lines 415 to 417 in f80c123 I'd explored this once in the past and I think it was needed at the time based on how focus of blocks works. Would be worth exploring, but not necessary here since it's more a simplification of removing unnecessary event handlers. |
|
This was punted as part of the 2.3 release because of the complexity of the changes on a release day, but I'd like to get this in to allow remaining issues to be settled in time for the next release. I will plan to merge this if there is no other remaining feedback. |
See: #2934 See: https://codepen.io/aduth/pen/MQxRME Something needs to capture the focus event, which is skipped by button press on the block's toolbar, but bubbles up wrongly to the parent's edit wrapper (which has tabIndex) and causes parent block to select itself.
Corresponding to move of tabIndex from inner edit element to wrapper, we also need to capture focus from wrapper. This way, when focus is applied via WritingFlow, the block appropriately becomes selected. This may also make "onSelect" of movers unnecessary.
Can now rely on focus behavior captured from wrapper node to reflect selection onto blocks, even those without their own focusable elements
Multi-selection causes focus event to be triggered on the block, but at that point `isSelected` is false, so it will trigger `onSelect` and a subsequent `focusTabbables`, thus breaking the intended multi-selection flow.
Focus will bubble to block wrapper (or in case of Firefox/Safari, where button click itself does not trigger focus, focus on the wrapper itself) to incur self-select.
This reverts commit 90a5dab.
|
What started as an exploration of fixing an issue where arrow navigating to an image placeholder would focus but not select a block (4a85e23) resulted in a cascade of commits. In the end, I think it's a more stable implementation overall, but I've come to the conclusion that this is all rather risky code to be touching, so would appreciate another look. |
|
Noticing an issue, but also exists on master:
Expected: In subsequent paragraph block |
This pull request seeks to resolve conflicts between the WritingFlow's selection transitioning and nested blocks. Prior to these changes, when attempting to press "Down" from the first block in a column, the new block would be added, but focus would be moved back to the first block in the column. This was caused by two related issues:
closestTabbableis the default block appender itself. For non-nested blocks, attempting to select parent block has no effect because there is no "closest block" for the appender. In a nested context, however, the closest block is the wrapper block (Columns), so therefore it becomes selected and focuses its first tabbable which is the first block's wrapper.this.node(i.e. clicking at the edge of a block, or a block without its own focusable elements).IgnoreNestedEventscomponent to flag events as handled so that ancestor blocks don't attempt to handle them. With 8bd012a, since any focus event would cause the block to select itself if not already selected, we need to ensure that focus events from nested blocks are appropriately flagged. However, the previously described behavior of focusing the default block appender occurs outside the block itself, so it will never apply the expected_blockHandledflag.DefaultBlockAppenderwith its ownIgnoreNestedEvents, capturing and applying the handled flag to events managed in the appender.BlockListLayoutto a child's rendering behavior (theDefaultBlockAppender). I had unsuccessfully explored other options, including aIgnoreNestedEvents.Barriercomponent sitting at the top of a nested context and flagging every event. Besides being wasteful (not all events are monitored, and some events dispatch very often likemousemove), it introduced more problems of its own where expected events were not being handled correctly (like a block dismissing its hover effects).Testing instructions:
Verify that there are no regressions in the behavior of selecting a block via focus, both for blocks with their own inputs (paragraph) and those without (image placeholder).
Verify that you can add a default block to the end of a block list, both in a nested and non-nested context, by pressing "Down" from the last non-empty block.