Skip to content

feat: Add CSS classes for more focus states. #9046

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

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

BenHenning
Copy link
Contributor

The basics

The details

Resolves

Fixes #9044

Proposed Changes

TODO: Finish this PR description.

Reason for Changes

Test Coverage

Documentation

Additional Information

This adds better tracking support and is not yet completed.
@github-actions github-actions bot added the PR: feature Adds a feature label May 13, 2025
@BenHenning
Copy link
Contributor Author

NB: This isn't completed yet as subtrees still need to be added, and a lot more testing needs to be conducted.

I'm also unlikely to be comfortable merging this without new focus manager tests as the edge cases here can be quite complex.

@BenHenning
Copy link
Contributor Author

I was originally pushing to try and get this into v12, though while that has become increasingly less likely as v12 approaches launch I also no longer want to. The CSS changes for fields and trees are relatively benign, but adding the new CSS classes for subtrees actually introduces non-trivial risks:

  • It involves adding a lot more potentially very expensive (but hopefully not) tree crawls for every node change in order to properly calculate parent trees that are inheriting an active/passive node.
  • The act of interacting with all these trees could stir up new problems if there are certain conditions where trees are partly deleted or in a bad state. While those bugs could already exist today, this logic would increase the likelihood of hitting them and introducing a catastrophic failure.

While I actually expect the overall changes to end up being safe and sufficiently performant, the risk is too high to risk trying to rush this into v12.

This is not finished or working yet.
…ocus-states

Conflicts:
	core/focus_manager.ts
@BenHenning BenHenning changed the base branch from rc/v12.0.0 to develop June 27, 2025 00:44
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Jun 27, 2025
This ensures that it updates when the focused node changes. This will,
of course, need thorough testing due to the bunch of possible edge
cases.
@BenHenning
Copy link
Contributor Author

I think this actually implements all of the CSS classes that we need per #9044 except for the subtree ones.

Those are...a bit hard to implement correctly. I'm actually considering inverting the way subtrees are provided so that they are actually indicated through registration. This will probably require us to do cycle checking, but the current pattern doesn't prevent that and we don't currently check for it. :)

The advantage of explicit registration is that it would make it much easier to notify parents when something changes in a subtree, including multiple levels of updates, such as active/passive focus changing, i.e., this PR. Since we already require trees to register, this isn't a big difference in management for callers and, to some extent, actually makes things a bit easier for ensuring that unregistered subtrees aren't accidentally represented.

That's a pretty big change, and we'll need to file a follow-up bug to fully remove the nested trees function in v13. For now I think we can deprecate the function and remove all calls to it (plus update its documentation accordingly). Basically no one should be using subtrees outside core, anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce CSS classes for a variety of focus states
1 participant