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

Add an "ARIA label" field to the navigation module #7209

Open
wants to merge 2 commits into
base: 5.x
Choose a base branch
from

Conversation

leofeyer
Copy link
Member

Implements #6977

@leofeyer leofeyer added this to the 5.4 milestone May 10, 2024
@leofeyer leofeyer self-assigned this May 10, 2024
@leofeyer leofeyer requested a review from a team May 10, 2024 13:15
@leofeyer leofeyer linked an issue May 10, 2024 that may be closed by this pull request
@zoglo
Copy link
Contributor

zoglo commented May 11, 2024

I feel like this option should be available in many languages. I've had a project that focuses on an accessible navigation and I used translations for 5 different languages.

If people would want to change it, they wouldn't need the field but could override the translation, wdyt?

@fritzmg
Copy link
Contributor

fritzmg commented May 12, 2024

@zoglo you can use the {{label::*}} insert tag for that - or create multiple navigation modules (one for each language).

@zoglo
Copy link
Contributor

zoglo commented May 12, 2024

I was just thinking about pre-delivery of the Aria-labels within translations so people don't need to set it up manually, they could still override it within this module 🤔.

Maybe it's good the way it's implemented in the PR, especially for other types of navigations..

Going the long way of creating the language files for the label insert tag would just need a proper documentation.

@fritzmg
Copy link
Contributor

fritzmg commented May 12, 2024

What do you want to do to pre-deliver here? The label will be be different for each navigation module.

@zoglo
Copy link
Contributor

zoglo commented May 12, 2024

What do you want to do to pre-deliver here? The label will be be different for each navigation module.

You are right, you already convinced me :D

@leofeyer
Copy link
Member Author

leofeyer commented May 13, 2024

Having to create multiple modules just to change the label is quite inconvenient indeed. The {{label::*}} and the {{iflng::*}} insert tags are good workarounds, but maybe we can come up with a better (in terms of more i18n-like) solution?

@fritzmg
Copy link
Contributor

fritzmg commented May 13, 2024

Having to create multiple modules just to change the label is quite inconvenient indeed. The {{label::*}} and the {{iflng::*}} insert tags are good workarounds, but maybe we can come up with a better (in terms of more i18n-like) solution?

But you already have to do that with several modules anyway - especially the navigation module. For example:

  • If you want to have different labels for the back link in the news reader module, you either use {{label::*}} or multiple modules.
  • If you have a navigation with a reference page, you need to create multiple instances of this module if you have multiple website roots (for multiple languages).

In each case you typically create another module - the root page dependent module selector - and then integrate that module in your page layout or a page's content.

@zoglo
Copy link
Contributor

zoglo commented May 13, 2024

Having to create multiple modules just to change the label is quite inconvenient indeed. The {{label::*}} and the {{iflng::*}} insert tags are good workarounds, but maybe we can come up with a better (in terms of more i18n-like) solution?

That's why I proposed having a fallback translation in place when the aria label is not set.
However, it would just say something likeNavigation or Menu.

If people would want it to be more accessible, they could use the new field you introduced in this PR.

However, idk if it makes things worse or better because it would be so universal.

@fritzmg
Copy link
Contributor

fritzmg commented May 13, 2024

It would make things worse in my opinion. If you have multiple modules on your site - e.g. the main menu in your header and a footer menu in your footer, they would both just say aria-label="Main". Either you define the label yourself or no aria-label attribute should be output, otherwise there is no gain.

@@ -1,6 +1,6 @@

<!-- indexer::stop -->
<nav class="<?= $this->class ?> block"<?= $this->cssID ?><?php if ($this->style): ?> style="<?= $this->style ?>"<?php endif; ?>>
<nav class="<?= $this->class ?> block"<?= $this->cssID ?><?php if ($this->style): ?> style="<?= $this->style ?>"<?php endif; ?><?php if ($this->ariaLabel): ?> aria-label="<?= $this->ariaLabel ?>"<?php endif; ?>>
Copy link
Contributor

@fritzmg fritzmg May 13, 2024

Choose a reason for hiding this comment

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

Suggested change
<nav class="<?= $this->class ?> block"<?= $this->cssID ?><?php if ($this->style): ?> style="<?= $this->style ?>"<?php endif; ?><?php if ($this->ariaLabel): ?> aria-label="<?= $this->ariaLabel ?>"<?php endif; ?>>
<nav<?= $this->attr($this->cssID)->addClass((string) $this->class)->addStyle((string) $this->style)->setIfExists('aria-label', $this->ariaLabel) ?>>

@leofeyer wdyt? Should we do it like that for all templates, now that we have $this->attr()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, why not. Do we need a ->setIfExists('id', $this->???) instead of $this->cssID as well then?

Copy link
Contributor

@fritzmg fritzmg May 13, 2024

Choose a reason for hiding this comment

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

Well, the $this->??? is the problem :). The CSS ID is currently not available in the template. Only the string including the attribute (i.e. ' id="foobar"'). We could introduce a rawCssID variable or something and pass $cssID[0] to that variable in various places, so that we can use ->setIfExists('id', $this->rawCssID).

Another option would be to use:

$this->attr($this->cssID)->…

This should automatically parse the id="..." string and add it as an attribute (if applicable). (haven't tested this though)

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be nice if it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tested it - it does work.

Copy link
Member

Choose a reason for hiding this comment

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

$this->attr()…->mergeWith($this->cssID) would also work, JFI ☺️

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer passing it to attr().

Copy link
Member Author

Choose a reason for hiding this comment

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

See #7218

@leofeyer
Copy link
Member Author

That’s right, the label must never be "menu" or "navigation", because screen readers already announce the element type. It would read "menu navigation" or "navigation navigation". The label should just be "main" or "top".

@leofeyer
Copy link
Member Author

leofeyer commented May 13, 2024

If you have a navigation with a reference page, you need to create multiple instances of this module

Yes, because you change the module configuration → valid case. But if the configuration remains the same and only the label changes → not a valid case. At least in my opinion.

@fritzmg
Copy link
Contributor

fritzmg commented May 13, 2024

Yes, because you change the module configuration → valid case. But if the configuration remains the same and only the label changes → not a valid case. At least in my opinion.

It doesn't really matter what type of field you have to change. If you have 3 languages and 3 footer navigations (where the footer navigation is represented by one parent page in the site structure) you have to clone the navigation module 3 times and change one field accordingly. The rest of the configuration stays the same.

And regardless: all the fields of a module are part of its configuration, not just specific fields.

@fritzmg
Copy link
Contributor

fritzmg commented May 24, 2024

In #7218 I noticed that we already have a hardcoded aria-label="Breadcrumb" attribute for the breadcrumb module. I think this should then be adjustable in the module as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance navigation modules for accessibility
4 participants