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

feat(NcPopover): provide a11y attributes to the trigger #5086

Merged
merged 8 commits into from Jan 18, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jan 17, 2024

☑️ Resolves

A problem

  1. NcPopover trigger button should have a11y attributes
    • aria-haspopup="<role>"
    • aria-expanded="<is shown>"
  2. Trigger button is provided via slot in place, we don't control it
    <NcPopover>
      <template #trigger>
        Any button here should have attrs
      </template>
    </NcPopover>
  3. We don't want to control all the places where NcPopover is used to make sure its trigger button has attributes.

This implementation might look a bit complex, but it helps to make a lot of popovers more accessible.

1. Provide NcPopover:trigger:attrs from NcPopover to its #trigger slot's content

So any component used as a trigger may receive the attrs, required to be on the trigger

A small component NcPopoverTriggerProvider is created to provide content only to the trigger, not to the content.

2. Inject it in NcButton

NcButton is the most popular NcPopover trigger.

This may look incorrect that NcButton "knows" something about the popover. But as it is a part of one library, and they are used together very often, it is a plain solution to cover a lot of cases. Without requirements to change anything in the apps.

Covers:

  • NcColorPicker, NcEmojiPicker, NcActions
  • Most of custom NcPopover usages with NcButton trigger in all Nextcloud apps
<NcPopover>
  <template #trigger>
    <!-- 🥳 Magically this button will have all the attrs --> 
    <NcButton>My custom popover</NcButton>
  </template>
  
  Popover Content
</NcPopover>

3. Provide the attributes as slot props

To manually set them in other cases

  • Covers NcDatetimePicker, NcUserBubble
  • Allow very custom popover triggers in some Nextcloud apps

And the same in NcColorPicker

<NcPopover>
  <template #trigger="{ attrs }">
    <!-- 🙂 No magic, but we can still just assign attrs --> 
    <button v-bind="attrs">My Very Custom Button</button>
  </template>
  
  Popover Content
</NcPopover>

<NcColorPicker v-slot="attrs">
  <button v-bind="attrs">My Very Custom Color Picker Button</button>
</NcColorPicker>

4. Add role prop to specify the role of the popover

See: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup

🖼️ Screenshots

Component / Trigger Popover
NcDateTimePicker image
image image
NcColorPicker image
image image
Custom NcColorPicker image
image .
NcEmojiPicker image
image image
Custom EmojiPicker image
image .
NcUserBubble image
image .
NcActions - Dialog image
image image
NcActions - Navigation image
image image
NcActions - Menu image
image image
NcPopover - Custom image
image .

What still have to be done manually in place

  • For custom trigger button without NcButton: v-slot="{ attrs }" + v-bind="attrs"
  • For NcPopover content:
    • popupRole on <NcPopover>
    • The same role on the content
    • aria-label or aria-labelledby for dialog content

This is where a warning in the debug mode helps to find the issue:

image

Alternative solution

Using DOM API by querySelector find any button in the #trigger slot and mutate it.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added enhancement New feature or request 2. developing Work in progress feature: popover Related to the popovermenu component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 17, 2024
@ShGKme ShGKme added this to the 8.5.0 milestone Jan 17, 2024
@ShGKme ShGKme self-assigned this Jan 17, 2024
@ShGKme ShGKme changed the title Fix/nc popover trigger attrs feat(NcPopover): provide a11y attributes to the trigger Jan 17, 2024
@ShGKme ShGKme added feature: datepicker Related to the date/time picker component feature: actions Related to the actions components feature: colorpicker Related to the colorpicker component feature: userbubble Related to the userbubble component feature: emoji picker Related to the emoji picker component labels Jan 17, 2024
@ShGKme ShGKme force-pushed the fix/nc-popover-trigger-attrs branch 4 times, most recently from 36dc429 to 5606dae Compare January 17, 2024 23:57
@ShGKme ShGKme force-pushed the fix/nc-popover-trigger-attrs branch from 5606dae to 7cf1c44 Compare January 18, 2024 00:05
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 18, 2024
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 18, 2024

I'll add tests later when most of a11y issues are fixed 👉👈

@ShGKme ShGKme marked this pull request as ready for review January 18, 2024 00:08
@ShGKme ShGKme force-pushed the fix/nc-popover-trigger-attrs branch from 7cf1c44 to b03dd57 Compare January 18, 2024 00:08
@ShGKme ShGKme requested a review from emoral435 January 18, 2024 00:14
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

Great PR which resolves so many places! 🥳🥳 Works!

Thanks a lot!

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/nc-popover-trigger-attrs branch from b03dd57 to 32af0a7 Compare January 18, 2024 16:12
@susnux susnux merged commit ddd5dd6 into master Jan 18, 2024
16 checks passed
@susnux susnux deleted the fix/nc-popover-trigger-attrs branch January 18, 2024 16:18
computed: {
shownProxy: {
get() {
return this.shown
Copy link
Contributor

Choose a reason for hiding this comment

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

@ShGKme Shouldn't this be

			return this.internalShown

? With internal it does not work for vue3.

@Pytal Pytal mentioned this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities enhancement New feature or request feature: actions Related to the actions components feature: colorpicker Related to the colorpicker component feature: datepicker Related to the date/time picker component feature: emoji picker Related to the emoji picker component feature: popover Related to the popovermenu component feature: userbubble Related to the userbubble component
Projects
None yet
4 participants