Educator 1552 Implement roving tabindex for WYSIWYG editor for discussions - #16705
Conversation
There was a problem hiding this comment.
Not sure why this entire file says I made changes (but there is no difference in the before/after...), but this is the relevant part. The first button should be initialized with a tabindex of 0, and all subsequent buttons should have an initial value of -1.
672ec6b to
de85c03
Compare
ssemenova
changed the title
f74e098 to
a33a806
Compare
|
jenkins run python |
26397e0 to
48a0f6a
Compare
|
FYI: I reverted my last few commits, so a few quality tests will fail on my code. I'll fix those in a second, and raise the limit for the ones in Markdown.Editor.js, until we can address it in ED-1920. |
schenedx
left a comment
schenedx
left a comment
There was a problem hiding this comment.
Had a few NITs.
Looks good to me in general.
| var $currentButton, $nextButton, $toolbar, $allButtons, | ||
| keyPressed, nextIndex, currentButtonIndex; | ||
|
|
||
| var leftKey = this.LEFT_KEYCODE; |
There was a problem hiding this comment.
Why do we want to assign those constants to local variables? Why not just use them in the statements below?
There was a problem hiding this comment.
I just thought it made the code look a little more readable this way
|
|
||
| $currentButton = $(event.target); | ||
| keyPressed = event.which || event.keyCode; | ||
| $toolbar = $(event.target).parent(); |
There was a problem hiding this comment.
Isn't this just $currentButton.parent()?
| $currentButton = $(event.target); | ||
| keyPressed = event.which || event.keyCode; | ||
| $toolbar = $(event.target).parent(); | ||
| $allButtons = $toolbar.children('.wmd-button'); |
There was a problem hiding this comment.
What if the variable $allButtons does not contain any array items? What if you got no buttons?
Not very likely, but we should check for this condition to avoid surprises
There was a problem hiding this comment.
I can add the check just to make sure
c255d28 to
c8f6c2c
Compare
66bce8c to
4747483
Compare
|
jenkins run all |
4747483 to
a1f0679
Compare
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, December 11, 2017. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
3 participants
Educator 1552
Sandbox
The toolbar for the WYSIWYG editor in "Add new post" (and also the general post view, but the a11y audit didn't specify that page) is navigable by tabbing through it, but does not perform how a toolbar is expected to perform. See acceptable a11y example here. The expected behavior is:
Just in case someone looks at this PR in the future, the name for this behavior is a roving tabindex.
Drawing very very very heavily from the javascript code from the w3 example.