[BITV]: Remove unneeded roles menuitem and menu

[JuliaKirschenheuter]

Important

For QA, please just check that PR is merged to stable28

In most toolbar buttons wrong roles have been assigned. To correct the roles please pay attention on:

• All buttons and div's which are direct children of the toolbar should not have a role="menuitem"
• All buttons which contains menu inside (like heading) should not have any additional roles. All menu roles have been already implemented correct inside of NcActions component
• All selected buttons should not have aria-selected="true" but should have instead aria-pressed="true" and aria-pressed="false" on unpressed state (requires @nextcloud/vue update) (note that aria-pressed can't be on a div class="entry-action action-item")
• All popovers should have correctly used aria-haspopup (may require @nextcloud/vue update)

[JuliaKirschenheuter]

Dear @juliushaertl

could you please explain for us a difference between aria-active="false" on "B" or "I" and "Undo-button"?

Does it means if aria-active="false" have "I" button then "I" button is not pressed right now, right?

But what does it mean for button "Undo"? "Undo" button can't be active.

At the moment there are 3 "states" of buttons: pushed, not pushed, not toggle at all which are defined via 2 props: active="true" or active="false". Am i right?

[ShGKme]

The problem here is that boolean property active in the action button state in the mixin is used to define the active state for all the buttons, not only button that may have active state.

We have buttons:

1. Toggle buttons like "Bold", "Italic". They can be pressed (active=true) and not pressed (active=false). And they should always have aria-pressed attribute with the current active state.
2. Plain buttons like "Undo" and "Redo". They should have neither active not non-active state.
3. Menu trigger button like "Heading" on a large screen and pickers. They also should not have active state.

Having boolean active state on all the action buttons is not enough to determine which button is not pressed (not active) and which cannot be active at all.

active === false at the same time may mean:

• This is an unpressed toggle button
• This is not a toggle button

The question is, is there a way to determine that a button is a toggle button and has an active state?

If there is no, we can add it to the action description.

[ShGKme]

I guess this is isActive property.

[juliusknorr]

Took me a bit to refresh my overview on the menu code parts.

I think we can add an a property for what type a button is to the entry list and set the pressed state only for those that are toggle buttons.

From my overview we'd then need to:

• On toggle buttons: get rid of the active and aria-selected attribute and make use of aria-pressed instead
• On regular buttons: make sure neither active/selected state is there
• Menu trigger buttons:
  • The reason they had a (wrongly placed) active indicator is that the trigger button of NcActions at least visually indicates if any of the child buttons has a pressed state. I'm unsure how we can indicate that in a proper way
  • I also don't see a way to set the trigger button attributes through the props or slots that the NcActions component provides

I can check and push a commit for the first two parts if you want.

[juliusknorr]

I guess this is isActive property.

Ah, yes, we can just use that instead of an additional one

[ShGKme]

• The reason they had a (wrongly placed) active indicator is that the trigger button of NcActions at least visually indicates if any of the child buttons has a pressed state. I'm unsure how we can indicate that in a proper way

Yeah, I also don't have any ideas how we can show it...

• I also don't see a way to set the trigger button attributes through the props or slots that the NcActions component provides

I think if any of these attributes are needed in menus, it should be done on the NcActions side.

But I need to re-check it because we have different recommendations on different accessibility report issues 🙈

[juliusknorr]

@ShGKme Just checked out your recent push on the draft pr and that seems to work fine for the regular and toggle buttons.

Can I be of any further help investigating on the menu trigger buttons?

To summarize we have two "types" of menus there.

• Menus that just list further regular action buttons (e.g. link or image), those seem fine as they are without any selected/pressed indicator
• Menus that wrap a state and are used to switch between different types of that state (heading, callout box) that need clarification on whow to indicate that.

[JuliaKirschenheuter]

Thanks @juliushaertl for help!

[JuliaKirschenheuter]

clarification needed: #3911 (comment)

[ShGKme]

• Menus that wrap a state and are used to switch between different types of that state (heading, callout box) that need clarification on whow to indicate that.

Unfortunately, it is not possible to make a button at the same time a toggle button (with the "pressed" state) and menu opening button (with the "expanded" state). aria-haspopup="menu" + aria-extended overrides aria-pressed.

Correct solution could be to make this button not an opening sub-menu but a custom select. However, this would be a complex change of NcActions to support select-like actions.

The only simple solution I've added is to provide this information via aria-label.

• No selection: Callouts, Headings
• With selection: Callouts, "Info" is selected, Headings, "Heading 1" is selected.

I've pushed the fix to the PR.

@juliushaertl @JuliaKirschenheuter what do you think?

[juliusknorr]

Sounds reasonable. There might be some restructuring/redesign of the menubar planned #2836 / #4049 where we then should consider this matter as well, but for now this sounds like a good solution to move on.

[JuliaKirschenheuter]

I agree, let's move this way first and other improvements can be done afterwards