- 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.
- Merge request !605#3458840:Consider adding Apply and Clear buttons to Categories dropdown → (Open) created by utkarsh_33
- 🇮🇳India utkarsh_33
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😅.
- 🇺🇸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?
- 🇮🇳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.
- 🇮🇳India utkarsh_33
All the feedbacks are addressed and tests are also passing.Marking it NR.
- 🇺🇸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 scrollsSomething 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.
- 🇮🇳India utkarsh_33
I have added CSS changes that fixes the position of the Apply and Clear button.See the SS:-
- 🇺🇸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?
- 🇮🇳India utkarsh_33
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.
- 🇮🇳India utkarsh_33
Attaching the latest SS for ease of reviewing the changes.
- 🇮🇳India utkarsh_33
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. - 🇺🇸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');
- 🇮🇳India utkarsh_33
I tried adding the fix
$this->getSession()->evaluateScript('document.getElementById("67").scrollIntoView()');
,but it's not working for all the tests.
- 🇩🇪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 onadmin/content
. the general behavior in the context of keyboard navigation i'll retest later today. - Status changed to Needs work
4 months ago 7:45am 24 December 2024 - 🇮🇳India utkarsh_33
I tried to get the other tests pass but they are not working.
- 🇺🇸United States chrisfromredfin Portland, Maine
No API considerations, moving to stable. HOWEVER, leaving on the SHOULD HAVEs for -beta1.
- 🇺🇸United States chrisfromredfin Portland, Maine
I wonder if @lostcarpark could turn this part of the testing into a Nightwatch test? I'm not sure if that would actually help or not. Maybe its scrollIntoView works better, or it handles clicking things that aren't visible better...?
- 🇺🇸United States phenaproxima Massachusetts
This is extremely out of date, sadly, and needs to be completely refactored to be brought in line with the way the Svelte code currently works, with . I think we can certainly write an automated test of this.
- 🇮🇪Ireland lostcarpark
Agree this is beyond a rebase. Needs a new branch from 2.0, and for the changes to be reworked to match.
- First commit to issue fork.
- Merge request !792#3458840 "Rebased and added apply clear buttons categories" → (Open) created by libbna
- 🇮🇳India libbna New Delhi, India
I created a new branch, rebased it and applied the changes manually.
Adding the screenshot how the buttons are looking.I have unassigned the issue so that anyone else can work on tests.
- 🇺🇸United States chrisfromredfin Portland, Maine
chrisfromredfin → changed the visibility of the branch 3458840-consider-adding-apply to hidden.
- 🇺🇸United States chrisfromredfin Portland, Maine
Thank you, libbna! Good progress. A couple things I'm noticing.
(1) The apply/clear bar should be "sticky" at the bottom of the container that opens. That is, it should always be visible and only the list of checkboxes should be scrollable. That can probably also be a little less tall. Maybe something like a height of whatever 4.5 rows is, with maybe a max-height of like 90vh or something?
(2) the "Clear" button should act as a submit button. So, while it clears the checkboxes, you still have to scroll back down and click Apply, which shouldn't actually be necessary. Hitting clear should wipe out all the category selections and cause a projects refresh.
- First commit to issue fork.
- Merge request !797Project browser 3458840 3458840 apply clear buttons categories → (Open) created by snehalgaikwad
Addressed both points mentioned in #36 📌 Consider adding Apply and Clear buttons to Categories dropdown Active . Now both buttons will remain sticky, and only the options will be scrollable. Also, the clear button will clear the selected options (no need to click on apply after clear).
Keeping assigned to myself to work on failed pipeline issues.