Skip to content

Conversation

@JackKellyUK
Copy link
Contributor

Summary of Changes

Awesomplete is raising a Serious issue in axe for a missing title / aria-label attribute.

image

Added title attribute to results list.

There is an open issue about this, a fix looks to be implemented with the listLabel property, but it is currently unreleased.

Testing Instructions

  • Setup Smart Search > Search menu item (Smart Search > Search Suggestions global setting must be enabled)
  • Run axe browser plugin / find awesomplete list ul and check for title / aria-label attribute

Actual result BEFORE applying this Pull Request

No title / aria-label attribute present on Awesomplete list

Expected result AFTER applying this Pull Request

Title attribute present on Awesomplete list

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Dec 6, 2022
@JackKellyUK JackKellyUK changed the title Added title to awesomplete results list Awesomplete accessibility fix Dec 6, 2022
@chmst chmst added the a11y Accessibility label Dec 6, 2022
@brianteeman
Copy link
Contributor

What am I missing here as I dont see an accessibility issue
image

@JackKellyUK
Copy link
Contributor Author

What am I missing here as I dont see an accessibility issue image

If you enter a search term that populates the list you should see that the name property is blank.

@dgrammatiko
Copy link
Contributor

@JackKellyUK right now the label you're applying will only work for English language. What I'll suggest is either

  • extend the Joomla.getOptions('finder-search') to pass also the translated string for the label
  • or use a data-ul-label="<?php echo Text::_('SOME_STRING'); ?>" and grab the text from there

Although I'm the original author of the script (#21438) I think the whole script could be rewritten with way less code (eg the DOMContentLoaded event is harmful since the script is loaded either as a module or deferred, etc)

@wilsonge
Copy link
Contributor

wilsonge commented Dec 7, 2022

What am I missing here as I dont see an accessibility issue image

Not sure. But wouldn't this be exposed once you've entered some text

@JackKellyUK JackKellyUK requested a review from chmst as a code owner December 10, 2022 12:41
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Dec 10, 2022
@JackKellyUK
Copy link
Contributor Author

@JackKellyUK right now the label you're applying will only work for English language. What I'll suggest is either

  • extend the Joomla.getOptions('finder-search') to pass also the translated string for the label
  • or use a data-ul-label="<?php echo Text::_('SOME_STRING'); ?>" and grab the text from there

Although I'm the original author of the script (#21438) I think the whole script could be rewritten with way less code (eg the DOMContentLoaded event is harmful since the script is loaded either as a module or deferred, etc)

Thanks for the suggestion. Does this implementation look correct to you? 8eefbd4

@dgrammatiko
Copy link
Contributor

Thanks for the suggestion. Does this implementation look correct to you? 8eefbd4

LGTM

@brianteeman
Copy link
Contributor

The new string should probably just be "Search Results" - otherwise it will be "list of Results list"

@Hackwar Hackwar added the Small A PR which only has a small change label Feb 26, 2023
@Hackwar Hackwar added the bug label Apr 7, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:29
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.3-dev.

@obuisard
Copy link
Contributor

Awesomplete has not been updated in over 3 years, although the issue has been fixed in that library...

@dgrammatiko
Copy link
Contributor

Joomla is using the latest version:

"awesomplete": "^1.1.5",

https://www.npmjs.com/package/awesomplete?activeTab=versions

@obuisard
Copy link
Contributor

Joomla is using the latest version:

"awesomplete": "^1.1.5",

https://www.npmjs.com/package/awesomplete?activeTab=versions

Indeed, which means the fix that has been done in that library is not included in Joomla, therefore the fix here is still relevant.

@brianteeman
Copy link
Contributor

i must be missing something as i dont see any fix upstream just an open issue

@obuisard
Copy link
Contributor

i must be missing something as i dont see any fix upstream just an open issue

No problem, Brian @brianteeman. My understanding is this:

At the Awesomplete project, has been created a PR LeaVerou/awesomplete#17200 that fixes the issue we are having in Joomla. Because our issue comes from the library we are using apparently. The PR that fixes the issue in Awesomplete has been merged, but after 1.1.5 was released, which is the version we are using (but also the last official update).

@HLeithner HLeithner changed the base branch from 4.3-dev to 4.4-dev September 30, 2023 22:44
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.4-dev.

@drmenzelit drmenzelit self-assigned this Oct 24, 2023
@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Feb 21, 2024
@HLeithner HLeithner changed the title Awesomplete accessibility fix [4.4] Awesomplete accessibility fix Apr 24, 2024
@MacJoom MacJoom self-assigned this Jul 23, 2024
@drmenzelit
Copy link
Contributor

After years of inactivity there is a new version of Awesomplete available: 1.1.7
https://www.npmjs.com/package/awesomplete?activeTab=versions

We should check, if we can update it in Joomla. @dgrammatiko what do you think?

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Aug 10, 2024

The 1.1.7 has solved this with: LeaVerou/awesomplete@3c7ee84

	configure(this, {
		minChars: 2,
		maxItems: 10,
		autoFirst: false,
		data: _.DATA,
		filter: _.FILTER_CONTAINS,
		sort: o.sort === false ? false : _.SORT_BYLENGTH,
		container: _.CONTAINER,
		item: _.ITEM,
		replace: _.REPLACE,
		tabSelect: false,
		listLabel: "Results List", // <-- this
		statusNoResults: "No results found",
		statusXResults: "{0} results found", // uses index placeholder {0}
		statusTypeXChar: "Type {0} or more characters for results"
	}, o);

So this PR should just pass the listLabel: Text._('COM_FINDER_SEARCH_FORM_LIST_LABEL') to the initialization of the script. Probably use a FORM_FIELD_AUTOCOMPLETE_RESULTS or something similar (in the global translation files).
Probably it is easier to do another PR

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Aug 13, 2024
@HLeithner HLeithner changed the base branch from 4.4-dev to 5.2-dev November 15, 2024 13:22
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title [4.4] Awesomplete accessibility fix [5.2] Awesomplete accessibility fix Nov 15, 2024
@Hackwar
Copy link
Member

Hackwar commented Jan 16, 2025

@JackKellyUK Can you fix the conflicts and also implemented the requested changes?

@richard67
Copy link
Member

Closing in favour of #44756 .

@JackKellyUK Thank you for your contribution.

@richard67 richard67 closed this Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a11y Accessibility bug Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PBF Pizza, Bugs and Fun PR-4.4-dev Small A PR which only has a small change Updates Requested Indicates that this pull request needs an update from the author and should not be tested.

Projects

None yet

Development

Successfully merging this pull request may close these issues.