fix(menubar): menubar and table menus a11y by ShGKme · Pull Request #5218 · nextcloud/text

ShGKme · 2024-01-09T18:47:40Z

📝 Summary

Resolves: [BITV]: Remove unneeded roles menuitem and menu #5216
Resolves: [BITV] 9.4.1.2/3.16 - Custom editing menubar should be implemented using role=toolbar and not role=menubar. (2) #3911

Currently aria-* and role attrs in the MenuBar are set incorrectly. These attributes are already covered by @nextcloud/vue. This PR removes setting attributes from the Text and adjust using NcAction* components.

There are quite a lot of changes. Separated with 💙 into small commits.

Before	After

🚧 TODO

fix: apply a11y attrs for session list trigger

Migrate from deprecated slot syntax
Use a11y attrs from NcPopover on a custom button

No visual changes

👀 Place	🏚️ Before	🏡 After

fix(menubar): remove unneeded roles `menu`, `menuitem`

All these roles are correctly set by NcAction* components when needed.
Setting these roles in mixins and menu bar puts them on wrong elements.

👀 Place	🏚️ Before	🏡 After

fix(menubar): remove incorrect `aria-haspopup`

It must have string "true"/"menu" value
It must be placed on the button itself
It is already correctly set by NcActions and NcPopover

👀 Place	🏚️ Before	🏡 After

fix(menubar): remove incorrect `aria-activedescendant`

aria-activedescendant should identify a visually focused element when the real focus remains on this element.
In the current implementation, it identifies the checked element (even when the menu is closed), not the focused.
aria-activedescendant is not needed because NcAction has actual focus anyway.
In case it will be actually needed, it should be implemented in the NcActions

👀 Place	🏚️ Before	🏡 After

fix(table): replace `InlineActionsContainer` by `NcActionButtonGroup`

It has the same styles and correct a11y attributes.

👀 Place	🏚️ Before	🏡 After

fix(table): make text align buttons radio

🏚️ Before	🏡 After

fix(menubar): replace `aria-pressed` with text description in `ActionList`

aria-pressed is not valid for a menu trigger button. And
As an alternative solution - show that there is a selected value directly in the text
- Headings, "Heading 1" is selected
Remove incorrect name prop for NcActions

👀 Place	🏚️ Before	🏡 After

refactor: implement `ActionSingle` via template instead of render-function

Just to simplify further work and Vue 3 migration next release

No visual changes

fix(menubar): set correct type for buttons instead of `aria-selected`

Since @nextcloud/vue@8.6.0 correct attributes are covered by NcAction* and NcButton components, including fixes:
- Attribute should display not only active attr="true" state, but also non-active attr="false"
- It should be aria-pressed for buttons and aria-checked for menu items instead of aria-selected
Set correct type and active state
- type="radio" for a list of options like Heading
- type="checkbox" for toggle buttons like Bold
- type="button" for general buttons without active state like Undo

This doesn't fully work without the next one commit

👀 Place	🏚️ Before	🏡 After

fix(menubar): separate `NcActionButton` from `ActionSingle`

ActionSingle was used in 2 places:
1. As a single button in the menu
2. As an item in NcActions
NcActions doesn't fully support non-direct NcAction* children
Move NcActionButton usage from ActionSingle to a new component
This new component is named NcActionButton so that NcActions will consider it to be a valid NcAction* component

🏚️ Before	🏡 After

fix(menubar): add separator between radio groups

Separates different parts visually
Required for a11y to group radio button in the menu

🏚️ Before	🏡 After

fix(menubar): name `ActionFormattingHelp` as `NcActionButton`

Same as with separating NcActionButton - for NcActions support

👀 Place	🏚️ Before	🏡 After

🏁 Checklist

Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
Sign-off message is added to all commits
Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
Documentation (README or documentation) has been updated or is not required

All these roles are correctly set by `NcAction*` components when needed. Setting these roles in mixins and menu bar puts them on wrong elements. Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

- It must have string `"true"/"menu"` value - It must be placed on the `button` itself - It is already correctly set by `NcActions` and `NcPopover` Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

- `aria-activedescendant` should identify a visually focused element when the real focus remains on this element. In the current implementation it identified selected element (even when the menu is closed), not the focused. - `aria-activedescendant` is not needed because NcAction has actual focus anyway. In case it will be actually needed, it should be implemented in the `NcActions` Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

It has the same styles and correct a11y attributes. Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

- `aria-pressed` is not valid for a menu trigger button - As alternative solution - show that there is a selected value directly in the text - Remove incorrect prop for NcActions Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

- Since `@nextcloud/vue@8.6.0` correct attributes are covered by `NcAction*` and `NcButton` components, including fixes: - Attribute should display not only active `attr="true"` state, but also non-active `attr="false"` - It should be `aria-pressed` for buttons and `aria-checked` for menu items instead of `aria-selected` - Set correct `type` and active state - `type="radio"` for a list of options like `Heading` - `type="checkbox"` for toggle buttons like `Bold` - `type="button"` for general buttons widhout active state like `Undo` Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

max-nextcloud

Thanks a lot for splitting up the individual aspects. This was very helpful for understanding.

I only looked at the code so far. I'd like to play a bit with it before approving. I also have one minor comment I want to look into further.

max-nextcloud

Works like a charm. 🪄

emoral435

The splitting of the issues within the description was really nice! No comments from me, just always great to learn how I should structure future problems with your best practices :)

JuliaKirschenheuter

wooow! That looks great and works right!

All good from my side besides:

But this is NcVue part

Pytal · 2024-01-31T23:41:08Z

+ <div class="avatardiv icon-group" />
+ <AvatarWrapper v-for="session in sessionsVisible"
+ :key="session.id"
+ :session="session"
+ :size="40" />


The div here should be switched to span and the divs inside AvatarWrapper also need to be span to not have block-level elements inside the inline button
Otherwise good!

Thanks. I'll do it in another PR with fixing HTML validation issues.

ShGKme · 2024-02-01T11:00:04Z

Two fixup!s:

Fix potential issue @max-nextcloud found (reviewed)
To make NcActions' children named NcActionButton is was enough to change name property, without having to change the filename.

One new change:

Fix 1/2 issues @JuliaKirschenheuter has found (the second one is on @nextcloud/vue and is less critical)

- ActionSingle was used in 2 places: 1. As a single button in the menu 2. As an item in NcActions - NcActions doesn't fully support non-direct NcAction* children - Move NcActionButton usage from ActionSingle to a new component - This new component is named NcActionButton so that NcActions will consider it to be a valid NcAction* component Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

- Separates different parts visually - Required for a11y to group radio button in the menu Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

…NcActions Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

ShGKme · 2024-02-01T11:16:12Z

Rebased onto master, squashed fixup commits (only small changes like renaming components were made, still works).

juliusknorr · 2024-02-01T11:22:05Z

/backport to stable28

ShGKme added bug Something isn't working 2. developing accessibility labels Jan 9, 2024

ShGKme added this to the Nextcloud 29 milestone Jan 9, 2024

ShGKme self-assigned this Jan 9, 2024

ShGKme linked an issue Jan 9, 2024 that may be closed by this pull request
[BITV]: Remove unneeded roles menuitem and menu#5216
Closed
4 tasks

This comment was marked as resolved.
Sign in to view

JuliaKirschenheuter self-assigned this Jan 10, 2024

ShGKme force-pushed the fix/5216/remove-incorrect-roles branch 2 times, most recently from f4d2a4d to ad73420Compare January 10, 2024 13:32

This comment was marked as resolved.
Sign in to view

JuliaKirschenheuter removed their assignment Jan 10, 2024

ShGKme force-pushed the fix/5216/remove-incorrect-roles branch 6 times, most recently from 7cb42b4 to 007d27dCompare January 31, 2024 10:51

ShGKme added 3. to review and removed 2. developing labels Jan 31, 2024

ShGKme requested review from JuliaKirschenheuter, Pytal, juliusknorr, luka-nextcloud, max-nextcloud and susnux and removed request for juliusknorr and max-nextcloud January 31, 2024 10:56

ShGKme added 4 commits January 31, 2024 11:57

fix(menubar): remove unneeded roles menu, menuitem
b53602a
All these roles are correctly set by `NcAction*` components when needed. Setting these roles in mixins and menu bar puts them on wrong elements. Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

fix(menubar): remove incorrect aria-haspopup
a762115
- It must have string `"true"/"menu"` value - It must be placed on the `button` itself - It is already correctly set by `NcActions` and `NcPopover` Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

fix(table): replace InlineActionsContainer by NcActionButtonGroup
21e76f3
It has the same styles and correct a11y attributes. Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

ShGKme force-pushed the fix/5216/remove-incorrect-roles branch from 007d27d to 4ccb4bfCompare January 31, 2024 10:57

ShGKme changed the title ~~fix: remove incorrect role attrs from menu bar actions~~fix(menubar): menubar and table menus a11yJan 31, 2024

ShGKme added 4 commits January 31, 2024 12:15

fix(table): make text align buttons radio
ada2fdd
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

