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

Toolbar buttons tooltip: show help instead of label #5107

Merged
merged 2 commits into from
May 17, 2020

Conversation

manics
Copy link
Contributor

@manics manics commented Dec 7, 2019

Currently the tooltip for a toolbar button with just an icon is the help text for the action/command.
If a toolbar button has a label the label text is shown alongside the button icon as expected, but it also replaces the tooltip help.

With this PR the help text is always shown as the tooltip.

Old behaviour:

44-10
44-20

New behaviour:

45-17

45-26

@lresende
Copy link
Member

What is the accessibility side effect of this change? @jasongrout @takluyver any thoughts here?

@kevin-bates
Copy link
Member

@jasongrout - can you please comment on Luciano's accessibility question? Otherwise, I think this looks good.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

My review is based purely on the excellent screenshots - thank you.

There's still the question of accessibility that should be resolved.

@kevin-bates
Copy link
Member

@tgeorgeux - I believe you deal in the realm of UX. Do these changes introduce any accessibility issues and, if so, might there be a way they can be addressed while still providing this improved behavior?

@kevin-bates kevin-bates mentioned this pull request May 5, 2020
24 tasks
@blink1073 blink1073 added this to the 6.1 milestone May 15, 2020
@afshin afshin force-pushed the toolbar-button-help-tooltip branch from 0d592d3 to d7f86ef Compare May 16, 2020 11:29
Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I rebased your branch against master and pushed. I would suggest adding an aria-label and also defaulting to the el.label in cases where help is not available and just switching the default order instead of removing it altogether. Something like this diff:

diff --git a/notebook/static/notebook/js/toolbar.js b/notebook/static/notebook/js/toolbar.js
index 0e47e3470..295975c30 100644
--- a/notebook/static/notebook/js/toolbar.js
+++ b/notebook/static/notebook/js/toolbar.js
@@ -94,7 +94,8 @@ define(['jquery','base/js/i18n'], function($, i18n) {
                 }
                 var button  = $('<button/>')
                     .addClass('btn btn-default')
-                    .attr("title", i18n.msg._(action.help))
+                    .attr("aria-label", el.label)
+                    .attr("title", i18n.msg._(action.help)||el.label)
                     .append(
                         $("<i/>").addClass(el.icon||(action||{icon:'fa-exclamation-triangle'}).icon).addClass('fa')
                     );

I'm happy to push a commit with this myself since this PR was opened quite some time ago and you may be otherwise occupied.

@tgeorgeux
Copy link

@kevin-bates Sorry I missed this ping. I don't really know enough about the mechanics of what's going on to know if this change will negatively impact accessibility. It looks like a net improvement to me, with @afshin's changes mentioned above.

@manics manics force-pushed the toolbar-button-help-tooltip branch from 38689d8 to 5d47947 Compare May 17, 2020 21:08
@manics
Copy link
Contributor Author

manics commented May 17, 2020

Thanks for looking, I've made the requested change.

@afshin afshin removed the request for review from jasongrout May 17, 2020 21:30
@afshin afshin merged commit e9ce1b7 into jupyter:master May 17, 2020
@afshin
Copy link
Member

afshin commented May 17, 2020

Thank you!

@manics manics deleted the toolbar-button-help-tooltip branch May 17, 2020 22:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants