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.

Sign up for GitHub

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

Jump to bottom

Icon buttons used in toolbar is not getting 'tabindex' value '0' when clicked #1422

Closed
vivinkrishna-ni opened this issue Aug 10, 2023 · 3 comments · Fixed by #1642
Closed

Icon buttons used in toolbar is not getting 'tabindex' value '0' when clicked #1422

vivinkrishna-ni opened this issue Aug 10, 2023 · 3 comments · Fixed by #1642
Assignees
@mollykreis
Labels
bug Something isn't working

Comments

@vivinkrishna-ni
Copy link
Contributor

vivinkrishna-ni commented Aug 10, 2023 •
edited
Loading

🐛 Bug Report

When using buttons with icons within the nimble-toolbar and clicking on the icon, the button does not receive a tabindex=0. By default, the first button in the toolbar possesses a tabindex=0, whereas the remaining buttons have a tabindex=-1. Consequently, upon pressing the Tab key to shift focus from the previously selected field to the buttons, the focus does not transition to the button that was most recently clicked.

💻 Repro or Code Sample

  1. Add nimble buttons with an icon in it to a nimble-toolbar.
  2. Inspect the buttons and verify the default tabindex value.
  3. Click the icon in the button which has tabindex=-1.
  4. Notice the tabindex value in the dev tools.

🤔 Expected Behavior

The button which is clicked should be updated to tabindex=0. Also, the button which has tabindex=0 previously should be updated to tabindex=-1.

😯 Current Behavior

The button which is clicked is not updated to tabindex=0 when clicking on the icon. However, when clicking the button and not on the icon, the tabindex=0. However, upon clicking outside the icon, the tabindex gets updated.
tabindex-issue-toolbar

💁 Possible Solution

Add pointer-events: none; to start or end slots where the icon is placed.

🔦 Context

In nimble-rich-text-editor component, for footer formatting options we use nimble-toolbar with the nimble-toggle-button just the icon enabled with it. For more details, see: #1416 (comment) (issue 2)

Currently, nimble-rich-text-editor has the workaround fix as mentioned in the possible solution, and here is the link to the PR: #1503. Once the actual fix is implemented, the workaround in the editor styles should be removed.

🌍 Your Environment

  • OS & Device: Windows on PC
  • Browser Google Chrome
  • Version 115.0.5790.170
@vivinkrishna-ni vivinkrishna-ni added bug Something isn't working triage New issue that needs to be reviewed labels Aug 10, 2023
@mollykreis
Copy link
Contributor

I believe the issue is related to the clickHandler in FAST's toolbar. Within the clickHandler, the toolbar attempts to update its activeIndex, which represents which item in the toolbar should be focused. However, the check is that the item clicked is one of the focusable elements within the toolbar, not whether or not the item clicked is within one of the focusable elements. As a result, when the icon is clicked, the check doesn't pick up that the icon is within the focusable toolbar button, and the activeIndex doesn't get updated.

See: https://github.com/microsoft/fast/blob/218cfc014f5c76464493725f83bd883e8b07defa/packages/web-components/fast-foundation/src/toolbar/toolbar.ts#L134

@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Aug 15, 2023
@m-akinc m-akinc moved this to Current Iteration in Nimble Design System Priorities Aug 15, 2023
@aagash-ni aagash-ni mentioned this issue Aug 31, 2023
Merged
1 task
@mollykreis mollykreis self-assigned this Sep 5, 2023
@mollykreis mollykreis moved this from Current Iteration to In progress in Nimble Design System Priorities Sep 5, 2023
@mollykreis
Copy link
Contributor

I created microsoft/fast#6819 to track the underlying issue in the FAST toolbar.

However, we should also evaluate whether adding pointer-events: none to some of our nimble styles makes sense, such as the start and end slots of our button styles.

@vivinkrishna-ni vivinkrishna-ni mentioned this issue Sep 8, 2023
Merged
1 task
vivinkrishna-ni added a commit that referenced this issue Sep 13, 2023
@vivinkrishna-ni
Rich text editor | Fix for updating "tabindex" of button in toolbar w…
0940f82 
…hen clicking on the icon (#1503)

# Pull Request

## 🤨 Rationale

<!---
Provide some background and a description of your work.
What problem does this change solve?

Include links to issues, work items, or other discussions.
-->
Partial fix for #1422

**Before fix**: `tabindex` value of the icon buttons in the toolbar not
getting updated when clicking on the icon.
**After fix**: `tabindex` value of the icon buttons in the toolbar gets
updated when clicked on the icon.

With the fix, the last clicked button(anywhere in the button) will get
the focus when tabbing to the footer section next time.

## 👩‍💻 Implementation

<!---
Describe how the change addresses the problem. Consider factors such as
complexity, alternative solutions, performance impact, etc.

Consider listing files with important changes or comment on them
directly in the pull request.
-->
As mentioned in this comment
(#1422 (comment)),
adding `pointer-events: none;` to all the icons used inside the
`nimble-toggle-button`.

## 🧪 Testing

<!---
Detail the testing done to ensure this submission meets requirements. 

Include automated/manual test additions or modifications, testing done
on a local build, private CI run results, and additional testing not
covered by automatic pull request validation. If any functionality is
not covered by automated testing, provide justification.
-->
- Verified the `tabindex` value after clicking on and around the icon in
the buttons.
- Written test cases to check if clicking the slot content actually
updates the `tabindex` value of the button.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@vivinkrishna-ni vivinkrishna-ni mentioned this issue Sep 19, 2023
Merged
1 task
@jattasNI jattasNI added the blocked Blocked on a third-party issue label Oct 5, 2023
@jattasNI
Copy link
Contributor

jattasNI commented Oct 5, 2023

Marking as blocked to reflect the dependency on the FAST issue.

@mollykreis mollykreis removed the blocked Blocked on a third-party issue label Nov 1, 2023
@mollykreis mollykreis mentioned this issue Nov 1, 2023
Merged
1 task
mollykreis added a commit that referenced this issue Nov 2, 2023
@mollykreis
Update fast-foundation dependency (#1642)
6f571be 
# Pull Request

## 🤨 Rationale

The latest version of fast-foundation has fixes for two of our toolbar
issues, so this PR pulls in the latest version.

Resolves #1422 
Resolves #1423 

## 👩‍💻 Implementation

Bump version in package file
Update the rich-text-editor page object to adjust for a change in the
toolbar code

## 🧪 Testing

Used storybook to verify that the two toolbar issues are resolved

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@m-akinc m-akinc moved this from In progress to Done in Nimble Design System Priorities Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mollykreis mollykreis

Labels
bug Something isn't working
Projects
Nimble Design System Priorities
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update fast-foundation dependency ni/nimble
4 participants
@m-akinc @jattasNI @mollykreis @vivinkrishna-ni