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

Make tooltips dismissable #8147

Closed
wants to merge 1 commit into from
Closed

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jul 23, 2018

Work in progress, please don't merge.

This PR is a first attempt to improve the tooltips behaviour after #8033. As mentioned there and in the related issue, tooltips should persist when hovered. However, as correctly pointed out, they should also be dismissable by users.

WCAG reference:
Success Criterion 1.4.13 Content on Hover or Focus
https://www.w3.org/TR/WCAG21/#content-on-hover-or-focus
https://www.w3.org/WAI/WCAG21/Understanding/content-on-hover-or-focus.html
there are 3 items to address to meet the requirement:

  • dismissable
  • hoverable
  • persistent

More details on #8033 and the related issue.

By making use of onMouseDown this PR seeks to solve two issues:

  • makes the tooltips dismissable when clicking on them
  • prevents the click event to be passed to the button and trigger the associated action
  • also adds a cursor: default style to the tooltip

Todo:

  • ideally, tooltips should be dismissable also when pressing Escape; honestly, I have no idea how to implement it
  • seems to me the persistent tooltips work pretty well except for the block side controls: the Move Up, Move Down, and More Options buttons. Worth noting these controls may completely change if Display block tools underneath the block, instead of to the sides #6224 gets in (they would be moved underneath the block). Otherwise, there will be the need of further changes:
    • when hovering a block and then hovering the movers or the ellipsis button, the tooltips don't persist because the button themselves are not rendered; seems to me this happens because the hovered left or right areas can be null when hovering the tooltips (see screenshot below)
    • the tooltips on the block movers should be positioned differently depending if the block has a wide/full alignment or not

Any help would be greatly appreciated.

screen shot 2018-07-23 at 15 20 45

@afercia afercia added the [Status] In Progress Tracking issues with work in progress label Jul 23, 2018
@afercia afercia requested a review from youknowriad July 23, 2018 18:05
@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 23, 2018
@aduth
Copy link
Member

aduth commented Sep 13, 2018

Is it likely this will be resumed, or can it be closed?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018
@aduth
Copy link
Member

aduth commented Oct 15, 2018

Closing as stale without response. It can be reopened if development is expected to continue, or a new pull request can be submitted.

@aduth aduth closed this Oct 15, 2018
@aduth aduth deleted the update/improve-tooltips-behavior branch October 15, 2018 20:40
@afercia
Copy link
Contributor Author

afercia commented Oct 16, 2018

Actually, I was waiting for some feedback 🙂

Any help would be greatly appreciated.

Noting that, technically, tooltips are not WCAG compliant because they should be dismissable by the user. I'm not so skilled to propose solutions other than the one in this PR. If anyone can come up with better ideas, please do. Thanks.

@aduth aduth restored the update/improve-tooltips-behavior branch October 17, 2018 13:11
@aduth
Copy link
Member

aduth commented Oct 17, 2018

Stale label application is indiscriminate, with the subsequent closure delayed to allow time for corrections. Apologies for the missed subnote of the comment.

Reopening, and I'll follow-up with thoughts on how to proceed.

@aduth aduth reopened this Oct 17, 2018
@gziolo gziolo removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Oct 31, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

To the question of handling escape, it seems like one way to achieve this would be through rendering a <KeyboardShortcuts /> element from the Tooltip component when it's visible, which would respond to the Escape press by having itself be made hidden (managing isOver state, which at that point ought to be considered for rename).

To avoid an issue of conflicting Escape handling between it and that of the "clear selected block" behavior, it would probably need to be implemented as wrapping the children (button or whatever else the tooltip is attached to) rendered by Tooltip, leveraging the feature of KeyboardShortcuts to "capture key events which occur on or within the children" and stopping propagation to avoid other handling.

I could help assist with this. There are conflicts which will need to be resolved first. I could look to resolve them too.

@@ -119,7 +119,9 @@ class Tooltip extends Component {
createToggleIsOver( eventName, isDelayed ) {
return ( event ) => {
// Preserve original child callback behavior
this.emitToChild( eventName, event );
if ( event.type !== 'mousedown' ) {
Copy link
Member

Choose a reason for hiding this comment

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

What about click ? Noting since it's mentioned in the text of the original comment.

@gziolo
Copy link
Member

gziolo commented Feb 1, 2019

@aduth and @afercia - do you plan to update this PR? I'm marking it as Stale to make reviewing 200+ PRs easier. However feel free to remove the label as soon as this PR gets refreshed.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 1, 2019
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Apr 24, 2019
@afercia
Copy link
Contributor Author

afercia commented Apr 24, 2019

Created a tracking issue for this PR: #15145 with "Needs dev" label

@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

Thank you @afercia. Let's close this one in the meantime 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants