components icon indicating copy to clipboard operation
components copied to clipboard

fix: Hotspot events inside a checkbox label unexpectedly toggle the checkbox

Open connorlanigan opened this issue 2 years ago • 2 comments

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

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.

connorlanigan avatar Aug 31 '22 11:08 connorlanigan

Codecov Report

Merging #225 (73f82a7) into main (af5b6ce) will decrease coverage by 0.00%. The diff coverage is 100.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.

codecov[bot] avatar Aug 31 '22 12:08 codecov[bot]

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.

visual_regression_result

The failing release workflow was a configuration issue and unrelated to this change.

connorlanigan avatar Sep 12 '22 14:09 connorlanigan