? ? Pending

User tests: Successful: Unsuccessful:

avatar hans2103
hans2103
1 Jun 2020

Pull Request for Issue # .

Summary of Changes

With #29343 I've implemented aria-current="page"
But... the menu item is not necessary the current page. => see #29343 (comment)

Therefor I have rewritten the aria-current="page" implementation by adding some logic to the foreach loop of items in the getList function of MenuHelper. Added comparing the url parameters option, view and id (if applicable) with those from the menu item.

If the comparison results true, the new field $item->current changes from false into true

Testing Instructions

  • Joomla 4.0 beta with sample data installed (forgive me ... I don't know which sample data, since I've pressed all of them.)

  • Click on menu item blog

  • Open your browser develop tools and find the output of mod_menu

  • Find the anchor of menu item blog, the current page and see the attribute aria-current="page"

  • Click on article Welcome to your blog

  • Open your browser develop tools and find the output of mod_menu

  • Find the anchor of menu item blog, the current page and see the attribute aria-current="page"

  • A menu item to the login page has no id. The solution handles a non-set item->id.

  • The $item->current is not set to true if the menu item is not connected to the output of a component... for instance an alias item.

Expected result

Expected is that menu item blog does not have attribute aria-current="page" when visiting the blog article Welcome to our blog.

Actual result

Both the menu item of the category blog as the articles of this menu item contain attribute aria-current="blog".

After applying the patch, the actual result is equal to the expected result.

Documentation Changes Required

no documentation changes required.

avatar hans2103 hans2103 - open - 1 Jun 2020
avatar hans2103 hans2103 - change - 1 Jun 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jun 2020
Category Modules Front End
avatar SharkyKZ
SharkyKZ - comment - 1 Jun 2020

This is even less reliable. Other components may use different input variables.

avatar hans2103
hans2103 - comment - 1 Jun 2020

@SharkyKZ can you provide an example where option and view are not present?

avatar SharkyKZ
SharkyKZ - comment - 1 Jun 2020

Option is always present. But view and ID not necessarily. And many other variables can be used in a menu link.

avatar hans2103
hans2103 - comment - 1 Jun 2020

Back to the drawing board and find a way to differ a category blog from an article.
I will try to make it more general.

avatar SharkyKZ
SharkyKZ - comment - 1 Jun 2020

Also don't forget that you can have multiple menu items point to the same page. Using query variables is not good in that case. Multiple menu items would be marked as active.

avatar hans2103
hans2103 - comment - 2 Jun 2020

correct me if I'm wrong.. if the currently merged solution aria-current="page" is open for improvement, so is the existing code which adds a the class current. Is that piece of code open for improvement too?

if ($item->id == $active_id || ($item->type === 'alias' && $itemParams->get('aliasoptions') == $active_id))
{
$class .= ' current';
}

If so... we have to extend the MenuHelper with a function that finds out if the menu item is indeed the current page. And not only for an opened item in category blog, also for other components.

avatar brianteeman
brianteeman - comment - 2 Jun 2020

Everything is open for improvement!

As I stated in the other pr you are trying to resolve the exact same issue as the display of modules on non-parent items. So if you can resolve this it will be appreciated by many. Even if it removes some of the use cases for "advanced module manager"

avatar HLeithner
HLeithner - comment - 2 Jun 2020

correct me if I'm wrong.. if the currently merged solution aria-current="page" is open for improvement, so is the existing code which adds a the class current. Is that piece of code open for improvement too?

if ($item->id == $active_id || ($item->type === 'alias' && $itemParams->get('aliasoptions') == $active_id))
{
$class .= ' current';
}

If so... we have to extend the MenuHelper with a function that finds out if the menu item is indeed the current page. And not only for an opened item in category blog, also for other components.

The current class does not the same then the aria-current="page" expect, our current class is used for the most specific menu entry but doesn't care if you have a complete shop system under this link.

I'm not even sure if you find the correct menu entry by comparing all input variables.

The other way could be a bit more "secure" check if additional parameters are set and only set aria-current if no other input-var is set, but in theory you page can have another content based on your session or on a cookie.

avatar SharkyKZ
SharkyKZ - comment - 2 Jun 2020

I didn't see the discussion in the other PR before.

If the idea is to mark only the exact page defined in the menu item and not any child pages, you could probably get all input variables (minus some system ones like Itemid) and compare them against menu item query.

avatar HLeithner
HLeithner - comment - 2 Jun 2020

I didn't see the discussion in the other PR before.

If the idea is to mark only the exact page defined in the menu item and not any child pages, you could probably get all input variables (minus some system ones like Itemid) and compare them against menu item query.

that's maybe (hopefully) true for core but you can't say this for 3rd party extensions they maybe include other (from another source we don't know) to decide which page is displayed. I'm not sure if it's better to display none or a wrong one.

avatar hans2103 hans2103 - change - 2 Jun 2020
Labels Added: ?
avatar hans2103
hans2103 - comment - 2 Jun 2020

Changed the implementation by replacing the comparison.
First implementation compared parameters option, view and id.
New implementation walks through each Factory::getApplication()->getInput()->get($key) and compares it with the equal named $key from $item->query.

Tested with all Joomla 4 Sample data installed.
see: https://github.com/joomla/joomla-cms/pull/29369/files

avatar HLeithner
HLeithner - comment - 2 Jun 2020

I would suggest to use array_diff_assoc instead of looping over the query array

avatar hans2103
hans2103 - comment - 2 Jun 2020

Updated PR with newly changes pulled from branch 4.0 dev

avatar hans2103
hans2103 - comment - 2 Jun 2020

@HLeithner

I would suggest to use array_diff_assoc instead of looping over the query array

Would have chosen that option too if I could reach the content of the protected data from Factory::getApplication()->getInput()

avatar hans2103
hans2103 - comment - 4 Jun 2020

@SharkyKZ @HLeithner please review my improved PR.

avatar HLeithner
HLeithner - comment - 4 Jun 2020

As explained there are more ways to decide if a link to the page is the current page or not and $_GET is not everything.

avatar richard67
richard67 - comment - 4 Jun 2020

@hans2103 Sorry if I'm wrong because I am not very deep into that subject. But as a quick reader, to me what you try to do here reads as if you wanted to implement the router again just at another place.

avatar hans2103
hans2103 - comment - 4 Jun 2020

As explained there are more ways to decide if a link to the page is the current page or not and $_GET is not everything.

@HLeithner feel free to make a PR on my suggestion. I would love to learn from it.

avatar HLeithner
HLeithner - comment - 4 Jun 2020

I think it's not possible for the Joomla! cms to provide 100% accurate data and I personally would prefer not to set wrong value. If someone want's this he/she can use a simple template override for the mod_menu.

avatar hans2103
hans2103 - comment - 4 Jun 2020

@HLeithner how can we improve J4 so we can provide 100% accurate data?
It would be great to provide this with J4 out of the box, not with a simple template override.

If we cannot implement it now, I am afraid that it will have to wait until J5.

The Joomla Accessibility Statements tells us we are committed to being accessible to the widest possible audience, regardless of ability or technology.
https://www.joomla.org/accessibility-statement.html

avatar HLeithner
HLeithner - comment - 4 Jun 2020

Due the nature of the CMS which is extensible by 3rd party developers I don't see a chance to make this 100% accurate ymmv.

But another question what's the benefit of this link? I mean which group of people have a benefit of this?
And you would like to deliver a wrong solution then non solution (we already have some of wrong solution that block joomla enhancement since 1.0 or so)

avatar hans2103
hans2103 - comment - 4 Jun 2020

Due the nature of the CMS which is extensible by 3rd party developers I don't see a chance to make this 100% accurate ymmv.

The solution in the PR covers this issue by comparing the parameters. If any parameter don't match, the aria-current="page" will not be set.
And it is not wrong if 3th party developers code more in the direction of Joomla coding standards. (wishful thinking... I know)

But another question what's the benefit of this link? I mean which group of people have a benefit of this?

This question is irrelevant due to the first couple of sentences of https://www.joomla.org/accessibility-statement.html

And you would like to deliver a wrong solution then non solution

I would like not to. But I haven't stumbled upon a situation where my proposed solution fails. Please provide me with such a situation and I try to solve it.

(we already have some of wrong solution that block joomla enhancement since 1.0 or so)

Create PR to solve them one by one... start mentioning them in new issues.

avatar HLeithner
HLeithner - comment - 4 Jun 2020

But another question what's the benefit of this link? I mean which group of people have a benefit of this?

This question is irrelevant due to the first couple of sentences of https://www.joomla.org/accessibility-statement.html

Sorry but why is a question I have personally is irrelevant?

avatar hans2103
hans2103 - comment - 4 Jun 2020

But another question what's the benefit of this link? I mean which group of people have a benefit of this?

This question is irrelevant due to the first couple of sentences of https://www.joomla.org/accessibility-statement.html

Sorry but why is a question I have personally is irrelevant?

Sorry... I have no means to offend you and let me answer your question.
This addition to the menu item will benefit accessibility of the website.
And accessibility improvements come with baby steps. This PR is one of the baby steps I try to achieve.

The idea that web accessibility only benefits people with disabilities is one of the most common web accessibility myths. (source: https://www.boia.org/blog/common-web-accessibility-myths)

6 Unexpected Benefits of Web Accessibility (source: https://www.boia.org/blog/6-unexpected-benefits-of-web-accessibility)

avatar brianteeman
brianteeman - comment - 4 Jun 2020

Adding this is definitely an important improvement for accessibility.

Is there a negative impact by having this attribute on a parent page when we are on a child page? I have no idea. It would be good if the Accessibility team actually spoke up on this issue. No point in having the team if they don't do anything ;)

avatar HLeithner
HLeithner - comment - 4 Jun 2020

@hans2103 your answer was no answer, I know why we do a11y I wanted to know what's the benefit is from exactly this attribute but I think I found it my self at the meantime.

Anyway there are so many situations where this entry could be false like one-pagers, ajax-updated content, SPA, Paging and search saved parameter saved in cookies or session data and so on.

Actually that is correct and is not miss leading people who really needs this information. I have the feeling that you think it's better if it's wrong then it's missing. So I'm out here the @joomla/joomla-accessibility-team-jat should say what to do.

Beside that I think it's wrong thanks for you contribution.

avatar chmst
chmst - comment - 5 Jun 2020

The aria-current="page" is not required and no success criterion for any requirement. So it can be set and might be useful IF the value is always reliable.
Aria labels should not be used as self-purpose.
If @HLeithner says that the values are not 100% reliable, it is better to leave it.

avatar hans2103
hans2103 - comment - 6 Jun 2020

Although aria-current="page" is not required and no success criterion for any requirement, it is already implemented. This PR tries to make it more reliable.

if ($item->id == $active_id)
{
$attributes['aria-current'] = 'page';
}

if the values are not 100% reliable, it is better to leave it.

@chmst my PR adds a check to compares the parameters of the application input with the query of the menu item. If it fails, the attribute will not set.

@HLeithner would it be an idea if a config item is added where you can toggle this attribute
Apply aria-current="page"? YES / NO (default)
This option will conditional add the attribute aria-current to the menu item which indicates the current page.

avatar zwiastunsw
zwiastunsw - comment - 6 Jun 2020

I'm trying to understand this discussion and I really don't understand what problem is being solved.
Do you mean that e.g. a link points to a category, because the menu item is a category blog and theand the viewed page (current page) is some kind of a blog article that does not have its specific position in the menu system?
A menu item is not the same as a link to a page. One current menu item can be the current item for multiple pages. Distinguishing a menu item with CSS and showing the screen readers with aria-current is exactly the same.
You write that currently when I'm on a blog article page, the menu item has aria-current. You think that the menu item at this point should not have aria-current attribute, because it's not a category page, it's a subpage, your thinking is wrong.
The aria-current attribute is used when an element within a set of related elements is visually styled to indicate it is the current item in the set.
At this time, when I'm on a blog article page, the visual styling indicates an item like Category blog in the menu, because the item for each article is not on the menu at all. Therefore this stylized item should also have aria-curent=page (or aria-current=location if you want to be very precise). Removing aria-current from a menu item is not a good solution. Because there is no indication for people using assistive technology which menu item is current. When you have a menu, there is always one menu item that is current.
But maybe I don't understand something.
In my opinion, the current aria-current implementation in menu is sufficient. Nothing here needs to be improved.

avatar softforge softforge - test_item - 6 Jun 2020 - Tested successfully
avatar softforge
softforge - comment - 6 Jun 2020

I have tested this item successfully on 44f84dd

I see there is discussion as to its merits but the patch does do as it says it should in the test so its a successful test


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29369.

avatar softforge
softforge - comment - 6 Jun 2020

I have tested this item successfully on 44f84dd

I see there is discussion as to its merits but the patch does do as it says it should in the test so its a successful test


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29369.

avatar brianteeman
brianteeman - comment - 6 Jun 2020

Please read https://tink.uk/using-the-aria-current-attribute/ and the comments. Specifically
"you’d only use aria-current on the link representing the currently selected page."

The article without a menu item accessed from a blog category would therefore mean that the menu link for the blog category is not aria-current

avatar zwiastunsw
zwiastunsw - comment - 6 Jun 2020

@brianteeman : I know that article. And I know the comment. But the question that Leonie Watson answered was: Should we use aria-current=”page” on all three levels?
I'm not saying I'm right. But I'm looking at e.g. the WAI W3C page and I see aria-current="page" at the current page, but also aria-current="location" at the top menu item. See: https://www.w3.org/WAI/test-evaluate/tools/selecting/. Link Evaluation Tools have an attribute: aria-current="location", link "Selecting Evaluation Tools" have an attribute: aria-current="page".

avatar brianteeman
brianteeman - comment - 6 Jun 2020

For me you answered the question.
If you are on the page then it is aria-current=page
If you are on a child page then it is aria-current=location

It cannot be correct for multiple completely different pieces of content all be marked as a page with the same name

avatar zwiastunsw
zwiastunsw - comment - 6 Jun 2020

I agree.

avatar brianteeman
brianteeman - comment - 6 Jun 2020

but your first comment disagreed with that?

avatar zwiastunsw
zwiastunsw - comment - 6 Jun 2020

You know, it's not easy for me to describe such complex things in English. That's why I wrote in the previous commentary: Therefore this stylized item should also have aria-curent=page (or aria-current=location if you want to be very precise)

avatar hans2103
hans2103 - comment - 27 Jul 2020

Merged all 4.0-dev changes to keep this PR up to date with 4.0-dev

After the clearing discussion by @zwiastunsw and @brianteeman (thank you both for that) I've updated this PR.

It will set aria-current=page to the menu item when you are on the current page.
It will set aria-current=location to the menu item when you are on a child page of the current page.

For instance... testing with Sample Data Testing it will set aria-current=page to menu item "Park Blogs" and will change the aria-current=location of that menu item when you click one of the two blog posts it contains.

avatar hans2103 hans2103 - change - 27 Jul 2020
Title
Origin/feature/mod menu current page
[4.0] [a11y] Origin/feature/mod menu current page
avatar hans2103 hans2103 - edited - 27 Jul 2020
avatar hans2103 hans2103 - change - 27 Jul 2020
Title
[4.0] [a11y] Origin/feature/mod menu current page
[4.0] [a11y] mod_menu aria-current page/location
avatar hans2103 hans2103 - edited - 27 Jul 2020
avatar brianteeman
brianteeman - comment - 2 Aug 2020

@hans2103 just for reference it isnt necessary to keep updating your branch unless there are any conflicts

avatar david0296 david0296 - test_item - 4 Aug 2020 - Tested successfully
avatar david0296
david0296 - comment - 4 Aug 2020

I have tested this item successfully on f52f447

before applying the patch: <a href="/index.php/blog-1" aria-current="page">Blog</a>
after applying the patch: <a href="/index.php/blog-1" aria-current="location">Blog</a>
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29369.

avatar nurcihandem nurcihandem - test_item - 5 Aug 2020 - Tested successfully
avatar nurcihandem
nurcihandem - comment - 5 Aug 2020

I have tested this item successfully on f52f447

Menu item Blog is on area-current="page" on home page
Menu item Blog is on area-current="location" on "Welcome to your blog"-detail view


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29369.

avatar richard67
richard67 - comment - 5 Aug 2020

@brianteeman @HLeithner @SharkyKZ Due to previous discussions I am not sure if I shall set this PR to RTC now as it has 2 good tests. Does anything speak against RTC from your point of view? Please report back if or if not. Thanks in advance.

Pinging also @wilsonge for his opinion.

avatar HLeithner
HLeithner - comment - 5 Aug 2020

@zwiastunsw are you happy with this PR? if yes then it can be merged.

@hans2103 can you update the branch because drone got some fixes yesterday

avatar hans2103
hans2103 - comment - 5 Aug 2020

Will update as soon as possible. On vacation right now

Op wo 5 aug. 2020 om 12:26 schreef Harald Leithner <notifications@github.com

@zwiastunsw https://github.com/zwiastunsw are you happy with this PR?
if yes then it can be merged.

@hans2103 https://github.com/hans2103 can you update the branch because
drone got some fixes yesterday


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29369 (comment),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAE4GTWJPXSL25FES2CXXNDR7EXWTANCNFSM4NQBYMHA
.

--
met vriendelijke groeten,

Hans Kuijpers

HKweb
info@hkweb.nl
https://hkweb.nl
tel: +31 654 224 518

avatar zwiastunsw
zwiastunsw - comment - 6 Aug 2020

Yeah, it's OK.

avatar Quy Quy - change - 7 Aug 2020
Status Pending Ready to Commit
avatar Quy
Quy - comment - 7 Aug 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29369.

avatar hans2103 hans2103 - change - 7 Aug 2020
Labels Added: ?
avatar hans2103
hans2103 - comment - 7 Aug 2020

merged recent changes on branch 4.0-dev into this PR

avatar wilsonge wilsonge - close - 9 Aug 2020
avatar wilsonge wilsonge - merge - 9 Aug 2020
avatar wilsonge wilsonge - change - 9 Aug 2020
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-08-09 22:27:36
Closed_By wilsonge
avatar wilsonge
wilsonge - comment - 9 Aug 2020

Seems like a valid attempt to me. We'll only find out on this kinda stuff by seeing the real world experiences. Thanks!

Add a Comment

Login with GitHub to post a comment