[JW-11320]<Teaching Channel 9: No confirmation message is provided it on selecting captions styles 'Reset' button> - #3964
Conversation
| constructor(_content, _action, _template = itemButtonTemplate) { | ||
| super(_content, _action, _template); | ||
| this.statusEl = document.createElement('span'); | ||
| this.statusEl.id = 'reset-status-message'; |
There was a problem hiding this comment.
jw prefix, use classes rather than ids in case of multiple players per page (id is unique in semantically correct HTML).
| super(_content, _action, _template); | ||
| this.statusEl = document.createElement('span'); | ||
| this.statusEl.id = 'reset-status-message'; | ||
| this.statusEl.style = 'font-size: 0;'; |
There was a problem hiding this comment.
Avoid using CSS in JS whenever possible, as it can't be overwritten by CSS without !important suffixing, which would prevent user customization.
| this.statusEl.style = 'font-size: 0;'; | ||
| this.statusEl.setAttribute('role', 'status'); | ||
| this.el.appendChild(this.statusEl); | ||
| this.el.setAttribute('aria-controls', 'reset-status-message'); |
There was a problem hiding this comment.
This isn't what aria-controls is for, see examples section here: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls
|
Might want to re-write as an aria-live region that gets updated by anything that needs this sort of alert. See here: https://cccaccessibility.org/web-1/web-developer-tutorials/using-aria-live |
8ea017f to
9a3da8d
Compare
|
test this please |
| } | ||
| } | ||
|
|
||
| export class ResetMenuItem extends MenuItem { |
There was a problem hiding this comment.
Should this be in its own file? Something like reset-menu-item?
| captionsSettingsItems.push(resetItem); | ||
| captionsSettingsMenu.setMenuItems(captionsSettingsItems); | ||
| if (isReset) { | ||
| resetItem.statusEl.innerHTML = 'Reset Captions Successful'; |
There was a problem hiding this comment.
Might be better to use textContent here and below rather than innerHTML
jwbrandon
left a comment
jwbrandon
left a comment
There was a problem hiding this comment.
This could use a unit test, at least for the ResetMenuItem class. It does seem like it might be very difficult to unit test the actual fix here, but it might still be worth doing.
4 participants
This PR will...
Add confirmation message "Reset Captions Successful" when the reset button in the captions menu is pressed
Why is this Pull Request needed?
If the screen reader does not notify anything after selecting "Reset" button non-sighted users will not be able to confirm or notice that the information has been reset
Are there any points in the code the reviewer needs to double check?
Nope
Are there any Pull Requests open in other repos which need to be merged with this?
Nope
Addresses Issue(s):
https://jwplayer.atlassian.net/browse/JW8-11320
Checklist