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

pkp/pkp-lib#6296 Article breadcrumbs use aria-current but do not point to the current page - OJS 3.4 #4133

Open
wants to merge 1 commit into
base: stable-3_4_0
Choose a base branch
from

Conversation

israelcefrin
Copy link
Contributor

This PR fixes the issue pkp/pkp-lib#6296 and also implements:

  • aria-current="page" attribute was not being use on a link to the current page
  • Removed aria-current="page" attribute from li element, it should to be used on link elements only
  • Include a link, using the page title as the label, to the last item on the breadcrumb (article and archive pages)
  • Added aria-hidden="true" to the breadcrumb separators (they were announced as "slash" by screen readers which is annoying for screen reader users and don't add any information to the context)

Even though showing the full article title might look overwhelming for sighted people, it is helpfull to people that rely on assistive technology like screen readers or screen magnifiers. They can double-check whether they are on the expected page through the breadcrumb interface element.

@israelcefrin
Copy link
Contributor Author

Hi @asmecher , could you please have a look and let me know if everything is fine? This PR fixes the main issue from this one: pkp/pkp-lib#6296 and also covers more improvements on ARIA attributes that were missing or misplaced.

@asmecher
Copy link
Member

asmecher commented Jan 9, 2024

@jardakotesovec, could you review this? I'm probably the last person who should be looking at front-end markup :)

@jardakotesovec jardakotesovec self-assigned this Jan 11, 2024
@jardakotesovec
Copy link
Contributor

@israelcefrin Thank you! Looks good to me. You mentioned that the aria-current should be used only on links? If thats the case, should not we also update the other cases to be links? For example if we go to /index.php/publicknowledge/about, than About the Journal is not link.

@israelcefrin
Copy link
Contributor Author

@israelcefrin Thank you! Looks good to me. You mentioned that the aria-current should be used only on links? If thats the case, should not we also update the other cases to be links? For example if we go to /index.php/publicknowledge/about, than About the Journal is not link.

Hi @jardakotesovec , you are correct. And I am already tackling it , i.e. the top menu , on this issue: pkp/pkp-lib#5199

@israelcefrin
Copy link
Contributor Author

Hi @jardakotesovec , I've implemented the same feature on the breadcrumbs of OPS and OMP Default Theme breadcrumb template files.

OPS 3.4 PR: pkp/ops#630
OMP 3/4 PR: pkp/pkp-lib#9684

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

Successfully merging this pull request may close these issues.

3 participants