-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better user interface for screen readers and keyboard navigation #2946
Conversation
can you rebase this on latest develop so it doesn't include a load of unrelated changes? |
b2d1505
to
410e685
Compare
Requires matrix-org/matrix-react-sdk#616, ftr |
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.
a couple of nits it would be nice to fix
} | ||
|
||
} | ||
|
||
if (this.props.roomId) { | ||
buttonGroup = | ||
<div className="mx_RightPanel_headerButtonGroup"> | ||
<div className="mx_RightPanel_headerButton" title="Members" onClick={ this.onMemberListButtonClick }> | ||
<div className="mx_RightPanel_headerButton"> |
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.
are the divs still needed here?
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.
Didn't want to touch it just in case.
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.
Seems like it'd be fine if the class were to be moved to the button element.
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 got rid of the divs.
@@ -61,35 +81,42 @@ module.exports = React.createClass({ | |||
} | |||
}, | |||
|
|||
_clearSearch: function() { | |||
this.refs.search.value = ""; | |||
this.onChange(); |
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 should happen automatically?
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.
Doesn't.
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.
You're kinda right, for reasons that are ... non-obvious. In case you're interested, and for my own reference:
Essentially the onChange
callback is a React fabrication. Native-DOM <input>
elements have an onchange
attribute, but it is only called when the element loses focus.
React actually listens for the input
event, and calls the onChange
callback when it sees one. Apparently the DOM does not fire an input
event when the value
is changed from js - which is probably lucky otherwise React would have to jump through hoops to avoid getting stuck in a loop.
What we ought to do here is stick with the react side, and just make the change through react:
this.setState({ searchTerm: "" });
That will trigger a re-render, which will update the DOM.
See also: https://facebook.github.io/react/docs/forms.html#controlled-components.
@JaniM hang on - i'm confused. i thought this had already landed? or did we only merge the react-sdk half of it? |
Changed most button-like divs to actually behave as such (via the new AccessibleButton element). Also made filtering rooms more keyboard friendly.
Signed-off-by: Jani Mustonen [email protected]