Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1222 Filters (Layered Navigation) - Add keyboard focus and adjust the way tab order should work #3034

Conversation

vasilii-b
Copy link

@vasilii-b vasilii-b commented Mar 3, 2021

Description

This PR adds the a11y changes to the Layered Navigation (Filters) modal on the category pages according to the specifications in #1222 .

Related Issue

Closes #1222.

Acceptance

Verification Stakeholders

@dpatil-magento @sirugh

Specification

Verification Steps

  1. Go to any category page that contains products and has filter items.
  2. Open the Filters modal.
  3. Make sure Filters modal opens.
  4. Make sure the modal's close (x) icon is focused.
  5. From the keyboard, press the Tab key.
  6. Make sure the focus switches to the next element. E.g filter item (Price).
  7. Press the Tab key. Make sure the focus switches to the next filter item. E.g. "Color".
  8. Press the Space key on the filter item "Color". The options should show. E.g. "Purple" and "Yellow"
  9. Press the Space key. You should navigate through "Color" options.
  10. Press the Tab key a few times until you focus the next filter item. E.g. "Material".
  11. Press the Tab key a few more times until you focus the "Close" button again (that's when no filter option applied).

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added translations for new strings, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Mar 3, 2021

Messages
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 3551640

@vasilii-b
Copy link
Author

ℹ️ plan to proceed with the failed tests once the main changes are approved;

For now, will work on the spotted bug:

  • after applying a filter item, the focus goes to the modal's "Close" icon.

@vasilii-b vasilii-b changed the title #1222 Filters (Layered Navigation) - Add keyboard focus and the way tab order should work #1222 Filters (Layered Navigation) - Add keyboard focus and adjust the way tab order should work Mar 4, 2021
@vasilii-b
Copy link
Author

update

✅ The aforementioned bug has been addressed

@sirugh
Copy link
Contributor

sirugh commented Mar 4, 2021

ℹ️ plan to proceed with the failed tests once the main changes are approved;

Sounds good - I've moved the ticket to the "ux review" column. Once approved by UX it will move on to dev review.

@vasilii-b
Copy link
Author

@sirugh may I ask to re-review this?
Added the latest updates according to the discussion in #1222 .
Thanks.

@dpatil-magento
Copy link
Contributor

dpatil-magento commented May 6, 2021

@sirugh Please check following -

  1. It has broken [Fix bug]: Filter and Sort buttons should display at same time #2681 (Nice to have unit test if possible :) )
  2. On Safari, after clearing a filter using space key, focus is going back to browser URL bar
    Image from Gyazo

@sirugh
Copy link
Contributor

sirugh commented May 6, 2021

It has broken #2681 (Nice to have unit test if possible :) )

I'll fix this.

@sirugh
Copy link
Contributor

sirugh commented May 6, 2021

On Safari, after clearing a filter using space key, focus is going back to browser URL bar

Interesting. I noted on Chrome that we also "lose" focus, though pressing tab goes to the top-most filter tab ie "Price". @vasilii-b do you have any ideas on how to fix this?

Edit: Actually, looking at #3137 you can see in the demo https://pr-3137.pwa-venia.com/venia-bottoms.html?page=1&price%5Bfilter%5D=0-100%2C0_100 that it behaves sort of the same. IMO we should open a bug about "moving focus to next element when a filter is removed". Probably not worth holding this PR up anymore.

@dpatil-magento
Copy link
Contributor

dpatil-magento commented May 6, 2021

@sirugh When there are 0 filter results then sort button should not be displayed but its displaying. MFTF test failing, please check.

image

Signed-off-by: sirugh <rugh@adobe.com>
@sirugh
Copy link
Contributor

sirugh commented May 6, 2021

@sirugh When there are 0 filter results then sort button should not be displayed but its displaying. MFTF test failing, please check.

This is confusing, so we should show sort/filter button together, unless there are zero results in which case we should show the filter button but not the sort button?

@sirugh
Copy link
Contributor

sirugh commented May 6, 2021

In my opinion we should ignore #2681 as rendering both together doesn't make sense.

sirugh added 2 commits May 7, 2021 08:44
Signed-off-by: sirugh <rugh@adobe.com>
@sirugh sirugh force-pushed the feature/1222-layered-nav-add-keyboard-focus-and-a11y branch from 3ce8e55 to 9661347 Compare May 7, 2021 13:54
tjwiebell
tjwiebell previously approved these changes May 7, 2021
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 tiny bit of whitespace in test name, but ready for another round of QA.

sirugh and others added 3 commits May 7, 2021 10:08
Co-authored-by: Tommy Wiebell <twiebell@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
tjwiebell
tjwiebell previously approved these changes May 7, 2021
Signed-off-by: sirugh <rugh@adobe.com>
@dpatil-magento
Copy link
Contributor

dpatil-magento commented May 7, 2021

QA Approved, @tkacheva fyi ignored #2681

@tkacheva
Copy link
Contributor

Looks good from product perspective

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partner: Atwix partners-contribution pkg:peregrine pkg:venia-ui Progress: done version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature]: Filters (Layered Navigation) - Add keyboard focus and the way tab order should work
7 participants