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

BorderBoxControl: Add Tooltips around split border controls #42661

Closed
wants to merge 1 commit into from

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Jul 25, 2022

Related:

What?

Add basic tooltips around each individual side's border control within the BorderBoxControl

Why?

Assists speech recognition software users in identifying the names of individual border fields. See: #42095 (comment)

How?

  • Follows the same approach to adding Tooltips as used by BoxControl and BorderRadiusControl components
  • Individual border controls are wrapped in a Tooltip with an inner div wrapper.

Known Issues

The recent addition of a Tooltip to the border color and style picker button means when it is hovered two Tooltips are displayed at once.

Screen Shot 2022-07-25 at 2 24 58 pm

Possible options
  1. Remove the tooltip from the color/style picker button
  2. Revert the recent change rendering the color style picker dropdown as a UnitControl prefix. This means we could wrap the UnitControl with a tooltip independently to the dropdown.
  3. Extend InputControl to support optionally wrapping its InputField with a tooltip.

Testing Instructions

  1. Start up Storybook and visit the BorderBoxControl page.
  2. Click the "Unlink sides" button to see the individual side border controls.
  3. Hover over the various fields and confirm appropriate tooltips display
  4. Notice the concurrent tooltips when hovering a side's border color/style picker button
  5. Confirm the BorderBoxControl functions the same as on trunk despite the tooltips.

Screenshots or screencast

Screen.Recording.2022-07-25.at.3.13.08.pm.mp4

@aaronrobertshaw aaronrobertshaw marked this pull request as draft July 25, 2022 05:19
@aaronrobertshaw aaronrobertshaw self-assigned this Jul 25, 2022
@aaronrobertshaw aaronrobertshaw added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Jul 25, 2022
@aaronrobertshaw aaronrobertshaw requested review from mirka and ciampo July 25, 2022 05:31
@aaronrobertshaw
Copy link
Contributor Author

@ciampo or @mirka do you have any suggestions or ideas on the best course of action to address the issue noted in the PR description relating to concurrent tooltips?

The recent addition of a Tooltip to the border color and style picker button means when it is hovered two Tooltips are displayed at once.

Possible options

  1. Remove the tooltip from the color/style picker button
  2. Revert the recent change rendering the color style picker dropdown as a UnitControl prefix. This means we could wrap the UnitControl with a tooltip independently to the dropdown.
  3. Extend InputControl to support optionally wrapping its InputField with a tooltip.

@ciampo
Copy link
Contributor

ciampo commented Jul 25, 2022

@ciampo or @mirka do you have any suggestions or ideas on the best course of action to address the issue noted in the PR description relating to concurrent tooltips?

The recent addition of a Tooltip to the border color and style picker button means when it is hovered two Tooltips are displayed at once.

This is an interesting one!

If we need an quick solution, since we recently assessed that BorderControl is never used in isolation, we could simply

  1. Remove the tooltip from the color/style picker button

More in general, this looks a bit like the problem we're facing with labelling nested groups of inputs, discussed in #42630. And similarly to the conversation in #42630, I believe that we could come up with a way display a Tooltip optionally (using context) only if it's not nested inside another tooltip.

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the quick feedback @ciampo 🙇

I believe that we could come up with a way display a Tooltip optionally (using context) only if it's not nested inside another tooltip.

Agreed, this appears to be the way to go longer term. I was hoping to try and improve the BorderBoxControl in the short term while we implement such as solution.

Remove the tooltip from the color/style picker button

This also seems like a quick win however I wasn't sure how we'd evaluate the tradeoff between prioritising the BorderBoxControl side field tooltips vs the color/style picker tooltip, given issues such as: #41799

Do you think there is more to gain by removing the tooltip from the color/style picker button in favour of tooltips on the split border controls?

@ciampo
Copy link
Contributor

ciampo commented Jul 25, 2022

This also seems like a quick win however I wasn't sure how we'd evaluate the tradeoff between prioritising the BorderBoxControl side field tooltips vs the color/style picker tooltip, given issues such as: #41799

Do you think there is more to gain by removing the tooltip from the color/style picker button in favour of tooltips on the split border controls?

Good point — the tooltip on the color/style picker button seems to be a useful enhancement, and it would a pity to remove it.

I think that we may be focusing on the wrong issue here: if we require so many tooltips for BorderBoxControl, this could be a symptom that the component doesn't currently provide a good and intuitive UX. We may want to assess if there's a better, more intuitive way to represent these controls to the user, as retro-fitting accessibility improvements is a very hard task and never the best choice.

For example, couldn't we display some label text above each BorderControl instead of adding a tooltip ?

@aaronrobertshaw
Copy link
Contributor Author

I think that we may be focusing on the wrong issue here: if we require so many tooltips for BorderBoxControl, this could be a symptom that the component doesn't currently provide a good and intuitive UX.

Taking the primary use case in our editors, if we had access to the merged global styles values in the block editor, we'd be able to set a meaningful and accurate color to the control by default. I think this would help improve how intuitive the control is.

This issue of the nested Tooltips has only come about due to me recently moving the border color/style dropdown into a UnitControl prefix. If we went back to the old approach, we'd have control over both separately. Would this give us the best all-around option? We get the visualizer, compact and clean UI, control over tooltips for the appropriate elements etc. I appreciate that leveraging native HTML labelling is the gold standard, though.

For example, couldn't we display some label text above each BorderControl instead of adding a tooltip?

I believe one of the original goals for the BorderBoxControl was to manage the explosion of border-related controls in the editor sidebars. The "visualizer" was a nice touch but does limit the real estate we have in terms of labelling or at least makes things cluttered when labelled.

Screen Shot 2022-07-25 at 6 29 52 pm

