-
Notifications
You must be signed in to change notification settings - Fork 10.8k
[Accessible] Make tabs on product page accessible #53158
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
Conversation
Hi , Apart from reviewing the code changes, please make sure to review the testing instructions and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. You can follow this guide to find out what good testing instructions should look like: |
…focus is on the stars or tabs.
@dkotter This PR is ready for code review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Manussakis, works as expected! Thanks you for working on it. Could you mark the PR ready for review and remove WIP from PR title? Unless there's something missing?
Once marked as ready for review I'm happy to approve and merge 🚀
@kmanijak Thanks for testing this PR! Before removing it from the draft, let's wait for @dkotter and @qasumitbagthariya from 10up to review it too. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ankitguptaindia @qasumitbagthariya This is ready for QA
QA/Test Report ✅I have checked this PR in the
Screen.Recording.2024-11-28.at.6.07.10.PM.movTesting Environment
@Manussakis Please remove [WIP] from the title and take this PR out of draft. cc @vikrampm1 |
@kmanijak This PR is ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Manussakis, thanks for the ping! I'm just not sure I understand one case you covered. Do you mind elaborating? 🙌
.on( 'focusout', '.wc-tabs li a, ul.tabs li a, #respond p.stars a', function() { | ||
setTimeout( function () { | ||
var $activeElement = $( document.activeElement ); | ||
var sliderKeyupBlockers = [ '.stars', '.tabs', '.wc-tabs']; | ||
var $closestBlocker = $activeElement.closest( sliderKeyupBlockers.join( ', ' ) ); | ||
|
||
if ( $closestBlocker.length ) { | ||
// Prevent keyup events from being triggered on the flexslider when the focus is on the stars or tabs. | ||
productGalleryElement.data('flexslider').animating = true; | ||
return; | ||
} | ||
|
||
productGalleryElement.data('flexslider').animating = false; | ||
}, 0); | ||
} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what case does it cover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code fixes this issue that @qasumitbagthariya found on the #53018 PR.
The seTimeout
function is necessary to ensure the correct active element is returned, otherwise, the body
element is returned.
Please let me know if you have any questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 folks, it looks like that line 84 creates a regression: #57292
When the user clicks on the description and reviews tabs, the animation is set to true
(and never reset) and this caused that this logic never runs and so the animation isn't triggered.
I'm going to see how we can fix it, but sharing in case you have a better idea to avoid setting animating
to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR created: #57373
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gigitux
According to my investigation, the issue is that when the focus is on the tab items (Description, Review,...), clicking on the slider, either on the thumbnails or on the active image, doesn't trigger the focusout event from the focused tab item, only if you click on any other element outside it.
Once the focus moves away from the tab, the slider works back.
Blocking the slider when the tab is active is intended, as this avoids the slides from moving when the user is using the keyboard to navigate through the tabs' content.
I am also investigating it and will keep you posted.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code fixes this issue that @qasumitbagthariya found on the #53018 PR.
The seTimeout function is necessary to ensure the correct active element is returned, otherwise, the body element is returned.
Please let me know if you have any questions.
Appreciate the explanation! LGTM, thank you!
I end up working on a fix here: #53877 |
* Add aria attributes to tab link on product page * Make product tabs keyboard accessible * Add changelog file * Fix tab focus style on TT1 theme * Bump tabs template version * Prevent keyup events from being triggered on the flexslider when the focus is on the stars or tabs. * Update test to match the new tab role * Fix slider when clicking on the thumbnail with focus on tab or stars --------- Co-authored-by: Karol Manijak <[email protected]>
👋 Hey folks, I just came across this issue. According to the Lighthouse report, it seems this PR lowers the accessibility score of the Single Product Page. @Manussakis is it something that you can investigate? |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #51924 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment