- 🇦🇷Argentina abelpzl
Testing 2975863-26-taxonomy-overview-filter.patch found it works well overall but reloading the form after performing a search produced the following errors:
Notice: Undefined offset: -1 in Drupal\taxonomy\Form\OverviewTerms->buildForm() (line 200 of core/modules/taxonomy/src/Form/OverviewTerms.php).
Notice: Undefined index: term in Drupal\taxonomy\Form\OverviewTerms->buildForm() (line 258 of core/modules/taxonomy/src/Form/OverviewTerms.php).
Notice: Trying to access array offset on value of type null in Drupal\taxonomy\Form\OverviewTerms->buildForm() (line 258 of core/modules/taxonomy/src/Form/OverviewTerms.php). - 🇦🇷Argentina abelpzl
I share my solution to the errors named in #28 ✨ Taxonomy overview list with exposed filter Active
This patch is applicable to D9.5.3
Keep in mind that in this patch I change the word comparison criteria with respect to the other patches.
-
In the future I would like to work on a custom module that adds the search to the list of terms. For the moment this solves my problem with a very long list of terms. - 🇨🇦Canada gurjinder_pabla Canada 🍁
I also wanted similar implementation for Drupal 9.4.8 and hence had to update the patch based on the code.
- 🇩🇪Germany szeidler Berlin
Here's a reroll for 10.2.x . Note, that has been a change committed on that branch (after 10.2.0 release) that makes the overview page not load the whole taxonomy term entities anymore, but uses the terms as generic php classes, which needed some adoptions in the patch 🐛 Avoid loading all terms on the taxonomy overview form Needs work
This patch does not work with 10.2.0, but will work with 10.2.1 (unless there are more changes to come).
- 🇫🇷France dydave
Hi everyone,
Thank you very much for raising this issue and contributing a patch.
Just a quick comment to let you know I was brought here trying to fix the following D10 compatibility issue:
Class Drupal\module_name\Form\ModuleNameOverviewTerms extends @internal class Drupal\taxonomy\Form\OverviewTerms.
With a custom class that we had that was extending the core
class Drupal\taxonomy\Form\OverviewTerms
, which isn't possible anymore and not recommended (due to @internal), for more details, see for example:
https://drupal.stackexchange.com/questions/287378/extending-core-classes...We had several filters actually on various custom fields, so using this patch wasn't flexible enough.
In the end, we decided to go for a
hook_form_alter
implementation, which worked very well:
It is possible to add any types of fields, custom submit operations and handlers.Hope that helps others with similar types or requirements and issues.
Thanks again for your great help with this issue. - 🇦🇷Argentina abelpzl
I share a patch applicable to Drupal 10.2.2
I based it on the previous patches. Add a "Clear filter" button.
This patch solved my problem, but I haven't tested it enough to ensure it works correctly in all cases.Is it possible that this filtering functionality will be part of the Core in the future?
- 🇺🇸United States billdaff
I'm encountering a styling issue with this that I can't quite figure out, see image. The input is too close to buttons. I think it has to do with the form container but I am not sure how this is normally done
- 🇩🇪Germany szeidler Berlin
I encountered the issue with hierarchical terms. Whenever I search for a term, that has parents (so is nested further down the tree), I get a
Notice: Undefined offset: -1 in Drupal\taxonomy\Form\OverviewTerms->buildForm()
. So it tries to find element -1 in an numbered array, which does not exist. In addition does the term appear twice (with the same ID).Here's an updated patch against 10.2.x
- 🇺🇸United States paulmckibben Atlanta, GA
The patch from #38 works great for me in Drupal 10.3.2. Thanks @szeidler!
- Status changed to Needs review
4 months ago 6:29am 30 August 2024 - Status changed to Needs work
4 months ago 2:15pm 30 August 2024 - 🇺🇸United States smustgrave
Hello
Issue summary should be updated to follow standard template
Also all fixes should be in MRs vs patches
New feature will require test coverage
Thanks!
- 🇪🇸Spain vidorado Pamplona (Navarra)
vidorado → changed the visibility of the branch 2975863-8.9.x-taxonomy-overview-list to hidden.
- 🇪🇸Spain vidorado Pamplona (Navarra)
vidorado → changed the visibility of the branch 11.x to hidden.
- Status changed to Needs review
4 months ago 5:18pm 1 September 2024 - 🇮🇳India prashant.c Dharamshala
Overall, it looks great, however, I have a few points:
- Input is too close to the "Filter" button, on focus the focus highlight overlaps, checked with multiple themes.
- It would be good to have a placeholder as well to clarify what can be searched in it.
- 🇪🇸Spain vidorado Pamplona (Navarra)
Thanks for the helpful UX review, @prashant.c.
I've added a placeholder, which makes the filter's purpose clearer.
I couldn't figure out how to add a margin between the textfield and the filter button in a theme-agnostic way. It might be a theme-related issue. I've seen other pages in the configuration with the same issue.
- 🇮🇳India prashant.c Dharamshala
Thanks @vidorado for addressing this, if the margin one is present on other forms as well then it is out of scope for this. IMO it is good to go for RTBC.
- 🇬🇧United Kingdom malcomio
As noted on ✨ Add filter and sort capability to taxonomy overview page Closed: duplicate , it would also be useful to allow the list to be sorted, as well as filtered.
- 🇪🇸Spain vidorado Pamplona (Navarra)
@malcomio In my opinion, sorting is a bit tricky since it interferes with term weights. In fact, I haven’t thoroughly tested what happens when you rearrange filtered terms.
Perhaps we should disable table dragging while filtering, which could provide a safer way to manage the order. :)
I'll look into it.
- 🇮🇳India prashant.c Dharamshala
I also do think that sorting would be tricky as the terms already have weights which might create confusion and for the scope of this issue Filtering feature only we should go with I guess.
- 🇬🇧United Kingdom malcomio
Good point - this page handles at least two use cases:
1. finding an individual term
2. changing the overall tree structureSorting and filtering is useful for 1, but problematic for 2
Drupal version 11.x-dev,
Patch applied 36- https://www.drupal.org/project/drupal/issues/2975863#comment-15614291 ✨ Taxonomy overview list with exposed filter Active
Steps followed1. Create long list of Vocabulary
2. You can use Devel to generate if you don’t have list.
3. This Feature is very useful in finding / filtering terms.
4. It also finds /Filters hierarchical terms.
5. Placeholder text is helpful it make it clear how to use.- 🇮🇳India prashant.c Dharamshala
Pipelines were failing for some unrelated reason, rebased the MR.
- 🇪🇸Spain vidorado Pamplona (Navarra)
I've disabled term reordering while filtering, as I believe this is safer to avoid strange things happening due to weight changes over a partial table.
I've also added a message to warn the user about the reordering not being available.
- Status changed to Needs work
3 months ago 5:34pm 11 September 2024 - 🇺🇸United States smustgrave
Pipeline issue needs to be resolved before review.
- Status changed to Needs review
3 months ago 11:36am 16 September 2024 - 🇺🇸United States smustgrave
Left some comments on the MR all small stuff.
Also UI changes should have before/after screenshot added to the "User interface changes" section of the summary.
- 🇺🇸United States smustgrave
Still an open thread. If you look at other tests that extend TaxonomyTestBase they inherit the docs.
- 🇪🇸Spain vidorado Pamplona (Navarra)
That's just what I was seeing :smile: Other tests don't inherit docs! :)
But I've added the statement, of course! All ready then!
- 🇮🇳India sagarmohite0031
Hello,
Verified this issue on D11, MR apply successfully.
Overall looks good but when we click on filter highlight overlaps with filter button and if term reordering disabled then its still there.Attaching screenshots -
- 🇪🇸Spain vidorado Pamplona (Navarra)
@prashant.c already noticed the textb9x overlapping issue in #48, and I found that the problem was hard to solve and out of scope.
I don't see the problem with the warning you squared in one of your screenshots. Could you explain it better?
Thanks!
- 🇮🇳India sagarmohite0031
Hello @vidorado,
I missed comment 57, I thought term reordering was disabled and why it's still there. That's why I was confused
Everything is fine. - Status changed to Needs work
19 days ago 7:11pm 3 December 2024 - 🇺🇸United States smustgrave
I rebased the MR as it was 350+ commits back and addressed the open thread about the missing inheritdoc
One think I noticed that seemed odd and think should be removed. When you filter the warning "Term reordering has been disabled because terms are being filtered. To enable term reordering, reset the filter." appears which I agree with.
But the filtered terms are highlighted like I have some unsaved changes, don't think the highlight is needed.
Tagging for usability review and will post in #ux on slack since the summary is very well done.
- 🇪🇸Spain vidorado Pamplona (Navarra)
@smustgrave Thank you for the rebase and the review!
The highlight purpose was to allow the user identify the real search results. It can be odd if you make a search for a string and you see multiple terms that don't match your query (i.e. the neccesary-to-put parents of the terms that did match).
Thanks also for posting this into #ux, let's see what the experts think about it :)
- 🇩🇪Germany rkoller Nürnberg, Germany
in regards of the highlighting one question. in the screenshot in the issue summary https://www.drupal.org/files/issues/2024-10-09/2975863-warning-about-reo... → you have both matches and none matches with the matches visually highlighted. with the MR applied i only see the matches (still visually highlighted) but no none matches shown? is that intended?
- 🇩🇪Germany rkoller Nürnberg, Germany
We discussed this issue at 📌 Drupal Usability Meeting 2024-11-29 Active . That issue will have a link to a recording of the meeting. For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, and @simohell.
First a few potential stumbling blocks we’ve identified during our initial testing:
- At the moment you have a
Filter
and aReset
button visible all the time. That pattern is sort of inconsistent with other places in Drupal Core. For example onadmin/content
you only see aFilter
button, theReset
button is only shown while a filter is being active. The other pattern available is for example onadmin/modules
, where you have noFilter
button at all, the module list is directly narrowed down and reflowed as soon as the first character is being entered in the filter field (which is not meeting SC3.2.2 -https://www.w3.org/WAI/WCAG22/Understanding/on-input.html). Instead of having aReset
button the filter field is using the browser’sClear
button. Unfortunately the support for theClear
button pattern is not that good across browsers, as outlined in the issue summary of ✨ Add a clear button to the fields ui Active . And we wanted to note that there is also an issue about the unification of the filter functionality across Core in general in the following issue ✨ Create Javascript library for searching rendered lists on the client. Needs work . - We considered the micro copy for the warning message (
Term reordering has been disabled because terms are being filtered. To enable term reordering, reset the filter.
) a bit too verbose. And warning messages in general are more about highlighting certain one time events, not about explaining a persistent inherent behavior a user is facing each and every time. - The way terms are labeled as matches have several a11y related problems. For one the background color that is labeling a match has a too low color contrast with
1.1:1
which is#FDF8ED
against#FFFFF
(SC 1.4.11). We’ve also quickly tested in VoiceOver (voiceover.mp4) and there is no indication in the aural interface to label that a row is a match. So the color is the sole way that communicates a match, which is not meeting SC1.4.1 (https://www.w3.org/WAI/WCAG21/Understanding/use-of-color.html). The width of the table header is also broader than the width of the rows when a filter is active (probably because the drag handles got hidden). That way the header and the rows are not aligned compared to if no filter is applied. And out of the scope for this issue, but I realized while recording the video for this summary, that not only the match state is not reflected in the aural interface, but also the level a term is within the term hierarchy is not reflected there either (voiceover.mp4). - It was also considered confusing to have a
Reset
button in the filter section and anotherReset to alphabetical
button at the bottom of the list next to theSave button
. With a filter active, and you the user pressing theReset to alphabetical
button, you get presented the following help textResetting a vocabulary will discard all custom ordering and sort items alphabetically.
on the confirmation page. Based on that help text the expectation and assumption would be that only the filtered results would be displayed in an alphabetical order. But not only the list gets switched back to an alphabetic order but also the filter gets reset - a detail that is not reflected in the help text. But in general it is sort of confusing in the first place to have to differently labeledReset
buttons with slightly different behavior within the same page.
The group concluded on the following points:
- We would recommend to hide the
Save
andReset to alphabetical
buttons at the bottom, anytime the filter is active. That avoids any ambiguities in regards of theReset
buttons plus theSave
button is sort of without any purpose while the user is unable to alter the term order. Strictly speaking you are currently having three reset buttons,Reset
andSave
simply reset the filter, whileReset to alphabetical
resets the filter and resets the order to alphabetical. - We had the clear consensus recommend trying to be more consistent with existing UI patterns in Core, and follow one of the aforementioned approaches used on either
admin/content
oradmin/modules
. While writing this comment and looking again at both approaches i’ve realized following the approach onadmin/modules
might not be the best choice due to the fact that it is not meeting SC3.2.2 plus it is relying on the browser’s clear button which has additional issues - after a brief followup discussion we've agreed on going with the pattern used onadmin/content
as our recommendation. - We would drop the highlighting of matching terms by color. Instead an icon could be added, which could be for example either a bullseye or a bullet in the position currently the drag handles are in when the filter functionality is inactive. The icon could have an alt text or aria-label communicating that the term in this row is a filter match and by using such a bullseye or bullet in the position of the drag handle when the filter is active the table header and the table underneath would stay seamlessly aligned without the shift it currently has (voiceover.mp4). And if such an icon solution is chosen it is recommended to add the following line, right before, on top of the table header:
[Icon] indicates matching terms. Parents of matching terms are also shown.
- In regards of the message currently communicated with a warning message the group agreed on shortening the string and making it more concise by changing it to:
Reordering is disabled while terms are filtered.
. The only detail we had not consensus on was how and where to present that message. From our perspective there are two viable options. Either keep the string in an admin message. If that is the case change the type from a warning to an info message, since nothing is broken or going wrong, it is simply an informal message about the inherent behavior of the interface on that page. But as a downside that message draws a lot of attention and uses a lot of screen real estate every time a filter is applied. The other option we’ve considered was to remove the admin message entirely and append the string to the help text ([vocabulary] contains terms grouped under parent terms. You can reorganize the terms in Taxo using their drag-and-drop handles.
). Downside with that approach, due to the fact that the primary button, the filter section and the table have a way higher affordance, drawing all the attention, the odds are high that this text is potentially glossed over and even missed entirely.
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
- At the moment you have a
- 🇪🇸Spain vidorado Pamplona (Navarra)
Wow! This is an awesome UX review! I will get to it in a couple of days.
Thanks a lot!
- 🇩🇪Germany rkoller Nürnberg, Germany
thanks for working on the changes @vidorado! i've taken a look at the latest state of the MR, in the following my personal opinion, not the opinion from the group that has discussed the issue last friday (even though i have shared my thoughts and the demo video in the thread for last weeks meeting on the drupal slack: https://drupal.slack.com/archives/C1AFW2ZPD/p1733493024401899).
point 1 and 2 i consider fixed and also the from my perspective the right approach to recommend. looks good me hiding the save and reset to alphabetical button as well as following the pattern used on admin/content which is hiding the reset button while no filter is entered yet.
point 3 and 4 probably need some more work. i've created a short video illustrating the recent changes including the aural interface with voiceover - vo.mp4.:
1) If you press the filter button after entering a filter string the focus is getting lost. So if you are keyboard user the focus isnt around the filter section in the DOM but at the beginning of the tabindex,.you notice after pressing the filter button the first element to focus is the
skip to main content
skip link instead for example thereset
button.2) I am completely uncertain what the best approach for communicating the filter matches is. i still am inline with our review that just adding a background color to the rows with matches has for one accessibility issues but also the problem that people associate those rows with a background color as "errors" because the color is the same or at least close to the color of the warning message, and the fact that a warning message was shown in the first place. but on the other hand i am also not sure if our suggested idea is the right choice and having just an icon at the beginning of the row is already clear enough. well first of all using a caret at the beginning has at least another problem, that caret a user associates with collapsible and expandible lists. i always think i am able to expand those two rows in the video. but i've tried to replace the caret with the bullet and the bulls eye suggestions we've made during the call as well as an arrow, an additional suggestion @simohell made during our followup discussion in the drupal slack, but none of those really clicks for me :/
Problem is, i've also checked the drupal design system and there is neither a perfect fit for an icon nor a defined component how filter matches should be communicated visually. i was hoping maybe i have missed anything the first time i was looking. i still think removing the warning message is the right step but "maybe" adding an icon (which still has to be decided on) AND having a background color might do the trick? or maybe we should ask the claro maintainers for design input?
3) the aural interface for the icon is not necessarily clear to a none sighted users without any visual cues.
black pointer
is not really clear if someone directly skims through a table not checking the help text upfront. and still if the person knows that help text, there is still a cognitive transfer that you have to transpose the announcement of black pointer equals to a match. i wonder even would it be viable to make the icon decorational while adding a visually hidden span to the name which prepends something like "Match" ? Not sure what others think?4) about the help text. i think i would move "Reordering is disabled while terms are filtered" up to the help text and make it available all the time. the legend/explanation i would keep there and only show with an active filter. the only thing about that legend, the brackets were only intended as a placeholder to highlight there an icon should be inserted. cuz now the brackets in combination with the caret create the association of a play button for me :D
- 🇩🇪Germany rkoller Nürnberg, Germany
and i forgot to change the status, needs some more work and discussion
- 🇪🇸Spain vidorado Pamplona (Navarra)
Thanks for the thorough review, @rkoller!
I’ve just committed a proposal that I believe could be acceptable:
- I’ve moved the warning just below the initial help message, as you suggested, while keeping it prominent with a strong, so that the majority of users don’t overlook it. New users would probably notice it even without the emphasis, but experienced site builders might unconsciously skip it, so I think the emphasis is necessary.
- I’ve placed the legend in the table caption. I’m not sure if I should add
aria-hidden="true"
, as it seems unnecessary for aural navigation. - I’ve changed the "play button" :) to an arrow. The arrow feels more natural to me than the bullseye, and it avoids the "accordion illusion" problem.
- I’ve fine-tuned the markup so screen readers announce "Matching term: xxx" or "Non-matching term: xxx" for each result.
- I’ve highlighted only the matching terms, not their entire row, and used a success color instead of a warning color. I believe this better conveys the intended meaning.
I’ve tested the navigation with the ChromeVox Chrome add-on, and it seems accessible to me. :)