Site setup content types add new - CMS UI - a11y#8058
Site setup content types add new - CMS UI - a11y#8058
Conversation
Wagner3UB
requested review from
davisagli,
giuliaghisini,
pnicolli,
stevepiercy and
wesleybl
stevepiercy
left a comment
stevepiercy
left a comment
There was a problem hiding this comment.
LGTM. Just the news. Also merge #8077 to fix linkcheck.
| @@ -0,0 +1 @@ | |||
| Fixed accessibility issues in ModalForm: focus management, aria-live announcements and aria-modal support @Wagner3UB | |||
There was a problem hiding this comment.
| Fixed accessibility issues in ModalForm: focus management, aria-live announcements and aria-modal support @Wagner3UB | |
| Fixed accessibility issues in `ModalForm` with focus management, `aria-live` announcements, and `aria-modal` support. @Wagner3UB |
github-project-automation
bot
moved this from In progress
to Review in progress
in RedTurtle priorities
github-project-automation
bot
moved this from In progress
to Review in progress
in Accessibility
|
@Wagner3UB Perhaps it would be better to convert |
|
The bad thing about waiting for the conversion to a functional component is that the backport to Volto 18 will become more difficult. @sneridagh said that we cannot do the backport of the conversion to functional components. But the changes here can work for Volto 18. |
|
@wesleybl From my perspective, the conversion to a functional component is a separate matter. We merge the one that is ready first, and then update the other one. |
@plone/volto-team What do you think, folks? I can sustain these changes for Volto 18 and do the refactor that @wesleybl suggested. More work to do, but, can be beneficial. |
|
@Wagner3UB I think to make backporting to Volto 18 easier, you can continue here and then we'll do the conversion to functional. |
| * @extends Component | ||
| */ | ||
| class ModalForm extends Component { | ||
| headerId = `modal-title-${Math.random().toString(36).slice(2)}`; |
There was a problem hiding this comment.
It's best to use a static counter to avoid collisions. And it should also be inside the constructor:
static idCounter = 0;
constructor(props) {
this.headerId = `modal-title-${++ModalForm.idCounter}`;
}| @@ -215,6 +226,22 @@ class ModalForm extends Component { | |||
| * @param {Object} prevState | |||
| */ | |||
| async componentDidUpdate(prevProps, prevState) { | |||
There was a problem hiding this comment.
There is no await. You can remove the async.
| aria-labelledby={this.headerId} | ||
| aria-modal="true" | ||
| > | ||
| <Header id={this.headerId} aria-live="assertive" aria-atomic="true"> |
There was a problem hiding this comment.
There's already an aria-live="assertive" in the div above. It's best to remove it from here:
| <Header id={this.headerId} aria-live="assertive" aria-atomic="true"> | |
| <Header id={this.headerId}> |
| {/* Sentinel: closing the modal when the user tabs past the last element */} | ||
| {onCancel && ( | ||
| <button | ||
| type="button" | ||
| onFocus={() => { | ||
| if (this.announceRef.current) { | ||
| this.announceRef.current.textContent = | ||
| this.props.intl.formatMessage(messages.dialogClosed); | ||
| } | ||
| onCancel(); | ||
| }} | ||
| style={{ position: 'absolute', opacity: 0 }} | ||
| /> |
There was a problem hiding this comment.
I find it strange to close the modal with a tab. I think there should be an explicit action to close it. But the way it exits the modal is also strange. I think it should stay in a loop inside the modal.
| opacity: 0, | ||
| }} | ||
| /> | ||
| <Modal |
4 participants
Accessibility improvements for ModalForm and Content Types panel
Fixed several accessibility issues in the ModalForm component and the Content Types control panel:
Issue: #5141