fix(NcPopover): remove invalid aria-describedby #5304
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShGKme
added
bug
ShGKme
requested review from
susnux,
JuliaKirschenheuter,
Pytal and
emoral435
ShGKme
commented
Feb 26, 2024
Comment on lines
+394
to
401
| /** | ||
| * Triggered after the tooltip was visually displayed. | ||
| * | ||
| * This is different from the 'show' and 'apply-show' which | ||
| * run earlier than this where there is no guarantee that the | ||
| * tooltip is already visible and in the DOM. | ||
| */ | ||
| this.$emit('after-show') |
There was a problem hiding this comment.
Moved the comment so it works as event documentation for styleguidist.
ShGKme
commented
Feb 26, 2024
Comment on lines
+297
to
+300
| const triggerElements = triggerContainer.querySelectorAll('[data-popper-shown]') | ||
| for (const el of triggerElements) { | ||
| el.removeAttribute('aria-describedby') | ||
| } |
There was a problem hiding this comment.
In Vue 2 triggerElements is always only one root element, but in Vue 3 there could be multiple nodes. Using querySelectorAll for Vue 3 compatibility.
ShGKme
force-pushed
the
fix/nc-popover--remove-aria-describedby
branch
from
afbea40
to
585a6a5
Compare
JuliaKirschenheuter
approved these changes
Feb 26, 2024
|
PR conflicts |
emoral435
approved these changes
Feb 26, 2024
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
ShGKme
force-pushed
the
fix/nc-popover--remove-aria-describedby
branch
from
585a6a5
to
f0628ca
Compare
|
/backport to next |
2 tasks
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves
When the popover is shown,
floating-vuemutates the root elements of the trigger addingdata-popper-shownandaria-describedbyattributes.aria-describedbyis not correct here. All the popover content should not describe the trigger button.See: https://github.com/Akryum/floating-vue/blob/8d4f7125aae0e3ea00ba4093d6d2001ab15058f1/packages/floating-vue/src/components/Popper.ts#L734
Unfortunately, because
floating-vuedirectly mutates the button via DOM API, it is not possible to remove the attribute in a Vue way without the same DOM mutation...🖼️ Screenshots
🏁 Checklist
nextrequested with a Vue 3 upgrade