-
-
Notifications
You must be signed in to change notification settings - Fork 827
A11y: repurpose more divs into AccessibleButtons. #2189
Conversation
With this more of the controls that look like buttons can be operated via the keyboard and navigated to by screen reader users. This includes editor buttons such as File upload, Audio / Video call, Right pannel hide button, Jump to the bottom timeline button, and some more buttons found in the user settings. Also I have added alt texts to some images that in turn label buttons which these happen to be packed in and removed some untranslated alt texts from decorative non-actionable images that might add more verbosity when talking about screen reader user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also:
- Lint errors
- DCO signoff
otherwise this looks good, thanks for catching all these
@@ -1233,10 +1233,10 @@ module.exports = React.createClass({ | |||
value={this.presentableTextForThreepid(val)} disabled | |||
/> | |||
</div> | |||
<div className="mx_UserSettings_threepidButton mx_filterFlipColor"> | |||
<AccessibleButton className="mx_UserSettings_threepidButton mx_filterFlipColor"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this one will work since the onClick
is on the img
rather than the div
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for alerting me to this. It turns out I have added more issues like this. I can fix almost all of them with upload avatar button being more tricky. So I'm leaving that out for today.
@@ -96,7 +96,7 @@ export default React.createClass({ | |||
} | |||
return ( | |||
<div className="mx_MatrixToolbar"> | |||
<img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="Warning"/> | |||
<img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I really see why this is being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text it-self is descriptive enough to be understood as warning. Thus I consider having set something other than an empty string to the alt attribute on this image an over verbosity for screen reader users.
@@ -35,11 +35,11 @@ module.exports = React.createClass({ | |||
render: function() { | |||
return ( | |||
<div className="mx_MatrixToolbar"> | |||
<img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="Warning"/> | |||
<img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I don't really get either
@@ -51,7 +51,7 @@ export default class CookieBar extends React.Component { | |||
const toolbarClasses = "mx_MatrixToolbar"; | |||
return ( | |||
<div className={toolbarClasses}> | |||
<img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="Warning" /> | |||
<img className="mx_MatrixToolbar_warning" src="img/warning.svg" width="24" height="23" alt="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - why remove the alt text?
@@ -6,7 +6,7 @@ const AppWarning = (props) => { | |||
return ( | |||
<div className='mx_AppPermissionWarning'> | |||
<div className='mx_AppPermissionWarningImage'> | |||
<img src='img/warning.svg' alt={_t('Warning!')} /> | |||
<img src='img/warning.svg' alt='' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this one
@@ -34,7 +34,7 @@ export default React.createClass({ | |||
src="img/warning.svg" | |||
width="24" | |||
height="23" | |||
alt="Warning" | |||
alt="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one too
} else { | ||
image = <img className="mx_MatrixToolbar_warning" src="img/spinner.gif" width="24" height="23" alt={message}/>; | ||
image = <img className="mx_MatrixToolbar_warning" src="img/spinner.gif" width="24" height="23" alt="" />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more remove alt texts
I think all is solved now except DCO signoff. For that to happen shal I create a new PR or how would I push signed commits replacing those unsigned? Just for the record I have fixed lint errors and addressed most of the review comments. Regarding removing some of the alt text attributes I think decorative images don't have to include these. |
With this more of the
controls that look like buttons can be operated via the keyboard and
navigated to by screen reader users. This includes editor buttons such
as File upload, Audio / Video call, Right pannel hide button, Jump to
the bottom timeline button, and some more buttons found in the user
settings.
Also I have added alt texts to some images that in turn label buttons
which these happen to be packed in and removed some untranslated alt
texts from decorative non-actionable images that might add more
verbosity when talking about screen reader user experience.