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

[Focus]: Set delegatesFocus on all components #559

Merged
merged 1 commit into from
Feb 25, 2024
Merged

Conversation

fallaciousreasoning
Copy link
Collaborator

This delegates the focus to the first focusable element (or the element with autofocus) inside the shadowDOM when .focus() is called on the element.

I don't know if we want this for every component, but it does seem like a sensible default.

https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/delegatesFocus

@fallaciousreasoning
Copy link
Collaborator Author

@petemill @AlanBreck any idea of the wider implications of this?

Copy link
Contributor

👋 Thanks for Submitting! This PR is available for preview at the link below.

✅ PR tip preview: https://559.pr.nala.bravesoftware.com/
✅ Commit preview: https://559.pr.nala.bravesoftware.com/commit-5dc55b45574284ea0449f5a5af1e01d7649761f6/

Variables Diff
--- ./tokens/css/variables.old.css	2024-02-20 22:26:26.141932107 +0000
+++ ./tokens/css/variables.css	2024-02-20 22:25:59.294033083 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Tue Feb 20 2024 20:43:18 GMT+0000 (Coordinated Universal Time)
+ * Generated on Tue Feb 20 2024 22:25:59 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {

@AlanBreck
Copy link
Collaborator

which may be undesirable if that element is not meant to be part of the tabbing order (for example, an element with tabindex="-1"), or if a more 'important' focusable element should receive initial focus (for instance, the first text field rather than the 'close' button which precedes it).

This seems like a caveat worth consideration. What are the negatives of not doing this?

@fallaciousreasoning
Copy link
Collaborator Author

fallaciousreasoning commented Feb 20, 2024

The negatives of not doing this are that for elements like the Button, Input, Dropdown ect calling .focus() on the HTMLElement doesn't behave as expected.

However, it sounds like we might not want this for non input type="..." components? I dunno.

WICG/webcomponents#126 (comment)

@fallaciousreasoning
Copy link
Collaborator Author

fallaciousreasoning commented Feb 21, 2024

FWIW, @nullhook PR brave/brave-core#22220 depends on this.

Maybe we need to come up with a way for components to opt in/out of this behavior?

@fallaciousreasoning
Copy link
Collaborator Author

Going to not merge this for another couple of days, for the brave-core release

@fallaciousreasoning fallaciousreasoning merged commit ba7e443 into main Feb 25, 2024
8 checks passed
@fallaciousreasoning fallaciousreasoning deleted the focus branch February 25, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants