Skip to content
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

Added accessibility features to mwc-textfield #1852

Closed
wants to merge 5 commits into from
Closed

Added accessibility features to mwc-textfield #1852

wants to merge 5 commits into from

Conversation

christophe-g
Copy link
Contributor

As a follow-up of #1215.

The purpose of this PR is to make mwc-textfield more accessible, basically following https://github.com/material-components/material-components-web/tree/master/packages/mdc-textfield/helper-text#accessibility.

  • b020c1f makes test suite work on my machine. Test engine would not start without.

  • c3c45c1 is added to make use of chai a11y aXe testing lib (https://open-wc.org/testing/testing-chai-a11y-axe.html). The way it is embedded now is pretty ugly. Open to any better suggestions on how to hook this to the current testing library (I am not a all used to typescript, so bare with me)

  • main work done under 0693e31. Tell me what needs to change before I commit similar work to textarea.

@christophe-g
Copy link
Contributor Author

Test failing on 0693e31 certainly because I used pnpm and not npm locally. I wish not to change this.

@asyncLiz
Copy link
Collaborator

asyncLiz commented Oct 5, 2020

Thanks for the PR, and sorry for the delay on commenting!

We discussed this at our triage meeting last week, and our consensus is to hold off on implementing #1215 right now for a few reasons that are not immediately obvious:

  1. Text field a11y must meet strict requirements internally and sync up to its internal implementation. While it may be possible to go back and forth on this PR right now to get it synced up, there are several outstanding bugs for the internal text field's a11y (some of which are being closed in a fixit this week), so it would quickly become out of sync. We'd like to find a better approach for keeping them in sync first or wait until the a11y story is more stable.
  2. Design has stated that non-decorative actionable icons should display a ripple, since they are their own touch targets. Rather than adding more complex logic, we wish to implement this in MWC (and upstream in MDC) by using icon buttons. This would eliminate many of the requirements in the text field icon foundation and Add missing icon/helper-text foundation features to textfield #1215 around icon a11y since that is built in to icon buttons already.
  3. Changes to our build and test system are very difficult to bring in internally, and would require much more refactoring on this PR and effort that we don't think is beneficial right now for the short term given our current priorities.
  4. The text field container element (which holds the icons) is expecting a refactor this quarter, which would make much of the above high effort work lost or redone.

We really appreciate the contribution, but given these reasons and our need to carefully upcoming work, we'd like to wait before addressing this outstanding bug.

@christophe-g
Copy link
Contributor Author

Thanks a lot for the detailed answer, truly appreciated !

Beyond the specifics of this PR, I think it illustrates a series of concerns I was not sure where/how to state. So, I'll do it here, not without thanking the devs who are putting tremendous amount of quality in the code base.

  1. It is difficult for me (us ?) to know where the library is heading and what is happening behind the scene. I am de-facto left with proxy indicators at hands (like pace of development, type of PR being merged, milestones being pushed back) to guess what is happening. For the time being, they leave me intrigued.

Would it make sense to post some type of high-level outline of medium term vision / problems encountered / area of work / what to expect in next releases ... It would be interesting to know a bit more about thinking behind theming options, envisioned long-term relationship with MDC foundation... It would also be kind of reassuring. For instance, discussion in #1121 was helpful.

  1. I understand the rationale behind "This repo is for extending the base layer with Web Components, and is not intended to be a complete replacement of MDC" (material-components-web vs material-components-web-components #223). I hope this is not on the expenses of loosing lit-element clarity and expressiveness and investing a large portion of the efforts just keeping in sync with upstream.

  2. This is partially a consequence of 2 and potentially a costly one: it's hard to contribute! Except for very minor changes, it is often not clear where to intervene as the dependencies/multi-plexing with the base layer forces to digest multiple layers of overhead.

I came to select this library because of the pleasant experience playing with / extending / contributing to other lit-based components (also because of very good experience with using Polymer elements). It wish it could be more of the same here.

As a consequence I end up, like in the old times, with a series of fragile override for doing things the library does not yet care of.

I will now be closing this PR, as - again - reasons for not incorporating it makes sense, and open a couple follow-up issues (#1873, #1874).

@@ -387,6 +392,10 @@ export abstract class TextFieldBase extends FormElement {
return html`
<input
aria-labelledby="label"
aria-controls="helper"
aria-describedby="helper"
aria-errormessage="helper"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allan-chen - aria-errormessage and aria-invalid, ref for #1872

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks - I believe someone else is working on aria-invalid for textfield so I'll leave it out of scope of my PR for now.

CC/ @brandondiamond - for your consideration we should add incorporate MWC into the aria-invalid enhancements, should be little overhead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants