- Issue created by @primsi
- 🇦🇹Austria drunken monkey Vienna, Austria
drunken monkey → made their first commit to this issue’s fork.
- 🇦🇹Austria drunken monkey Vienna, Austria
Thanks, great suggestion!
The code also looks pretty good already. Since you already helpfully added the label to the test view I just went and also added assertions for that.I do have two questions, though:
- In the documentation you linked it is suggested that the
role
andaria-label
attributes should actually be on the<form>
(or other container) element, not on the input field. Is there a particular reason why you placed it on the input field instead?
I do agree that it makes sense to only activate this behavior if there is an exposed “Search: Fulltext search” filter (people use the Search API to build lots of things that aren’t searches), but I think just manipulating$form['#attributes']
inSearchApiFulltext::buildExposedForm()
should work fine to still put the attributes on the form itself in this case. It’s then just a bit unclear whether that filter is really the correct place to have the configuration for the ARIA label. (Otherwise, what should happen when there are multiple such filters?) - Speaking of the configuration: Is there a reason you put the
aria_label
option into the “root” options of the filter instead of nesting it withinexpose
? As this will only take effect when the filter is exposed, having it as part of the “expose” option seems to make more sense.
Anyways, thanks a lot again for your work on this, really a very good MR already!
- In the documentation you linked it is suggested that the
- 🇸🇮Slovenia primsi
Hi, this went off my radar. Thanks for the reminder.
Both are valid points and are mostly due to unfamiliarity with the subject. I've updated the MR.
- 🇸🇮Slovenia primsi
Hm, not sure about this test fails. Tried locally, but for me it fails before (at /var/www/html/web/modules/contrib/search_api/tests/src/Functional/ViewsTest.php:184).
- 🇦🇹Austria drunken monkey Vienna, Austria
Oh, darn, it actually seems like this is a bug in Drupal Core: I created 🐛 TypeError when having exposed form in block and setting "role" attribute on it Active to address it.
As this not only affects the tests, but can actually be reproduced on an actual site if you have an exposed form in a block, I fear we cannot actually merge this until the Core bug has been fixed and we depend on a version of Drupal Core that contains that fix.
I unfortunately don’t even think we can really work around this problem in this module without an unacceptable amount of code.