perf(a11y): Make modals accessible #348
Conversation
|
I've created a PR and added a first draft how I would approach modals. @klues Can you please have a look about the current changes? Opening/closing modals is possible, style needs to be updated, but I wanted to verify/discuss this approach before making changes in modal-related all files. I've had a look in all modal components. All of them look like this, right? File: src/vue-components/modals/addMultipleModal.vue <div class="modal">
<div class="modal-mask">
<div class="modal-wrapper">
<div class="modal-container" @keyup.27="$emit('close')" @keyup.ctrl.enter="save()"> ... </div>
</div>
</div>I think we can drop CSS classes 'modal', 'modal-mask', and 'modal-wrapper'. |
|
Thanks for your contribution! 👍
correct.
Yes, currently the CSS is in I like the approach, looks good to me! What we should consider to add to
Edit Element Modal: |
|
For the headers, probably also some generic code makes sense - possibility to simply pass a header text for the default case. Related to this: a generic modal for replacing the browser-internal |
Yes, I agree.
Yes, we can add that. We could do it in two ways, depending on what we want to achive:
Do you mean to pass, for instance, two functions like
I think it might make sense to implement <template>
<modal>
...
<slot #footer>
<button @click="handleCancel">Cancel</button>
<button @click="handleOk">Ok</button>
<button @click="handleCustomAction1">Custom Action 1</button>
<button @click="handleCustomAction1">Custom Action 2</button>
</slot>
</modal>
</template>Furthermore, we could provide modal-related CSS classes to allow easier/quicker design and less repetition, e.g. <button class="modal-btn row-2 ???">Custom Action 1</button>For standard cases, where only Cancel and OK are required, one could maybe pass handlers like that. <template>
<modal :modal-cnl="handleCancel" :modal-ok="handleOk">
...
</template>I think this is a more understandable design when customizing functionalities of modals, rather than passing arrays around. Furthermore, it keeps the modal component cleaner and separates the responsibilities more appropriately. At least, from my current understanding. Is there a difference between the
|
Yes, I think both suggestions are really good and make sense! I guess, the header text, like "Edit grid item" from the previous screenshot should be passed as a parameter to the modal. Right? |
I would opt for this. I think almost every modal should have the same possibility to close it. An exception maybe would be modals which require some user input, but I don't think we have something like this until now ("confirm" modals should evaluate "X" as "cancel"). But if we need it, I would simply hide the "X" via a property.
OK, then
Yes, currently there's only the edit element modal with 4 buttons, I think all other ones have two buttons. So for now, we can stick with your suggestion and adapt/extend it, if needed at some point in the future.
As said,
If we would have more modals with many buttons, I would like the idea to not have to think about the layout every time - but a said, if it's only the exception like now, I agree with you.
Same thing.
Correct. But in this case, it will be a custom header, since it also shows a small image and the label of the element you're currently editing. And it reminds me of another thing that would be good in the global |
|
Another question came up: should the default modal implementation contain the "X" for closing and a button "Cancel" for canceling? If so, are they any different or do they behave the same? Do we need both? |
|
I think both should be part of the default modal implementation, but should result in the same action, both are simply closing the modal without additional actions. I think it makes sense to have both, since some users will search for the "X" at the top and some for the possibility to "cancel" at the buttons. Other implementations like bootstrap are doing it in the same way. |
|
I'm wondering about <div class="modal-footer">
<div class="button-container">
<button @click="$emit('close')" :title="$t('keyboardEsc')">
<i class="fas fa-times"/> <span>{{ $t('cancel') }}</span>
</button>
<button @click="save()" :title="$t('keyboardCtrlEnter')">
<i class="fas fa-check"/> <span>{{ $t('insertWords') }}</span>
</button>
</div>
</div>This snippet is from I've seen that the Cancel and OK button, usually look the same, with almost identical implementation. However, in the This is my current suggestion: <template>
<dialog ref="modal">
<div class="modal-header">
<span>{{ title }}</span>
<slot name="header"></slot>
<button @click="close"><i class="fas fa-times"></i></button>
</div>
<div class="modal-body"><slot></slot></div>
<div class="modal-footer">
<slot name="footer">
<slot name="cancel-button">
<button @click="close" :title="$t('keyboardEsc')">
<i class="fas fa-times">{{ $t('cancel') }}</i>
Cancel
</button>
</slot>
<slot name="ok-button">
<button :title="$t('keyboardCtrlEnter')">
<i class="fas fa-check">{{ $t('ok') }}</i>
Ok
</button>
</slot>
</slot>
</div>
</dialog>
</template>This way, one add custom content to the header, and we can make Since Cancel and OK are almost the same for all components, it might make sense to allow to make either the whole "footer" or "cancel" and "ok" buttons individually customizable via slots. Cancel functionality is the same for all components, so it can be part of the |
|
Questions answered personally ;) |
|
Do you have an idea (a klue 🤓) what of the previous changes might have caused this error message? mainVue.js:117 TypeError: Cannot read properties of null (reading 'id')
at Proxy.render (gridEditView.vue?./node_modules/vue-loader/lib/loaders/templateLoader.js??ruleSet%5B1%5D.rules%5B1%5D!./node_modules/vue-loader/lib/index.js??vue-loader-options:211:37)
at Vue._render (vue.esm.js:2581:28)
at VueComponent.updateComponent (vue.esm.js:3021:27)
at Watcher.get (vue.esm.js:4205:33)
at Watcher.run (vue.esm.js:4281:30)
at flushSchedulerQueue (vue.esm.js:3267:17)
at Array.eval (vue.esm.js:3902:20)
at flushCallbacks (vue.esm.js:3824:18) |
|
To which lines oft code does the browser take you, if you click the link in the message in the console? |
|
Initially I thought, the error may be caused by some changes in This is the line. I didn't click on it initially but looked it up manually. But the code loaded in the browser uses different sources..
Still, it's not obvious for me. I'd have to have a more detailed look. |
|
This is the entire console output. There is another line causing an error, that I didn't mention before. Maybe it helps..
|
|
The error seems to happen in |
|
|
This is the relevant line. Maybe it's caused by the fact that the parent/root component inside of the |
|
I assume it's this line where the The property |
close #210