-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
fix: gutter hover tooltip a11y improvements #5747
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5747 +/- ##
==========================================
+ Coverage 87.06% 87.08% +0.01%
==========================================
Files 598 598
Lines 43722 43801 +79
Branches 7164 7171 +7
==========================================
+ Hits 38067 38143 +76
- Misses 5655 5658 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/layer/gutter.js
Outdated
@@ -524,6 +525,7 @@ class Gutter{ | |||
annotationNode.setAttribute("aria-label", ariaLabel); | |||
annotationNode.setAttribute("tabindex", "-1"); | |||
annotationNode.setAttribute("role", "button"); | |||
annotationNode.setAttribute("aria-describedby", GUTTER_HOVER_TOOLTIP_ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this set aria-describedby
on all gutter annotations which are currently rendered, also on those which the open tooltip doesn't relate to? would that cause issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would indeed set it on all annotations, I dont see it being an issue though since the tooltip is the main way of viewing those annotations, so technically all of them are described by it. do you think its better to only add this attribute to the annotation currently described by the tooltip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With aria-describedby always being set to the tooltip, when you keyboard navigate over the gutter annotations does it still properly read the aria-label of that annotation or does it read the tooltip (even when the tooltip isn't currently open)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah indeed, it seems that besides the aria label it will also read the tooltip, resulting in cases where it would read the tooltip from the previous annotation. I'll see if I find an easy fix for this, if not I'll remove this and tackle it in another PR.
src/mouse/default_gutter_handler.js
Outdated
// dont close tooltip in case the user wants to copy text from it (Ctrl/Meta + C) | ||
if (e && e.type === "keydown" && (e.ctrlKey || e.metaKey)) | ||
return; | ||
// dont close tooltip if mouse is on tooltip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reading this comment it reads as if when I hover over a gutter annotation and the tooltip shows, pressing escape doesn't close it (as the mouse is still on the tooltip). But in the test we add that seems to be what happens, am I misreading this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tooltip will close when pressing escape even if mouse is on the tooltip, so I'll change the comment to better reflect that
dea1dd2
to
8fe9581
Compare
I think the first bullet-point of the PR description is no longer accurate, could you remove it to prevent future confusion 🙂 |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Pull Request Checklist:
ace.d.ts
) and its references: