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

fix(NcActions): fix LI roles #5203

Closed
wants to merge 1 commit into from

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Feb 1, 2024

☑️ Resolves

Currently the menu in NcActions has the following structure:

<ul role="POPUP_ROLE">
  <li role="LI_ROLE" />
</ul>

POPUP_ROLE can be - menu | dialog | tooltip or nothing (=list) in navigation:

  • When it is a navigation, native roles works fine
  • When it is a menu, <li> has role presentation, because there are menuitem-s inside.
  • When it is dialog or tooltip, <li> had native listitem role because there is a list in the popup.

The problem is that in the last case <UL> is at the same time:

  • A list items' container - should have list role
  • A popup's container - should have dialog or tooltip role.

But it is not possible to have 2 roles. So, when it is a dialog or tooltip, list items are not in a list.

A proper solution should have been to wrap UL into another element.

<div role="POPUP_ROLE">
  <ul>
    <li />
  </ul>
</div>

However, we decided that this is too risky change of the structure for apps and testing.

Temporal solution - remove listitem role in dialogs and tooltips.

🖼️ Screenshots

Example 🏚️ Before 🏡 After
image image image

🚧 Tasks

  • Better store HTML roles for different types of menu
  • Provide NcActions context with as menu type and LI role
    • Relaces existing isSemanticMenu to provide more important data
  • Add a composable to use this context
    • Replaces existing inject
  • Set list item role on all action items, except NcActionSeparator (it is always separator)

P.S.

I haven't used existing mixins (actionText, actionGlobal) because they are not used on all the action items.

I haven't created a new mixin, because I decided, that we should move to composables.

🏁 Checklist

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

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme added bug Something isn't working 2. developing Work in progress feature: actions Related to the actions components accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Feb 1, 2024
@ShGKme ShGKme added this to the 8.6.2 milestone Feb 1, 2024
@ShGKme ShGKme self-assigned this Feb 1, 2024
@ShGKme ShGKme modified the milestones: 8.6.2, 8.6.3 Feb 7, 2024
@susnux susnux modified the milestones: 8.6.3, 8.7.0, 8.8.0, 8.7.1 Feb 18, 2024
@Antreesy Antreesy modified the milestones: 8.7.1, 8.7.2 Feb 21, 2024
@Antreesy Antreesy modified the milestones: 8.7.2, 8.8.2 Feb 29, 2024
@ShGKme ShGKme modified the milestones: 8.8.1, 8.8.2 Feb 29, 2024
@juliushaertl juliushaertl modified the milestones: 8.8.2, 8.9.1 Mar 6, 2024
@Pytal Pytal modified the milestones: 8.9.1, 8.9.2 Mar 6, 2024
@ShGKme ShGKme modified the milestones: 8.9.2, 8.10.0 Mar 8, 2024
@ShGKme
Copy link
Contributor Author

ShGKme commented Mar 11, 2024

I forgot about this PR when was re-making it again in #5381 🙈

@ShGKme ShGKme closed this Mar 11, 2024
@ShGKme ShGKme removed this from the 8.10.0 milestone Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants