refactor!: set focus-ring attribute on programmatic focus #10049
Conversation
web-padawan
force-pushed
the
refactor/focus-ring-programmatic-focus
branch
from
f8d7a24 to
e6bb9e3
Compare
| } | ||
|
|
||
| super.focus(); | ||
| this._setFocused(true); |
There was a problem hiding this comment.
Removed _setFocused(true) as the item will call it anyway on receiving focus. Also updated the test to use tabindex="0" on the item element to make it focusable so that focusin would be fired.
web-padawan
force-pushed
the
refactor/focus-ring-programmatic-focus
branch
3 times, most recently
from
e090383 to
a2fb326
Compare
|
Does the focus ring always appear in confirm dialog button? I’d expect it to not appear if the dialog opened as a result of an action I did with the mouse. But I’m not 100% sure. Maybe it always should, so you know you can just press enter (and that pressing enter will actually trigger the confirmation). Perhaps it’s more about focusing the button in the first place, and we should rather just focus the dialog overlay. |
I could change to use |
|
Similarly with Upload: I wouldn't expect the focus ring to appear visibly on the Add File button when removing the last file by clicking with the mouse. |
|
Updated both |
| * @override | ||
| */ | ||
| focus() { | ||
| focus(options) { |
There was a problem hiding this comment.
Out of scope of this PR, just to mention it for completeness sack: flow also has s focus method which would need the overload and probably.. even tho I'm not sure where you wanna draw the line.. because some flow components implement Focusable and are not really your web-components (e.g. native html and so on..) so not sure if that would be the right interface..or your way of implementing it would also cover other elements
There was a problem hiding this comment.
Thanks for pointing out. Could you please create a separate issue in Flow? I think some users could also benefit from preventScroll: true.
|
I’m still unsure about the confirm dialog behavior. @rolfsmeds, opinions? If the confirm button is automatically focused when the dialog is opened, should it have a focus outline? I’m almost leaning towards that. The other option would be to keep the focus on the dialog overlay, not focus the button. That said, I suppose that’s a separate topic, and not blocking this PR. At least it’s consistent with the overall focus outline behavior, that most often it shouldn't become visible if the user navigates with the mouse. |
|
I'm also not 100% sure about confirm dialog. I'm also leaning towards outline focused button (even when opened with mouse). Reasoning: People should see what Funny example, MacOS Dialog to confirm git credentials.. even tho I focused "immer erlauben" (allow forever) - pressing enter only executes "Erlauben" (allow once) |
|
Yeah, macOS has always had this behavior. The Enter key always triggers the primary/submit action of that particular view/dialog, while the Space key triggers the focused button. |
|
There's also screen readers to take into account here. The neverending dilemma of where to put focus in a dialog and some already known issues with JAWS.. Adrian Roselli wrote a piece on it just recently: https://adrianroselli.com/2025/06/where-to-put-focus-when-opening-a-modal-dialog.html |
|
Let's not change where focus is put and keep focusing submit button. The question is, should it always have focus-ring? |
|
I'd say yes, for the reasons mentioned above. |
web-padawan
force-pushed
the
refactor/focus-ring-programmatic-focus
branch
from
b97c352 to
8a8522c
Compare
|
Changed the confirm-dialog to always set |
There was a problem hiding this comment.
Should AccordionMixin#focus and DashboardItemMixinClass get the options parameter as well?
This reverts commit b84a0bd.
web-padawan
force-pushed
the
refactor/focus-ring-programmatic-focus
branch
from
8a8522c to
801b5fb
Compare
Done. |
|
|
This ticket/PR has been released with Vaadin 25.0.0-beta2. |
|
@web-padawan this might need an update for tearbench as well. I noticed some of my screenshot comparison tests are failing cause clicking a button triggers programmatical focus.. see https://github.com/vaadin/testbench/blob/f217e0f726f99b7b84d247acfbb19dce23536c4d/vaadin-testbench-shared/src/main/java/com/vaadin/testbench/TestBenchElement.java#L198 |
|
Oh, that's an unfortunate side effect indeed. I will discuss with the team what to do about it. |
6 participants
Description
Fixes #437
Fixes #1357
focus-ringattribute when callingfocus()programmaticallyfocus({ focusVisible: false })to prevent settingfocus-ringfocus()method to pass options objectNotable changes:
vaadin-login-overlay, the username field now hasfocus-ringunlessnoAutofocusis setType of change