Size Change: +551 B (0%)

Total Size: 1.62 MB

_{compressed-size-action}

Flaky tests detected in 2f56283.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6181244288
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

@Mamaduka I know the E2E tests here are most certainly related, this is a good restructure. I think the introduced selector by @jeryj will be helpful but that still doesn't solve the problem of a last focus position being no longer available. That's what happens when we as developers change the UI so much that React can no longer reconcile the differences. For example, if you use the block toolbar to change to preview mode in the Custom HTML block, the last focus position no matter how it is stored is no longer available due to the whole re-rendering of the block UI.

The goal of the first stage of this review is to gather opinions on if this improved flow seems okay. If so, I'll work on improving the tests. If it would help, I can add a new writing flow test so you can see the intended goal but essentially this PR is trying to find a constant focus point for blocks that re-render their UI content.

Thanks.

Feel free to cherry-pick the commit that adds getLastFocus to the redux store (77a48ac2b7cd47316dd3f58f696261a3a83cae6b) and add it to this PR instead. That'd be great, as I'm happy to make the toolbar DOM refactoring PR smaller if possible 😅

@jeryj Done.

Should I also try to add a new reducer for storing the HTML element with last focus? That is what this PR tries to solve. When last focus element is no longer in the DOM.

Should I also try to add a new reducer for storing the HTML element with last focus?

I think I'm misunderstanding. setLastFocus stores whatever is focused event.target is when exiting the editor canvas. Do you mean adding one for where to return focus to if the stored element in getLastFocus isn't found?

@jeryj Yes, sorry. I asked it backwards. If the last focus element is not found in the DOM, should I also track the last selected block or is there a way to get the current clientId already?

should I also track the last selected block or is there a way to get the current clientId already?

Hmmm... I can see how it would be useful to bundle these concepts into one spot. The main concept we're implementing here is: where should focus go when it returns to the editor?

It would be nice to be able to easily send focus back to the editor from anywhere. I don't have much Gutenberg architecture experience, so I'm sorry I'm not more help here! I'm interested to learn along with this though, and can ping some people if needed to help.

@jeryj Yes, it would be great to know how to handle this. There are a few blocks now that have re-rendering UIs so sometimes the last focus element is no longer accessible. Need to find out if I should be tracking a reference to the block wrapper as well or if I could find the currently selected block wrapper with the clientId. Currently, the solution I propose is event.target.closest( '[data-block]' ); which works but now need to think about how to store the value I get from this.

Another way would be something like:

const getBlock = document.querySelector( '[data-block="${ clientId }"]' );

Then the variable could be passed in to focus.tabbable.findNext or other helper functions to determine a good focus location.

Right now, the ref stores this implementation but would be unavailable to anything outside of writing flow.

It feels like storing this in a reducer would be better vs. scanning the DOM for an attribute but that's just me.

As far as focus around the editor today, there isn't a constant focus point so that is the goal to fix.