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

Accessibility: Add missing tooltips to icon buttons #4318

Merged
merged 3 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Jan 5, 2018

closes #3384

This PR adds the missing tooltips to icon-only buttons.

  • The block switcher
  • Some media upload buttons

If you notice another missing tooltip, let me know.

@youknowriad youknowriad self-assigned this Jan 5, 2018

@youknowriad youknowriad requested a review from afercia Jan 5, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jan 5, 2018

Contributor

@youknowriad thanks!

1
Missing tooltips noticed so far:

  • Classic block: all (I know this is TinyMCE but the current editor has working tooltips on all its buttons, and they should have tooltips in Gutenberg as well)
  • Video block: pencil icon
  • Audio block: pencil icon
  • Table block: edit table

2
More related to design/consistency /cc @jasmussen : some tooltips have title-cased text, some not. Regardless of language-specific preferences (as far as I know title-case is preferred in English, but it's not in many other languages), there should be some consistency. Since these are tooltips and not button labels, I'd go for no title-casing. Some example of the current tooltips:

Insert block
Change block type
Align left
Move Up
Move Down
Edit Gallery
Edit image
Remove Image
Grid View
List View

3
Slightly unrelated, I guess it should be addressed elsewhere: when I insert an image, by default it has no link. However, the link button says "Edit link". In the classic editor, the link button always says "Insert/edit link" to avoid this issue.

4
Unrelated but worth mentioning: the image edit link "modal" has some issues...

screen shot 2018-01-05 at 14 36 11

Contributor

afercia commented Jan 5, 2018

@youknowriad thanks!

1
Missing tooltips noticed so far:

  • Classic block: all (I know this is TinyMCE but the current editor has working tooltips on all its buttons, and they should have tooltips in Gutenberg as well)
  • Video block: pencil icon
  • Audio block: pencil icon
  • Table block: edit table

2
More related to design/consistency /cc @jasmussen : some tooltips have title-cased text, some not. Regardless of language-specific preferences (as far as I know title-case is preferred in English, but it's not in many other languages), there should be some consistency. Since these are tooltips and not button labels, I'd go for no title-casing. Some example of the current tooltips:

Insert block
Change block type
Align left
Move Up
Move Down
Edit Gallery
Edit image
Remove Image
Grid View
List View

3
Slightly unrelated, I guess it should be addressed elsewhere: when I insert an image, by default it has no link. However, the link button says "Edit link". In the classic editor, the link button always says "Insert/edit link" to avoid this issue.

4
Unrelated but worth mentioning: the image edit link "modal" has some issues...

screen shot 2018-01-05 at 14 36 11

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 5, 2018

Contributor

I fixed the point "1". I tried to make the freeform block's tooltips look similarily to the default tooltips.

Contributor

youknowriad commented Jan 5, 2018

I fixed the point "1". I tried to make the freeform block's tooltips look similarily to the default tooltips.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 5, 2018

Contributor

Other points are unrelated and may be resolved separately.

Contributor

youknowriad commented Jan 5, 2018

Other points are unrelated and may be resolved separately.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jan 5, 2018

Contributor

Other points are unrelated and may be resolved separately.

Sure, will split them in a separate issue so this can be merged/closed.

Contributor

afercia commented Jan 5, 2018

Other points are unrelated and may be resolved separately.

Sure, will split them in a separate issue so this can be merged/closed.

@aduth

aduth approved these changes Jan 5, 2018

@@ -149,13 +150,14 @@ class GalleryBlock extends Component {
<MediaUploadButton
buttonProps={ {
className: 'components-icon-button components-toolbar__control',
'aria-label': __( 'Edit Gallery' ),
'aria-label': editButtonLabel,

This comment has been minimized.

@aduth

aduth Jan 5, 2018

Member

Would be nice if we could infer this from the tooltip passed to MediaUploadButton, but I guess the upload button is not always an icon button, so not so simple.

@aduth

aduth Jan 5, 2018

Member

Would be nice if we could infer this from the tooltip passed to MediaUploadButton, but I guess the upload button is not always an icon button, so not so simple.

This comment has been minimized.

@youknowriad

youknowriad Jan 5, 2018

Contributor

Actually, I was planning to revisit the MediaUploadButton to use render Props instead, that way we'd just use an IconButton in the render prop.

The thinking is that the MediaUpload should only deal with MediaUpload and not the UI to trigger the modal.

@youknowriad

youknowriad Jan 5, 2018

Contributor

Actually, I was planning to revisit the MediaUploadButton to use render Props instead, that way we'd just use an IconButton in the render prop.

The thinking is that the MediaUpload should only deal with MediaUpload and not the UI to trigger the modal.

Show outdated Hide outdated components/icon-button/index.js Outdated
@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jan 7, 2018

Contributor

I guess the following Trac ticket is relevant for the TinyMCE's tooltip: while the visual tooltips are translatable, the buttons aria-label attributes are not (not all, at least), see https://core.trac.wordpress.org/ticket/35710

Should it be tracked in a new issue?

Contributor

afercia commented Jan 7, 2018

I guess the following Trac ticket is relevant for the TinyMCE's tooltip: while the visual tooltips are translatable, the buttons aria-label attributes are not (not all, at least), see https://core.trac.wordpress.org/ticket/35710

Should it be tracked in a new issue?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 8, 2018

Contributor

Should it be tracked in a new issue?

Is this a TinyMCE issue? CC: @ephox-mogran

Contributor

jasmussen commented Jan 8, 2018

Should it be tracked in a new issue?

Is this a TinyMCE issue? CC: @ephox-mogran

@youknowriad youknowriad merged commit 4f80270 into master Jan 8, 2018

3 checks passed

codecov/project 39.72% (-0.25%) compared to 12ecd87
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/missing-tooltips branch Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment