Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigator: move focus while navigating between screens #37063

Closed
ciampo opened this issue Dec 2, 2021 · 8 comments
Closed

Navigator: move focus while navigating between screens #37063

ciampo opened this issue Dec 2, 2021 · 8 comments
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@ciampo
Copy link
Contributor

ciampo commented Dec 2, 2021

In #36952 (comment), @mtias bring up the issue that the _ "back" links need the ability to focus the element they represent when you go back,_

I'm trying to summarise the questions that we need to answer here:

There's a few questions to be solved:

  • Is focusing the exact link that "generated" the navigation necessary? Currently, a NavigatorScreen should already be able to move the focus on its own wrapper div when mounted.
  • Should we look for a solution that can be applied directly to the Navigator component?
  • What's the best way to store an ID for an element that gets removed for the DOM? (i.e. the "origin link" that we'd like to focus)\
  • If we think that we'll need to start storing state in our Navigator component, should we re-consider adopting a third-party routing solution?

cc @diegohaz @youknowriad @mirka

@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components [Feature] Component System WordPress component system labels Dec 2, 2021
@ciampo ciampo added this to Inbox (needs triage) 📬 in WordPress Components via automation Dec 2, 2021
@ciampo
Copy link
Contributor Author

ciampo commented Dec 2, 2021

Personally, I'm not sure we'd need to restore the focus on the previous clicked link — focusing the whole "screen" should be enough (as it generally happen when going to the previous page in a browser via its standard history)

@youknowriad
Copy link
Contributor

youknowriad commented Dec 2, 2021

Personally, I'm not sure we'd need to restore the focus on the previous clicked link — focusing the whole "screen" should be enough (as it generally happen when going to the previous page in a browser via its standard history)

This should be the case already with the useFocusOnMount

@ciampo
Copy link
Contributor Author

ciampo commented Dec 2, 2021

This should be the case already with the useFocusOnMount

Exactly, that's why I wrote:

Is focusing the exact link that "generated" the navigation necessary? Currently, a NavigatorScreen should already be able to move the focus on its own wrapper div when mounted.

I'm wondering if the current behaviour is not enough already

@mtias
Copy link
Member

mtias commented Dec 2, 2021

I see this behaviour for a list of Items comparable to toolbar items opening dropdowns — the expectation is when you close the dropdown the item that opened it is focused, not that it resets to the whole toolbar. Imagine the case with the list of blocks, which can be anywhere from 20+ to 100. You are making modifications to a blocks in sequence, but every time you go back it resets focus. It'd be pretty annoying.

@ciampo
Copy link
Contributor Author

ciampo commented Dec 2, 2021

If we do need to implement this behaviour, it means that we need to start passing/storing state when a route changes (as explained also by @diegohaz here).

This potentially means that we'd need to keep a navigation history inside the internal Navigator state (together with an associated identifier of the element/component that caused that specific route navigation), so that when we we navigate "back" we have a reference of which element to focus.

On top of that, we'd need to understand what's the best format for that "identifier"

@mtias
Copy link
Member

mtias commented Dec 3, 2021

Or we could also think about it more broadly as cases where focus needs to be transferred because an interface session "ended". An interface can "end" by closing a popover, closing a modal, closing a sidebar, deleting a block, and also navigating back from a panel to its parent panel. That means perhaps the state we need to store is not at the Navigator level but at the general UI level.

@ciampo
Copy link
Contributor Author

ciampo commented Dec 9, 2021

I've implemented a potential solution in a draft #37223. It still requires polishing, but it's definitely at a stage where we can judge if it's an approach that we want to implement or discard

@ciampo ciampo moved this from Inbox (needs triage) 📬 to Backlog (triaged) 📝 in WordPress Components Nov 18, 2022
@ciampo
Copy link
Contributor Author

ciampo commented Nov 18, 2022

Closing as focus restoration was initially implemented in #38149 and later refined

@ciampo ciampo closed this as completed Nov 18, 2022
WordPress Components automation moved this from Backlog (triaged) 📝 to Done 🎉 Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
Development

No branches or pull requests

3 participants