There is some ongoing work around adding UI to the spacing presets in the editor. It is based off the BoxControl but stacks the expanded/unlinked controls vertically in order to accommodate sliders. We could follow a similar approach here.

Screen Shot 2022-07-25 at 7 08 26 pm

Personally, I don't find this as aesthetically pleasing as the border control with the visualizer.

@ciampo
Copy link
Contributor

ciampo commented Jul 25, 2022

Taking the primary use case in our editors, if we had access to the merged global styles values in the block editor, we'd be able to set a meaningful and accurate color to the control by default. I think this would help improve how intuitive the control is.

Maybe, but it wouldn't really impact the UX of the BorderBoxControl component

This issue of the nested Tooltips has only come about due to me recently moving the border color/style dropdown into a UnitControl prefix. If we went back to the old approach, we'd have control over both separately. Would this give us the best all-around option? We get the visualizer, compact and clean UI, control over tooltips for the appropriate elements etc. I appreciate that leveraging native HTML labelling is the gold standard, though.

I'm not sure how moving the border color/style button back to being a sibling of UnitControl would help here — from what I understand, the Tooltip that is being added in this PR wraps BorderControl, and therefore it would wrap the border/color style button regardless of it being a prefix of UnitControl or not ?

Personally, I don't find this as aesthetically pleasing as the border control with the visualizer.

I agree with you 💯 % with respect to what approach is the most aesthetically pleasing.

What I'm trying to say is that, in case we can't figure out a good approach to improving the accessibility (or if the approach is very hard / convoluted), it may be a symptom that we may need to partially reconsider the way this component is structured. In general, I believe that it's much harder to retro-fit accessibility into a beautiful design, than it is to improve the design of an accessible control.

@aaronrobertshaw
Copy link
Contributor Author

aaronrobertshaw commented Jul 25, 2022

I'm not sure how moving the border color/style button back to being a sibling of UnitControl would help here — from what I understand, the Tooltip that is being added in this PR wraps BorderControl, and therefore it would wrap the border/color style button regardless of it being a prefix of UnitControl or not ?

My thinking here was that we could offer better tooltips via the BorderControl itself (one around the border color/style button, another for the input). Leveraging something like that, we wouldn't then be wrapping the entire BorderControl in a single Tooltip.

@ciampo
Copy link
Contributor

ciampo commented Jul 26, 2022

I'm not sure how moving the border color/style button back to being a sibling of UnitControl would help here — from what I understand, the Tooltip that is being added in this PR wraps BorderControl, and therefore it would wrap the border/color style button regardless of it being a prefix of UnitControl or not ?

My thinking here was that we could offer better tooltips via the BorderControl itself (one around the border color/style button, another for the input). Leveraging something like that, we wouldn't then be wrapping the entire BorderControl in a single Tooltip.

Just to make sure I understand correctly what you're proposing — for example, the bottom border's BorderControl would display two tooltips:

  • one around the border/color style button (e.g. "Bottom border color and style picker")
  • one around the UnitControl (e.g. "Bottom border width")

Is that what you had in mind?

@aaronrobertshaw
Copy link
Contributor Author

Is that what you had in mind?

Yes, I believe it's one option we can consider at least.

@mirka
Copy link
Member

mirka commented Jul 26, 2022

Possible options
  1. Remove the tooltip from the color/style picker button
  2. Revert the recent change rendering the color style picker dropdown as a UnitControl prefix. This means we could wrap the UnitControl with a tooltip independently to the dropdown.
  3. Extend InputControl to support optionally wrapping its InputField with a tooltip.

I don't know what our ultimate labeling strategy is going to be in the long-term, but in the short-term, out of the possible options, I think we can go with number 3. It's simple, and it will be useful for any other InputControl-based component having an interactive prefix/suffix.

@aaronrobertshaw
Copy link
Contributor Author

but in the short-term, out of the possible options, I think we can go with number 3. It's simple, and it will be useful for any other InputControl-based component having an interactive prefix/suffix.

I've drafted a PR adding the ability to wrap the inner InputField of an InputControl with a Tooltip. I'm not sure it will satisfy our needs relating to the BorderBoxControl though.

When in the split view, we wish to indicate each BorderControl is for a particular side. Yet, in the context of the BorderControl we need to indicate the input field is for the border width.

It's been raised that concatenating translated strings would yield poor results, so combining the border side with the width
label (e.g. Left border width or Left border - Border width ) isn't really doable.

Would allowing customised tooltip text help? How would we ensure it's useful for speech recognition software users etc.?

@ciampo
Copy link
Contributor

ciampo commented Jul 28, 2022

Thank you for opening #42726! I left my replies directly in the PR

@ciampo
Copy link
Contributor

ciampo commented Aug 10, 2022

With the PR adding support for Tooltip to InputControl in a good state, it's probably time to decide if we want to pursue that option or if need to find a different approach.

If this still the main option that we're considering?

I'm not sure how moving the border color/style button back to being a sibling of UnitControl would help here — from what I understand, the Tooltip that is being added in this PR wraps BorderControl, and therefore it would wrap the border/color style button regardless of it being a prefix of UnitControl or not ?

My thinking here was that we could offer better tooltips via the BorderControl itself (one around the border color/style button, another for the input). Leveraging something like that, we wouldn't then be wrapping the entire BorderControl in a single Tooltip.

Just to make sure I understand correctly what you're proposing — for example, the bottom border's BorderControl would display two tooltips:

  • one around the border/color style button (e.g. "Bottom border color and style picker")
  • one around the UnitControl (e.g. "Bottom border width")

Is that what you had in mind?

Yes, I believe it's one option we can consider at least.

@aaronrobertshaw
Copy link
Contributor Author

Closing this old PR. If we come back to this or a similar approach it can be reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants