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

feat(ui5-button): provide focus support for mobile #8414

Merged
merged 22 commits into from
Apr 11, 2024

Conversation

didip1000
Copy link
Contributor

Button elements can now receive focus when using keyboards on mobile devices. Focus outline is now displayed via CSS pseudo-classes and no longer relies on using the[focused] attribute.

The following components were updated:

  • ui5-button
  • ui5-toggle-button
  • ui5-segmented-button
  • ui5-split-button

Fixes: #8178
Related to: #8322

@didip1000 didip1000 self-assigned this Mar 8, 2024
packages/main/src/Button.ts Outdated Show resolved Hide resolved
packages/main/src/Button.ts Outdated Show resolved Hide resolved
packages/main/src/SegmentedButton.ts Outdated Show resolved Hide resolved
packages/main/src/SplitButton.hbs Outdated Show resolved Hide resolved
packages/main/src/SplitButton.ts Outdated Show resolved Hide resolved
packages/main/src/themes/SplitButton.css Outdated Show resolved Hide resolved
packages/main/src/themes/SplitButton.css Outdated Show resolved Hide resolved
@didip1000 didip1000 requested a review from unazko March 27, 2024 13:27
Copy link
Contributor

@unazko unazko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected and according to the task description.

@unazko unazko marked this pull request as ready for review March 27, 2024 15:32
@tsanislavgatev tsanislavgatev marked this pull request as draft March 28, 2024 08:09
@didip1000 didip1000 marked this pull request as ready for review April 1, 2024 10:16
@didip1000 didip1000 requested review from pskelin and ilhan007 and removed request for hinzzx April 1, 2024 12:14
@vladitasev
Copy link
Contributor

Why hasn't the focused property been removed from Button.ts? Is it still used by the component/its descendents?

@didip1000
Copy link
Contributor Author

Why hasn't the focused property been removed from Button.ts? Is it still used by the component/its descendents?

There was a misunderstanding between the team, we will remove it :)

@pskelin
Copy link
Contributor

pskelin commented Apr 4, 2024

I was wondering why all the focus handlers in the split button are still necessary?
Did you check if you can use :focus-within on the split button to find out exactly if the focus is on an internal button?

Ideally, there should be no focus handlers on the split button hbs, but in case something is wrong with the focus, we are working on a framework change that will allow you to use delegatesFocus

delegatesFocus Optional
A boolean that, when set to true, specifies behavior that mitigates custom element issues around focusability. When a non-focusable part of the shadow DOM is clicked, the first focusable part is given focus, and the shadow host is given any available :focus styling. Its default value is false.

This will be a solution for #8437, so please check if it will improve the split button implementation.

You can set it manually in UI5Element.ts

this.attachShadow({ mode: "open", delegatesFocus: true });

@didip1000
Copy link
Contributor Author

I was wondering why all the focus handlers in the split button are still necessary? Did you check if you can use :focus-within on the split button to find out exactly if the focus is on an internal button?

Ideally, there should be no focus handlers on the split button hbs, but in case something is wrong with the focus, we are working on a framework change that will allow you to use delegatesFocus

delegatesFocus Optional
A boolean that, when set to true, specifies behavior that mitigates custom element issues around focusability. When a non-focusable part of the shadow DOM is clicked, the first focusable part is given focus, and the shadow host is given any available :focus styling. Its default value is false.

This will be a solution for #8437, so please check if it will improve the split button implementation.

You can set it manually in UI5Element.ts

this.attachShadow({ mode: "open", delegatesFocus: true });

Hey Peter,

We are using the focus handlers to manage which element gets focus, depending on if we get focus through mouse or keyboard.

From what I understand, "delegatesFocus" will make it so that whenever the inner buttons are focused, the focus goes to the entire split button. However, we only want focus on the entire split button when navigating with a keyboard. Otherwise, the focus should remain on the inner buttons if they were focused by a mouse click.

If you have any ideas for how we could achieve this without focus handlers, I appreciate your feedback!

@pskelin
Copy link
Contributor

pskelin commented Apr 9, 2024

My point is that it is very strange to attach a focus handler on an element and then call e.target.focus() - this is already the handler triggered by the focus action. It should be possible to find out if the focus is on an element or on a child by the :focus and :focus-within selectors.

I commented both .focus() calls in SplitButton.ts in _onInnerButtonFocusIn and _onFocusIn and everything seems to work like expected if I add delegatesFocus: true for the button (otherwise the text button slot click doesn't focus). Makes me wonder if the focushandlers are necessary at all - I expect the inner buttons will focus themselves.

Please check if this can be simplified, otherwise you can go ahead with the change if I don't understand all details why this is necessary.

@didip1000
Copy link
Contributor Author

Hey @pskelin , you're right the focus() in the onFocusIn handler is unnecessary and I removed it. The focus in _onInnerButtonFocusIn is needed for the text button since focus doesn't show if the text itself is clicked.

As discussed seperately, the handlers will be removed at a later time.

packages/main/src/SplitButton.ts Show resolved Hide resolved
@pskelin
Copy link
Contributor

pskelin commented Apr 10, 2024

wdio.js file has Whitespace-only changes.
can you please remove it?

Also the tests will pass after a rebase from main.

@didip1000 didip1000 merged commit 4d9e32f into main Apr 11, 2024
9 checks passed
@didip1000 didip1000 deleted the button-focus-handling branch April 11, 2024 06:00
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.

UI5-Button | iOS | a11y - Button Focus outline not visible
5 participants