Consider adding Apply and Clear buttons to Categories dropdown

Created on 3 July 2024, 6 months ago

Problem/Motivation

📌 Improve the categories filter type in context to the rest of the filter component ui Fixed will likely be committed with a few shortcomings to keep scope reasonable.

One of those is that when someone is selecting multiple categories, it does a load each time they make a click. It's kind of a bet to hedge to see whether "most people" will click one category, or most want to click multiple. We could potentially solve this and optimize for multiple selections if we added an Apply (and Clear/Reset) button at the bottom of the categories dialog/dropdown.

Proposed resolution

First, discuss if we need/want to do this, or leave the behavior intact.

If we prefer the Apply button method, implement apply and clear buttons at the bottom of the dialog, and wire the behavior so that the filter is only implemented when hitting apply. NOTE that this would only pertain to the categories dialog, NOT the filter set as a whole.

📌 Task
Status

Active

Version

2.0

Component

User experience

Created by

🇺🇸United States chrisfromredfin Portland, Maine

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @chrisfromredfin
  • 🇮🇪Ireland lostcarpark

    One thing to bear in mind is the buttons could make things more difficult for keyboard users. Some thought would need to be given to make sure they aren't disadvantaged.

  • 🇩🇪Germany rkoller Nürnberg, Germany

    I've added the tag for wcag2.2. sc 3.2.2 https://www.w3.org/WAI/WCAG22/Understanding/on-input.html which basically illustrates why the proposed resolution, having an apply and clear button, is important or probably necessary.

    re #2 📌 Consider adding Apply and Clear buttons to Categories dropdown Active in what regard would/could the buttons make things more difficult for keyboard users?

  • 🇺🇸United States chrisfromredfin Portland, Maine

    I agree that the use of apply/clear buttons would enhance the UX. Going to add this to the roadmap.

  • 🇮🇪Ireland lostcarpark

    @rkoller At the moment the keyboard user can tab onto the category box, press space to open it, then use up/down arrows to move between categories, and space to select/deselect them. Pressing tab will close the drop-down and move to the next control.

    I agree that having the modules change behind the drop-down as we currently do is confusing. We just need to ensure there is a clear an intuative UI for getting from the list of categories to the apply/reset buttons.

  • First commit to issue fork.
  • Pipeline finished with Failed
    2 months ago
    Total: 459s
    #316814
  • Pipeline finished with Failed
    2 months ago
    Total: 467s
    #317745
  • Pipeline finished with Failed
    2 months ago
    Total: 498s
    #317832
  • Pipeline finished with Failed
    about 2 months ago
    Total: 388s
    #317883
  • Pipeline finished with Failed
    about 2 months ago
    Total: 477s
    #317894
  • Pipeline finished with Failed
    about 2 months ago
    Total: 345s
    #317904
  • Pipeline finished with Success
    about 2 months ago
    Total: 459s
    #317913
  • Pipeline finished with Failed
    about 2 months ago
    Total: 491s
    #318093
  • Pipeline finished with Success
    about 2 months ago
    Total: 347s
    #318141
  • Pipeline finished with Failed
    about 2 months ago
    Total: 488s
    #318942
  • Pipeline finished with Failed
    about 2 months ago
    Total: 561s
    #318987
  • Pipeline finished with Failed
    about 2 months ago
    Total: 601s
    #318995
  • Pipeline finished with Success
    about 2 months ago
    Total: 420s
    #319131
  • Pipeline finished with Failed
    about 2 months ago
    Total: 298s
    #319198
  • Pipeline finished with Failed
    about 2 months ago
    Total: 818s
    #319208
  • Pipeline finished with Failed
    about 2 months ago
    Total: 488s
    #320114
  • Pipeline finished with Failed
    about 2 months ago
    Total: 581s
    #320160
  • This is now down to nightwatch test failures.So I'll try to resolve them else i will post a detailed description of the problem with the tests and maybe solution😅.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 489s
    #320284
  • Pipeline finished with Failed
    about 2 months ago
    Total: 307s
    #320299
  • Pipeline finished with Success
    about 2 months ago
    Total: 918s
    #320301
  • Pipeline finished with Failed
    about 2 months ago
    Total: 1022s
    #328685
  • Pipeline finished with Failed
    about 2 months ago
    Total: 566s
    #328722
  • Pipeline finished with Failed
    about 2 months ago
    Total: 228s
    #328729
  • Pipeline finished with Success
    about 2 months ago
    Total: 349s
    #328731
  • 🇺🇸United States phenaproxima Massachusetts

    I don't see a ton here to complain about from the code perspective (I haven't manually tested it), but I do have some questions about using styling classes to hook into functional behaviors. That seems like a red flag to me. But maybe I'm missing something?

  • Pipeline finished with Failed
    about 2 months ago
    Total: 486s
    #329720
  • Pipeline finished with Failed
    about 2 months ago
    Total: 291s
    #329759
  • Pipeline finished with Failed
    about 2 months ago
    Total: 316s
    #329765
  • Pipeline finished with Failed
    about 2 months ago
    Total: 360s
    #329769
  • 🇮🇳India narendraR Jaipur, India

    Found some issues while testing this MR

    • Select/unselect a category multiple times and then click Apply. Category is showing multiple times in label.
    • Unable to unselect a category from keyboard.
    • Clicking Clear does not remove selected categories.
    • Unable to select multiple categories after clicking apply.
  • Pipeline finished with Failed
    about 1 month ago
    Total: 418s
    #334987
  • Pipeline finished with Failed
    about 1 month ago
    Total: 363s
    #335073
  • Pipeline finished with Failed
    about 1 month ago
    Total: 335s
    #335165
  • Pipeline finished with Success
    about 1 month ago
    Total: 594s
    #335216
  • Pipeline finished with Failed
    about 1 month ago
    Total: 417s
    #335881
  • Pipeline finished with Success
    about 1 month ago
    Total: 1038s
    #335941
  • All the feedbacks are addressed and tests are also passing.Marking it NR.

  • Pipeline finished with Success
    about 1 month ago
    Total: 441s
    #337401
  • 🇺🇸United States chrisfromredfin Portland, Maine

    I have one small UI request. I believe that we need to keep the Apply/Clear buttons visible always as long as the dropdown is shown.

    Right now, they are at the very bottom of the scrolling list in the window, and it will not be obvious that someone will need to click that to make the filter work. I would like to see:

    [ Dropdown \/ ]
    [ Categories here ] - this part is scrollable
    [ Apply | Clear ] - Always fixed at the bottom of the popup when someone scrolls

    Something like this:

  • 🇩🇪Germany rkoller Nürnberg, Germany

    +1 for making the buttons visible all the time. but there is one potentially tricky problem to consider, the user is navigation the multiselect by the up and down arrow key. how should the transition between the options in the select list and the apply and clear button be accomplished? at the moment if you navigate down to the end of the list the apply button gets into focus but you are unable to reach the clear button by the arrow keys. and if you press the tab key the security advisory coverage select gets into focus. on shift focus you get onto the filter button that was unreachable before. but how to handle things for keyboard users with the position fixed of that apply and clear button i consider tricky.

  • I have added CSS changes that fixes the position of the Apply and Clear button.See the SS:-

  • Pipeline finished with Failed
    about 1 month ago
    Total: 471s
    #338362
  • Pipeline finished with Failed
    about 1 month ago
    Total: 444s
    #338373
  • Pipeline finished with Failed
    about 1 month ago
    Total: 562s
    #338412
  • Pipeline finished with Failed
    about 1 month ago
    Total: 404s
    #338522
  • Pipeline finished with Failed
    about 1 month ago
    Total: 354s
    #338530
  • Pipeline finished with Success
    about 1 month ago
    Total: 330s
    #338535
  • Pipeline finished with Success
    about 1 month ago
    Total: 346s
    #340265
  • Pipeline finished with Success
    about 1 month ago
    Total: 339s
    #340268
  • 🇺🇸United States chrisfromredfin Portland, Maine

    @utkarsh_33 With many, many apologies, I think I really beefed this. I tried to rebase, which of course meant force-pushing. I then tried to restore, but I think my restore point was prior to these final fixes. If you still have the latest you did locally, can you please force-push it?

    Changes remaining that I want:
    - max-height of the dropdown should be "50vh" and not a pixel-based value
    - Apply and clear buttons should not be space-between, but right next to each other, with clear being 0.5rem away (get rid of justify-content and replace with "gap: 0.5rem;" should do it.

    That's it for aesthetic fixes; but then we need to address the a11y of keyboard navigation. I THINK the right/logical thing would be IF it's open, we should be able to tab to the apply/clear buttons. If they're closed, tab should jump to the next filter. (??? ¯\_(ツ)_/¯ )

  • 🇩🇪Germany rkoller Nürnberg, Germany

    in regards of aesthetics two details. first, i am unable to test the current state since it is broken or at least the buttons are not showing anymore at the moment, but in my previous tests i'Ve noticed a regression that is illustrated in #15 📌 Consider adding Apply and Clear buttons to Categories dropdown Active . we ve agreed on a previous issue that the user should always see only the half of the last visible option if there are more options than vertical available space. that way the user directly knows the list is scrollable. in the screenshot in #15 you see the last visible option fully visible instead. that way it is not directly apparent that the list is visible.

    and i wonder if it would make sense to align the background color of that box/bar that contains those two buttons with the dark of the bulk action bar? to visualize that this is a button bar and not part of the multi select list anymore, to make it visualy distinguished from the actual list?

    and in regards of the keyboard navigation. would it make sense to move the details about the keyboard navigation into 📌 Improve keyboard navigation for categories dropdown Active if the rest of this issue is ready?

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 289s
    #341754
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 96s
    #341755
  • Pipeline finished with Failed
    about 1 month ago
    Total: 380s
    #341757
  • Pipeline finished with Failed
    about 1 month ago
    Total: 520s
    #341767
  • I have adjusted the height to 50vh and also the spacing between the buttons as mentioned in #17 📌 Consider adding Apply and Clear buttons to Categories dropdown Active . Also i the concern raise by @rkoller in #18 📌 Consider adding Apply and Clear buttons to Categories dropdown Active is fixed related to

    last visible option fully visible

    along with the black background colour for the div containing the 2 buttons (which matches the bulk operation) and now it will be easier for the user to distinguish from the actual list.

    Also regarding

    and in regards of the keyboard navigation. would it make sense to move the details about the keyboard navigation into #3458844: Improve keyboard navigation/general a11y for categories dropdown if the rest of this issue is ready?

    I think we could do that as a part of the that issue.

  • Attaching the latest SS for ease of reviewing the changes.

  • The tests are failing because they are not able to interact with the elements(by scrolling).I can't figure out how does the addition of one more element in the scroll list is causing this issue.
    I tried adjusting the z-index for the apply and clear button's div but that also didn't help to fix the problem because as you can see the css of the apply and clear button's div is causing the issue (see the SS).

    For better understanding i changed the background colour.
    I also tried to move the apply and clear button out of the select list(which is not actually we want) but that seems to pose a greater problem as you can see the SS.

    If anyone has any idea of what could be reason for the test fails or what else can i try to fix the problem, it would be helpful if they can document it on the issue or give it a try to fix the problem.
    Thanks in advance.

  • Pipeline finished with Failed
    about 1 month ago
    #346646
  • Pipeline finished with Failed
    30 days ago
    Total: 489s
    #346719
  • 🇺🇸United States chrisfromredfin Portland, Maine

    https://drupal.slack.com/archives/C01UHB4QG12/p1732655176738539

    The problem, which I think you know, is that the element is hidden in the scrollable div that's above the buttons, so you can't ->focus() it or ->click() it.

    So, I think you would have to somehow figure out how to take that element and scroll it into view. I have tried looking at Mink documentation, and there doesn't seem to be a method to do this. However, I know there's a JavaScript method for it. I think there is a way to execute raw JS during the test, like we do in getElementText() of the ProjectBrowserUiTestTrait.php:

    $this->getSession()->evaluateScript(...);

    The following diff seems to at least fix the testCategoryFiltering() method, for example.

    diff --git a/tests/src/FunctionalJavascript/ProjectBrowserUiTest.php b/tests/src/FunctionalJavascript/ProjectBrowserUiTest.php
    index 775c0a5..0ce1360 100644
    --- a/tests/src/FunctionalJavascript/ProjectBrowserUiTest.php
    +++ b/tests/src/FunctionalJavascript/ProjectBrowserUiTest.php
    @@ -131,6 +131,7 @@ class ProjectBrowserUiTest extends WebDriverTestBase {
         $this->clickWithWait('.pb-filter__multi-dropdown', 'E-commerce', TRUE);
     
         // Click 'E-commerce' checkbox.
    +    $this->getSession()->evaluateScript('document.getElementById("104").scrollIntoView()');
         $this->clickWithWait('#104', '', TRUE);
         $this->assertNotEmpty($assert_session->waitForButton('Apply'));
         $this->getSession()->getPage()->pressButton('Apply');
    @@ -164,9 +165,11 @@ class ProjectBrowserUiTest extends WebDriverTestBase {
         $this->assertSession()->waitForText('Media');
     
         // Click 'Media' checkbox.
    +    $this->getSession()->evaluateScript('document.getElementById("67").scrollIntoView()');
         $this->clickWithWait('#67', '', TRUE);
     
         // Click 'E-commerce' checkbox.
    +    $this->getSession()->evaluateScript('document.getElementById("104").scrollIntoView()');
         $this->clickWithWait('#104', '', TRUE);
         $this->assertNotEmpty($assert_session->waitForButton('Apply'));
         $this->getSession()->getPage()->pressButton('Apply');
    
  • I tried adding the fix

    $this->getSession()->evaluateScript('document.getElementById("67").scrollIntoView()');

    ,but it's not working for all the tests.

  • Pipeline finished with Failed
    20 days ago
    Total: 507s
    #356176
  • 🇩🇪Germany rkoller Nürnberg, Germany

    a brief note about color contrast, the apply button has a too low color contrast. I would go with the same color suggested on the bulk action issue, --color-blue-400 which is already used on the bulk action bar on admin/content. the general behavior in the context of keyboard navigation i'll retest later today.

Production build 0.71.5 2024