-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fixed : Modal dialog: small improvement for elementShouldBeHidden #65941
Fixed : Modal dialog: small improvement for elementShouldBeHidden #65941
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @vk17-starlord! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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.
Added one suggestion please check.
…return statement
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.
LGTM
@vk17-starlord can you please add this PR info to changelog.md, it will go under |
element.hasAttribute( 'aria-hidden' ) || // Skip elements marked as aria-hidden | ||
element.hasAttribute( 'aria-live' ) || // Skip live regions | ||
( role && LIVE_REGION_ARIA_ROLES.has( role ) ) | ||
) // Skip elements with live region roles |
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 comment should be moved to the appropriate line. However, I'd consider to remove all these comments because the code is pretty self-explanatory.
packages/components/CHANGELOG.md
Outdated
@@ -3,7 +3,7 @@ | |||
## Unreleased | |||
|
|||
### Bug Fixes | |||
|
|||
- `Modal` : Modal dialog small improvement for elementShouldBeHidden - PR ([#65941](https://github.com/WordPress/gutenberg/pull/65941)). |
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.
The changelog entry should follow a specific format:
- Please remove the space before the colon.
- Please remove the
- PR
. - I'd consider to move the entry to the
Enhancements
section as it's not a bug fix.
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.
Thank you @vk17-starlord from a code perspective lookks good to me.
I left two small comments to address minor things, please have a look when you have a chance.
@afercia have a look at latest commit , i have implemented the suggestions you gave |
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.
LGTM, can be merged after tests pass.
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.
I spoke to soon. There's now a merge conflict in packages/components/CHANGELOG.md
that needs to be solved.
@afercia anything left to be done from my side ? |
@vk17-starlord yes: the changelog entry must be in the |
…to fix/Modal-dialog-#65829
@afercia done with changes !! |
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.
LGTM to me.
Thanks @vk17-starlord
…5941) * Fixed : Modal dialog: small improvement for elementShouldBeHidden #65829 * Refactor `elementShouldBeHidden` to combine all checks into a single return statement * Added : PR description in changelog.md * Removed comments from function and fixed changelog * Removed modal details from bug fixes and moved to enhancements * Add missing empty line to changelog. * moved : PR details to enhancement section * Add missing empty line to changelog. --------- Co-authored-by: vk17-starlord <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: up1512001 <[email protected]>
…rdPress#65941) * Fixed : Modal dialog: small improvement for elementShouldBeHidden WordPress#65829 * Refactor `elementShouldBeHidden` to combine all checks into a single return statement * Added : PR description in changelog.md * Removed comments from function and fixed changelog * Removed modal details from bug fixes and moved to enhancements * Add missing empty line to changelog. * moved : PR details to enhancement section * Add missing empty line to changelog. --------- Co-authored-by: vk17-starlord <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: up1512001 <[email protected]>
fixes #65829
Fix: Avoid redundant aria-hidden on elements with hidden attribute
This PR updates the elementShouldBeHidden function to skip applying aria-hidden="true" to elements that already have the hidden attribute, as they are already hidden from both visual and assistive technologies. This prevents unnecessary DOM manipulations and ensures cleaner, more efficient accessibility handling.
Changes:
Added a check for the hidden attribute in the function to avoid redundant aria-hidden application.