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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use aria attributes in dialog markup #6402

Merged

Conversation

@BatJan
Copy link
Contributor

commented Sep 22, 2019

Prerequisites

  • I have added steps to test this contribution in the description below

This PR addresses issue no. 157 from the #5277

Description

This PR adds some modification to the markup of the overlay markup in order to improve its accessibility - I have followed the guideline from https://bitsofco.de/accessible-modal-dialog/ for the markup pattern.

Once this is done there will still be some outstanding issues concerning the focus lock, which I'm still awaiting some feedback on in #4526 since using the suggested polyfill will handle much of the heavy listing for us very easily - Of course we should aim to make a generic directive or whatever makes sense so we can easily reuse it when dealing with different kinds of dialogs/overlays. But I've purposefully left this out of this scope until further discussion has been done in the referenced PR 馃槂

What's been done

  • Added role="dialog" to the container
  • Added aria-labelledby="umb-overlay-title" to the container
  • Added aria-describedby="umb-overlay-description" to the container
  • Changed the <h4> containing the title to <h1> since it will be the primary headline in the overlay context (None of the stuff it lays on top of should be possible to navigate to - Read my above comment 馃槈 ) - Also added an id="umb-overlay-title" so the aria-labelledby attribute is linked to correctly to the title/headline
  • Added id="umb-overlay-description id to the subtitle paragraph so the aria-describedby knows where to find some relevant data. If this description is empty it's not rendered and it should be OK since the aria-describedby isoptional as far as I know and remember
  • I have changed some semantics for some areas containing <h5> elements and where <div> elements wrapped some text. <h5> is changed to <h2> and the div's have been change to <p> elements. The styling has been updated accordingly

I was having a play with the overlay to figure out if I would be able to deal with reassigning the focus back to the triggering element - However this will require some more effort since it's not only a "open dialog" / "close dialog" scenario that is in play here. There is a bit more to it and think it's better to do an independent PR for this.

When I'm looking at the content in the overlay component I'm having a feeling that some of it might not be used anymore? Those scenarios I have been able to find where the overlay is being used does not trigger any of the conditions needed to render the div with the class of .umb-overlay__item-details - When I tried removing all the ng-if attribute and add dummy data it looked like some leftover stuff that does not belong - But I could be wrong of course 馃槂 But if I'm not I think it might be better to remove it? Same goes for the part in the footer that is only displayed if model.confirmSubmit.show is true... but this part has not been wrapped in nice classes for easier styling etc. and looks pretty outdated to me as well.

I also noticed that some of the texts may be a tad too generic in terms of providing a better non-visual context but I think that's for another PR do resolve maybe.

BatJan added 5 commits Sep 22, 2019
鈥dline and description so things can be paired up nicely.
鈥n it will lay on top of everything else and by our only context, which is why adding it in a h1 is perfectly fine
鈥 and hide the icons from screen readers
鈥t is visual in the dialog
@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Thanks Jan, we'll have a look soon! 馃憤

@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

@nul800sebastiaan It's still in draft mode though 馃槈 I need to have a look at managing focus when a dialog. There is some trickery if the dialog enables you to delete an item then the focus needs to be handled a bit differently - Once I've nailed this part then I'll change the state of the PR 馃槂

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Ah yes, didn't see it was a draft on mobile!

@BatJan BatJan marked this pull request as ready for review Sep 22, 2019
@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@nul800sebastiaan - Any chance this will be merged during Aarhus hackathon? :D

@nul800sebastiaan nul800sebastiaan merged commit 2cd92e7 into umbraco:v8/dev Sep 29, 2019
1 check passed
1 check passed
Cms 8 Continuous Build #5184 succeeded
Details
@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

Thanks @BatJan, this is all good. I am also not sure if the ones you mentioned are still in use I'm afraid :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.