components
components copied to clipboard
fix: Hotspot events inside a checkbox label unexpectedly toggle the checkbox
Description
The issue:
when a Hotspot is placed inside a checkbox label (or the label of a Toggle, Radio Button or Tile) and the user clicks on an element inside the Hotspot's Annotation popover, this click event also bubbles up to the checkbox's <label>
element. This then triggers a duplicated/redirected event on the native <input>
element.
While the bubbling of the first click event can be prevented with e.stopPropagation
, this does not prevent the dispatching of the second event. According to this StackOverflow answer, this is known "legacy-pre-activation behavior" and cannot be prevented since it is part of the WHATWG specification - the label->input event duplication/redirection happens before the actual event dispatching.
The change:
This change deactivates the legacy behaviour by replacing the <label>
element with a <div>
. To match the existing screenreader behaviour, it uses ARIA properties to connect the label to the checkbox. Event listeners are added to match the existing behaviour (but now the bubbling can be stopped with e.stopPropagation()
).
The Hotspot component now has an additional event handler that prevents events from bubbling into the surrounding component.
How has this been tested?
[How did you test to verify your changes?]
- Added a dev page where I performed manual testing
- Updated unit tests
[How can reviewers test these changes efficiently?]
- View the dev page locally and test its announcements and interactions with a screenreader
[Check for unexpected visual regressions, see CONTRIBUTING.md
for details.]
Documentation changes
[Do the changes include any API documentation changes?]
- [ ] Yes, this change contains documentation changes.
- [X] No.
Related Links
- AWSUI-19101
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
- [X] Changes are backward-compatible if not indicated, see
CONTRIBUTING.md
. - [X] Changes do not include unsupported browser features, see
CONTRIBUTING.md
. - [X] Changes were manually tested for accessibility, see accessibility guidelines.
Security
- [ ] If the code handles URLs: all URLs are validated through the
checkSafeUrl
function.
Testing
- [X] Changes are covered with new/existing unit tests?
- [ ] Changes are covered with new/existing integration tests?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Codecov Report
Merging #225 (73f82a7) into main (af5b6ce) will decrease coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #225 +/- ##
==========================================
- Coverage 92.66% 92.66% -0.01%
==========================================
Files 559 559
Lines 15764 15762 -2
Branches 4316 4309 -7
==========================================
- Hits 14608 14606 -2
Misses 1075 1075
Partials 81 81
Impacted Files | Coverage Δ | |
---|---|---|
...notation-context/annotation/annotation-popover.tsx | 100.00% <100.00%> (ø) |
|
...notation-context/annotation/annotation-trigger.tsx | 100.00% <100.00%> (ø) |
|
src/checkbox/internal.tsx | 100.00% <100.00%> (ø) |
|
src/internal/components/abstract-switch/index.tsx | 100.00% <100.00%> (ø) |
|
src/radio-group/radio-button.tsx | 100.00% <100.00%> (ø) |
|
src/table/selection-control/index.tsx | 100.00% <100.00%> (ø) |
|
src/tiles/internal.tsx | 100.00% <100.00%> (ø) |
|
src/toggle/internal.tsx | 100.00% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
The failing visual regression check is expected. There was a missing label in one of the tiles that was not caught by the automated tests, but was now detected after the change, and I've added the label now.
The failing release workflow was a configuration issue and unrelated to this change.