refactor: implement ActionSingle via template instead of render-function
e5629fc
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

ShGKme force-pushed the fix/5216/remove-incorrect-roles branch from 4ccb4bf to 9be897eCompare January 31, 2024 11:16

max-nextcloud reviewed Jan 31, 2024
View reviewed changes

Comment threadsrc/components/Menu/MenuBar.vue

max-nextcloud approved these changes Jan 31, 2024
View reviewed changes

emoral435 approved these changes Jan 31, 2024
View reviewed changes

JuliaKirschenheuter approved these changes Jan 31, 2024
View reviewed changes

Pytal approved these changes Jan 31, 2024
View reviewed changes

ShGKme added 3 commits February 1, 2024 12:03

fix(menubar): add separator between radio groups
e851f88
- Separates different parts visually - Required for a11y to group radio button in the menu Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

fix(menubar): name ActionFormattingHelp as NcActionButton to support …
9d31390
…NcActions Signed-off-by: Grigorii K. Shartsev <me@shgk.me>

ShGKme force-pushed the fix/5216/remove-incorrect-roles branch from 11bba34 to 9d31390Compare February 1, 2024 11:16

ShGKme enabled auto-merge February 1, 2024 11:20

backportbotBot added the backport-request label Feb 1, 2024

ShGKme merged commit 11a3295 into mainFeb 1, 2024

ShGKme deleted the fix/5216/remove-incorrect-roles branch February 1, 2024 11:30

backportbotBot mentioned this pull request Feb 1, 2024
[stable28] fix(menubar): menubar and table menus a11y #5330
Merged

backportbotBot removed the backport-request label Feb 1, 2024

blizzz mentioned this pull request Mar 5, 2024
29.0.0 beta 1 nextcloud/server#43991
Merged

Navigation Menu

Uh oh!

fix(menubar): menubar and table menus a11y - #5218

fix(menubar): menubar and table menus a11y#5218
ShGKme merged 12 commits into
mainfrom
fix/5216/remove-incorrect-roles

ShGKme commented Jan 9, 2024•
edited
Loading

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

max-nextcloud left a comment

Uh oh!

Uh oh!

max-nextcloud left a comment •
edited
Loading

Uh oh!

emoral435 left a comment

Uh oh!

JuliaKirschenheuter left a comment

Uh oh!

PytalJan 31, 2024

Uh oh!

ShGKmeFeb 1, 2024

Uh oh!

ShGKme commented Feb 1, 2024•
edited
Loading

Uh oh!

ShGKme commented Feb 1, 2024

Uh oh!

juliusknorr commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

ShGKme commented Jan 9, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Summary

🚧 TODO

fix: apply a11y attrs for session list trigger

fix(menubar): remove unneeded roles menu, menuitem

fix(menubar): remove incorrect aria-haspopup

fix(menubar): remove incorrect aria-activedescendant

fix(table): replace InlineActionsContainer by NcActionButtonGroup

fix(table): make text align buttons radio

fix(menubar): replace aria-pressed with text description in ActionList

refactor: implement ActionSingle via template instead of render-function

fix(menubar): set correct type for buttons instead of aria-selected

fix(menubar): separate NcActionButton from ActionSingle

fix(menubar): add separator between radio groups

fix(menubar): name ActionFormattingHelp as NcActionButton

🏁 Checklist

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

max-nextcloud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

max-nextcloud left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emoral435 left a comment

Choose a reason for hiding this comment

Uh oh!

JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

Uh oh!

PytalJan 31, 2024

Choose a reason for hiding this comment

Uh oh!

ShGKmeFeb 1, 2024

Choose a reason for hiding this comment

Uh oh!

ShGKme commented Feb 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShGKme commented Feb 1, 2024

Uh oh!

juliusknorr commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ShGKme commented Jan 9, 2024•
edited
Loading

fix(menubar): remove unneeded roles `menu`, `menuitem`

fix(menubar): remove incorrect `aria-haspopup`

fix(menubar): remove incorrect `aria-activedescendant`

fix(table): replace `InlineActionsContainer` by `NcActionButtonGroup`

fix(menubar): replace `aria-pressed` with text description in `ActionList`

refactor: implement `ActionSingle` via template instead of render-function

fix(menubar): set correct type for buttons instead of `aria-selected`

fix(menubar): separate `NcActionButton` from `ActionSingle`

fix(menubar): name `ActionFormattingHelp` as `NcActionButton`

max-nextcloud left a comment •
edited
Loading

ShGKme commented Feb 1, 2024•
edited
Loading