Skip to content

Conversation

@huyenltnguyen
Copy link
Member

src/css/App.css Outdated
background: #2a2a40;
}

.toggleButton {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this?

Copy link
Member

@ahmaxed ahmaxed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.
I noticed an area above the button is being highlighted
image

@huyenltnguyen
Copy link
Member Author

huyenltnguyen commented Apr 6, 2020

Thanks for the review, Ahmad!

I just found an issue with using keypress on this button, in which the global hotkey doesn't honor the stopPropagation set to the button, and captures the space keypress anyway. This causes some issue with the player's play event.

I'm marking the PR WIP for now and will find time to resolve the issue (it would require a dive into the react-hotkeys doc).

@huyenltnguyen huyenltnguyen changed the title fix(a11y): replace div with button element [WIP] fix(a11y): replace div with button element Apr 6, 2020
@huyenltnguyen huyenltnguyen force-pushed the fix/play-pause-button-a11y branch from 4ad42c8 to ea9fa95 Compare April 6, 2020 19:11
@huyenltnguyen huyenltnguyen force-pushed the fix/play-pause-button-a11y branch 3 times, most recently from fab2f14 to 7a3244f Compare May 2, 2020 06:45
@huyenltnguyen huyenltnguyen changed the title [WIP] fix(a11y): replace div with button element fix(a11y): replace div with button element May 2, 2020
@huyenltnguyen huyenltnguyen changed the title fix(a11y): replace div with button element fix(a11y): make play/pause button more accessible May 2, 2020
@huyenltnguyen
Copy link
Member Author

Hi @ahmadabdolsaheb, this PR is ready for another look now.

I initially replaced the div with a button element. However. a special thing about the button is, its click event can be triggered by Space and Enter keys by default. For the Space keypress, it unfortunately collides with the GlobalHotKeys set in App.js and causes the togglePlay function to run twice. This means if the audio is playing, we toggle it to pause, and quickly toggle it back to play, which is not what we want.

We can easily fix this by calling event.preventDefault() inside handleKeyDown(). This approach does work on Chrome (and probably other browsers), but doesn't work on Firefox due to an existing bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1552419

My workaround is to just keep the component as a div, so that by default its click event isn't triggered by the Space key, and then manually handle the keyDown event. This approach ensures that keyboard navigation and shortcuts are working across browsers.

@huyenltnguyen huyenltnguyen force-pushed the fix/play-pause-button-a11y branch from 7a3244f to 42623ce Compare May 2, 2020 07:10
@ahmaxed ahmaxed merged commit 2ab6653 into freeCodeCamp:master May 6, 2020
@huyenltnguyen huyenltnguyen deleted the fix/play-pause-button-a11y branch May 6, 2020 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[a11y] play button not recognized as button

2 participants