Skip to content

refactor(popover): add example with no header, refactor css a little, add some a11y attributes #837

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.

Sign up for GitHub

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

Jump to bottom
Merged
matthewcarleton merged 2 commits into from
Nov 9, 2018
Merged

refactor(popover): add example with no header, refactor css a little, add some a11y attributes #837

matthewcarleton merged 2 commits into from
Nov 9, 2018

Conversation

@mcoker
Copy link
Contributor

@mcoker mcoker commented Oct 24, 2018

fixes #834

@patternfly-build
Copy link
Collaborator

patternfly-build commented Oct 24, 2018 •
edited
Loading

Deploy preview for pf-next ready!

Built with commit a4ddb23

https://deploy-preview-837--pf-next.netlify.com

jameslau
jameslau reviewed Oct 25, 2018
View reviewed changes
Copy link
Member

@jameslau jameslau left a comment

Choose a reason for hiding this comment

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

The popover looks good. Code is clean with the CSS. I'll let @jgiardino confirm on the ARIA attributes before this is merged.

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Oct 29, 2018

looks good! I ran this through voiceover and noticed that the examples with header announce the entirety of the popover header/content but the example with no header just announces the aria-label on the popover. This looks like it's caused by aria-labelledby vs aria-label
We can add aria-describedby on the popover to fix this. My question is do we add it for just the header-less examples or all of them for consistency?

@mcoker
Copy link
Contributor Author

mcoker commented Oct 29, 2018

Thanks @matthewcarleton! I'm a little confused as to when to use aria-describedby. The examples I've read indicate that it isn't necessarily the body of the popover, but a longer description of what the popover is. Maybe those are the same thing?

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Oct 29, 2018

@michael-coker ya good point. I'd expect the same experience with/without the header but I'm not sure in our case if the description would be the same as the body or not. It depends if the expectation from a screen reader is to then continue to read the content after they've had the description announced. I'd assume they'd only want the content announced and not a description of it, but I think we'd have to do a bit more research to really know for sure.

@mcoker
Copy link
Contributor Author

mcoker commented Nov 2, 2018

@matthewcarleton I compared the popover to the modal, and I believe their accessibility attributes should be the same. Each of the modal examples have aria-describedby that points to the body of the modal, so I added those here. Now VO reads the entire contents of the popover in all of the examples. However, another thing I noticed (that's present in the header-less modal example, too) is that the close button isn't read by the screenreader when aria-label is used on the parent.

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Nov 5, 2018 •
edited
Loading

screen shot 2018-11-05 at 9 53 23 am

hmmm, this is what I hear with VO. Is it the same for you?

Also I'm noticing that ones with titles announce everything twice b/c they have 'aria-labelledby' and 'aria-describedby'. It seems aria-describedby is redundant if aria-labelledby is present but we'd have to test further to confirm.

@jgiardino
Copy link
Contributor

jgiardino commented Nov 6, 2018

These updates match what I'd expect, which is based on the documentation provided here: https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html

I also consider popovers and modals to be similar enough to have the same keyboard and screen reader experience.

I'm also a little confused by aria-describedby, but until we have an interactive JS implementation, it will be hard to judge whether we should keep this or not. So for now, I'd say lets keep it, and we can later decide if it hurts or helps the experience. I did come across this modal example referenced on inclusive-components.design, which does use aria-describedby. I tested this in VO, and it works great.

When reviewing our popovers and modals in VO, I found that I'm not able to access anything except the title and close button, which I thought was odd. I found this modal example which talks about adding role="document" to enable access to dialog contents in NVDA. Maybe a similar thing is happening in VO? On the other hand, our implementation is really similar to the one I mentioned above, and it works as expected. I'm not able to pinpoint where the difference might be (other than one is fully working and ours is static).

I didn't notice the behavior that @matthewcarleton described.

jgiardino
jgiardino approved these changes Nov 6, 2018
View reviewed changes
Copy link
Contributor

@jgiardino jgiardino left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@mcoker
Copy link
Contributor Author

mcoker commented Nov 6, 2018

@matthewcarleton

this is what I hear with VO. Is it the same for you?

Yep. That's what I would expect. Do you expect to hear something different?

Also I'm noticing that ones with titles announce everything twice b/c they have 'aria-labelledby' and 'aria-describedby'. It seems aria-describedby is redundant if aria-labelledby is present but we'd have to test further to confirm.

I hadn't noticed that, but I can confirm it's happening on my side, too. I just tested our modals, and it's happening there as well since they use the same aria properties.

@jgiardino after VO reads the label and body in either a popover or a dialog with a title (when the dialog reads aria-labelledby), if you wait a second, you don't hear VO read the body/description again?

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Nov 9, 2018

Merging this with the effort of a11y moving to a new issue because this also affects modal.

@matthewcarleton matthewcarleton merged commit 3cf77b5 into patternfly:master Nov 9, 2018
@mcoker mcoker mentioned this pull request Nov 9, 2018
Closed
@tlabaj tlabaj mentioned this pull request Nov 13, 2018
Merged
srambach referenced this pull request in srambach/patternfly-next Nov 14, 2018
@srambach
Merge branch 'master' of https://github.com/patternfly/patternfly-next
665f347 
* 'master' of https://github.com/patternfly/patternfly-next:
  fix(brand): remove min-height in example (#929)
  Breadcrumbs 832 (#912)
  chore(colors): update palette and support some new charting colors (#923)
  refactor(popover): add example with no header, refactor css a little, add some a11y attributes (#837)
  fix(page): fix padding var order, update dark attr selector (#908)
  fix(grid): add min-width/height 0 to override min-content (#921)
  chore(hbs): add contains helper to handlebars (#904)
  remove sequence folder (#903)
  fix(nav): update spacing for nav system (#890)
  chore(workspace): add ability to expand/collapse code areas (#896)
  feat(width): Introduce width utility (#820)
  enhancement(switch): Update the Switch to reflect the latest design (#877)
  chore(card): add oxford comma
  chore(card): Expand card implementation (#842)
  chore(node version): add nvm support, at Node >= v8.0.0 (#855)
  fix(naming): rename template styles.scss into componentName.scss (#861)
  fix(nav): Update expandable nav toggle's a11y attributes (#746) (#876)
  fix(page-layout): move nav styles to component (#884)
  fix(page-layout):revises condensed masthead  mobile view (#879)
  fix(workspace): Change bg of preview body to allow component bg to show (#874)
@mcoker mcoker deleted the issue-834 branch December 16, 2019 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jameslau jameslau jameslau left review comments

@jgiardino jgiardino jgiardino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mcoker @patternfly-build @matthewcarleton @jgiardino @jameslau