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

#1711: Include focus on tooltip link #1712

Merged
merged 7 commits into from
Feb 26, 2024

Conversation

srikant-ch5
Copy link
Contributor

Ref: #1711

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@nmgokhale
Copy link
Member

nmgokhale commented Feb 20, 2024

This isn't working well. I press tab and shift+tab keys to move focus in the video. When the focus is on tooltip link, after pressing tab key, focus goes to the browser URL. Ideally it should behave similar to carbon behavior here - https://v10.carbondesignsystem.com/components/tooltip/code/#live-demo

Also onBlur function isn't working. Tooltip should have been closed when I clicked on side panel button.

Screen.Recording.2024-02-20.at.10.12.41.AM.mov

In Carbon demo, When tooltip is open and link is in focus, when I press Escape key, tooltip closes and focus goes back to i icon. After pressing tab key, focus goes to next focusable element. When tooltip link is in focus, pressing tab and shift + tab keys, focus keeps shifting between tooltip elements but focus never goes out of the tooltip.

carbon-demo.mov

To mimic this behavior, Our tooltip code will have to capture the key presses and (if there is only one link) suppress them (and also any Shift + Tab key presses) - then capture the ESC key press and close the tip in that case. You can try evt.stopPropagation() and probably evt.preventDefault() to suppress tab, Shift + tab key presses when the link is in focus.

@srikant-ch5 srikant-ch5 marked this pull request as draft February 21, 2024 05:30
@srikant-ch5
Copy link
Contributor Author

Hi @nmgokhale I have updated PR with above changes please review and let me know of any changes.

nmgokhale
nmgokhale previously approved these changes Feb 22, 2024
@@ -320,6 +331,9 @@ class ToolTip extends React.Component {
// To prevent this default behavior, stopPropagation and preventDefault is used.
evt.stopPropagation();
evt.preventDefault();

// when tooltip with link is closed and another tooltip is opened newly opened tooltip should have focus
Copy link
Member

Choose a reason for hiding this comment

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

Please start sentences in the comments with a capital letter. If you start with a lowercase letter it looks like part of the sentence has been lost.

}
}}
onBlur={() => {
if (linkClicked) { // keep tooltip open when link is clicked
Copy link
Member

Choose a reason for hiding this comment

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

First letter in upper case please

evt.stopPropagation();
evt.preventDefault();

if (evt.key === "Escape") { // When Esc is pressed shift the focus to tooltip icon so that user can navigate following elements.
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid having inline comments like this stretch too far across the screen. Short comments are OK but with one this length I'd prefer it to be above the if statement. BTW - I do appreciate this file even has code that stretches a long way across the page so that is not a good model to follow. Ah! If only I could go back and re-review it ;-)

@srikant-ch5
Copy link
Contributor Author

Hi @tomlyn I have updated comments as per above format. Thanks.

@nmgokhale nmgokhale marked this pull request as ready for review February 23, 2024 18:11
@srikant-ch5
Copy link
Contributor Author

srikant-ch5 commented Feb 26, 2024

Hi @nmgokhale. Could you please review this PR and let me know incase of any changes needed ?

@nmgokhale nmgokhale merged commit 6115524 into elyra-ai:main Feb 26, 2024
3 checks passed
@nmgokhale
Copy link
Member

@srikant-ch5 Issue #1711 didn't close after merging this PR. Next time in the PR description, please add fixes: #issueNumber instead of Ref: #issueNumber.

@srikant-ch5 srikant-ch5 deleted the 3662_ToolipHover branch June 7, 2024 05:38
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