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

Rich text editor | Fix for updating "tabindex" of button in toolbar when clicking on the icon #1503

Merged
merged 11 commits into from
Sep 13, 2023

Conversation

vivinkrishna-ni
Copy link
Contributor

@vivinkrishna-ni vivinkrishna-ni commented Sep 8, 2023

Pull Request

🤨 Rationale

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

As mentioned in this comment (#1422 (comment)), adding pointer-events: none; to all the icons used inside the nimble-toggle-button.

🧪 Testing

  • 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

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@vivinkrishna-ni vivinkrishna-ni marked this pull request as ready for review September 8, 2023 13:35
@jattasNI jattasNI requested a review from mollykreis September 8, 2023 21:38
@rajsite
Copy link
Member

rajsite commented Sep 12, 2023

Since we are in alignment and it's well covered you don't need to wait on @mollykreis's review

@vivinkrishna-ni vivinkrishna-ni merged commit 0940f82 into main Sep 13, 2023
6 checks passed
@vivinkrishna-ni vivinkrishna-ni deleted the users/vivin/fix-for-other-button-focus branch September 13, 2023 03:47
vivinkrishna-ni added a commit that referenced this pull request Sep 20, 2023
…alue update in footer buttons (#1550)

# 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.
-->
We recognized that the workaround done in this PR
#1503 creates an issue in iOS mobile
browsers. In mobile/Tab iOS browsers, when the button is clicked, it
does not automatically focus back on the editor, and the keypad closes.

Bug link: https://dev.azure.com/ni/DevCentral/_workitems/edit/2526025

With `pointer-events: none` to `slot="start"` of toggle button:


https://github.com/ni/nimble/assets/123377523/a0ec33d0-2040-4cc9-ac82-2e8c720f2bba

Without `pointer-events: none` to `slot="start"` of toggle button:


https://github.com/ni/nimble/assets/123377523/5c4b0e95-5ea4-4248-8559-7450bcdc2d04

After aligning with the leads, as a quicker resolution, we thought that
this bug would create more impact for the mobile users than the other
`tabindex` issue.

## 👩‍💻 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.
-->
Reverting all the changes that were implemented here:
#1503

## 🧪 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.
-->
Manually tested iOS devices(mobile and tab) in Safari and Chrome
browsers.
1. Opened
[nimble.ni.dev](https://nimble.ni.dev/storybook/?path=/docs/incubating-rich-text-editor--docs)
on an iOS device and clicked the buttons, the editor loses the focus.
2. Opened the current [storybook
publish](https://60e89457a987cf003efc0a5b-kqouhrfikv.chromatic.com/) on
an iOS device and clicked the buttons, the editor got the focus as
expected. (As shared in the above recordings)

## ✅ 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.

---------
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.

6 participants