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

Accessibility: Improved keyboard accessibility in BarGauge #59382

Merged
merged 5 commits into from Dec 1, 2022
Merged

Conversation

lpskdl
Copy link
Contributor

@lpskdl lpskdl commented Nov 28, 2022

What is this feature?

  • use button instead of div
  • reset default button styles

Why do we need this feature?

  • better keyboard accessibility

Who is this feature for?

  • everyone

Which issue(s) does this PR fix?:

Fixes #59284

Special notes for your reviewer:

@lpskdl lpskdl added this to the 9.3.0 milestone Nov 28, 2022
@lpskdl lpskdl requested a review from a team November 28, 2022 11:24
@lpskdl lpskdl requested a review from a team as a code owner November 28, 2022 11:24
@lpskdl lpskdl self-assigned this Nov 28, 2022
@lpskdl lpskdl requested review from a team, mckn, oscarkilhed and zoltanbedi and removed request for a team November 28, 2022 11:24
@lpskdl lpskdl added type/accessibility Accessibility problem / enhancement backport v9.3.x labels Nov 28, 2022
const { title } = this.props.value;
const styles = getTitleStyles(this.props);

if (!title) {
return (
<div style={styles.wrapper} onClick={onClick} className={className}>
<button style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<button style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>
<button type='button' style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>

This needs to have button type to prevent the default submit behavior in forms, quite unlikely for this specific component, but better be safe.

);
}

return (
<div style={styles.wrapper} onClick={onClick} className={className}>
<button style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<button style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>
<button type='button' style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

ok couple of things to tackle here:

since onClick is optional, this should only be a button if there's an onClick.

so the render function should probably look something like:

    if (onClick) {
      return (
        <button type="button" style={styles.wrapper} onClick={onClick} className={cx(clearButtonStyles(theme), className)}>
          {title && <div style={styles.title}>{title}</div>}
          {this.renderBarAndValue()}
        </button>
      );
    }

    return (
      <div style={styles.wrapper} className={className}>
        {title && <div style={styles.title}>{title}</div>}
        {this.renderBarAndValue()}
      </div>
    );

once we've done that, there's a couple of other issues:

  • focus styles aren't appearing when tabbing (they're cut off by the outer panel having overflow: hidden)
  • panel links menu appears in the wrong place when using the keyboard (this isn't as much of a big deal tbh...)

Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

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

We only want it to be a button when there is an onClick action right? Because with these changes, even if there is no onClick, you can tab into the BarGauge and when you hover over it there is a pointer icon.

@grafanabot grafanabot removed this from the 9.3.0 milestone Nov 30, 2022
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.3.0 milestone because 9.3.0 is currently being released.

@lpskdl lpskdl added this to the 9.3.1 milestone Nov 30, 2022
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.3.1 milestone because 9.3.1 is currently being released.

@grafanabot grafanabot removed this from the 9.3.1 milestone Nov 30, 2022
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

think we need a different solution 🤔 not sure what tho... i'll do some digging

@lpskdl lpskdl added this to the 9.3.2 milestone Dec 1, 2022
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

great job 🙌

@lpskdl lpskdl merged commit d9f697b into main Dec 1, 2022
@lpskdl lpskdl deleted the lpskdl/59284 branch December 1, 2022 13:45
grafanabot pushed a commit that referenced this pull request Dec 1, 2022
* Accessibility: Improved keyboard accessibility in BarGauge

* use appropriate elements when rendering bargauge

* added space for focus-visible outline border

* Revert "added space for focus-visible outline border"

This reverts commit 9b83fa3.

(cherry picked from commit d9f697b)
lpskdl added a commit that referenced this pull request Dec 7, 2022
…59653)

Accessibility: Improved keyboard accessibility in BarGauge (#59382)

* Accessibility: Improved keyboard accessibility in BarGauge

* use appropriate elements when rendering bargauge

* added space for focus-visible outline border

* Revert "added space for focus-visible outline border"

This reverts commit 9b83fa3.

(cherry picked from commit d9f697b)

Co-authored-by: Leo <108552997+lpskdl@users.noreply.github.com>
GuYounes pushed a commit to paul-wurth/BIXpert that referenced this pull request Feb 8, 2023
…rafana#59653)

Accessibility: Improved keyboard accessibility in BarGauge (grafana#59382)

* Accessibility: Improved keyboard accessibility in BarGauge

* use appropriate elements when rendering bargauge

* added space for focus-visible outline border

* Revert "added space for focus-visible outline border"

This reverts commit 9b83fa3.

(cherry picked from commit d9f697b)

Co-authored-by: Leo <108552997+lpskdl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A11y: Fix keyboard accessibility in packages/grafana-ui/src/components/BarGauge/BarGauge.tsx
7 participants