-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(material-experimental/mdc-list): implement selection list #20279
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
Conversation
<ng-content></ng-content> | ||
|
||
<ng-template #checkbox> | ||
<div class="mdc-checkbox" [class.mdc-checkbox--disabled]="disabled"> |
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 is something I'm still a bit unsure about. The initial attempt of selection list seemed to use our pseudo checkbox, but it seems like we could just use the markup from the MDC checkbox to rely on MDC as much as possible?
My only worry is that that we'd need to wrap the MDC checkbox styles in the list-option styles. Any thoughts on this?
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.
We might as well go with this more now and separately push the MDC team for some equivalent to pseudo-checkbox. My concern is that the full checkbox is too heavy for use in lists and that it may be problematic for accessibility since a selection list should behave as a listbox and not like a list of checkboxes.
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 don't see what's stopping us from using the pseudo checkbox here. It looks identical to the MDC one, aside from the animations.
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.
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.
We might as well go with this more now and separately push the MDC team for some equivalent to pseudo-checkbox.
That's something they already partially already have (which is where I was going). Their components always consist of initial static HTML markup and a later "upgraded" variant (instantiated through the component/foundation).
We only use the markup here, as we do not rely on any of larger runtime code here. This seems almost equivalent to an pseudo checkbox, or sufficient to what we need here. The only difference is that the checkbox is still driven by an native input. This should not be an issue though as we do not include it in the tab order.
problematic for accessibility since a selection list should behave as a listbox and not like a list of checkboxes.
It still can do that; as said above, we purely use it decoratively through the static markup. Also as a side-fact: MDC also
has examples of selection list w/ checkboxes but they use role=group
and role=checkbox
for items. I've created an issue upstream for that as that doesn't work well with screen readers.
My concern is that the full checkbox is too heavy
Yeah, that is the primary concern I had too. This is something though that ultimately should not be a problem if we have fine-grained Sass for these components. That way we could just pull the base styles. I assume it will be still larger than our existing pseudo checkbox, but the benefit is that we share as much code as possible with MDC.
@crisbeto For sure we could use the existing pseudo checkbox. Though the point seems to rely as much as possible on MDC. We could always rely on existing non-MDC code. I don't think anything except for the current potential size overhead (that we might be able to boil down with fine-grained Sass features as per my proposal) is stopping us from using the actual MDC checkbox.
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 number of DOM and elements and SVG content is a concern of mine, but I'm fine with leaving this as-is for now and putting it on our backlog with MDC.
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.
It doesn't seem to expensive at first glance, compared to the other case where another new component is matched and instantiated all the time (i.e. the pseudo checkbox component). I don't feel very strong about it, and also I'm not 100% convinced of either approaches (hence this discussion). Agreed that we could look into this later as part of the backlog.
3065d87
to
c6cfed2
Compare
Implements the MDC-based selection list. Accessibility and various other concepts/features of the MDC list have been audited. Issues have been reported upstream and workarounds where possible and reasonable have been applied w/ proper comments. https://github.com/material-components/material-components-web/issues?q=is%3Aissue+author%3Adevversion+mdc-list+. Also various other remaining takss for the MDC-based list have been completed/resolved, except for implementing the dense attribute. It's unclear yet how that can work out.
c6cfed2
to
3a0e6fa
Compare
<ng-content></ng-content> | ||
|
||
<ng-template #checkbox> | ||
<div class="mdc-checkbox" [class.mdc-checkbox--disabled]="disabled"> |
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.
We might as well go with this more now and separately push the MDC team for some equivalent to pseudo-checkbox. My concern is that the full checkbox is too heavy for use in lists and that it may be problematic for accessibility since a selection list should behave as a listbox and not like a list of checkboxes.
e27dc70
to
52cd81a
Compare
Address feedback. Cleanup code and circular dependency.
52cd81a
to
721d0b3
Compare
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.
LGTM
@@ -23,4 +23,11 @@ import {MatInteractiveListBase, MatListBase} from './list-base'; | |||
{provide: MatListBase, useExisting: MatActionList}, | |||
] | |||
}) | |||
export class MatActionList extends MatInteractiveListBase {} | |||
export class MatActionList extends MatListBase { | |||
// An navigation list is considered interactive, but does not extend the interactive list |
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.
Is this comment right? it mentions a navigation list.
Also, I had discussed with Jeremy and intentionally chose to move over the the MDC style key handling, was there some discussion for why this was changed back?
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.
@mmalerba Oh, good catch. This is a mistake that happend when I addressed feedback.
I was not aware of this discussion and I assumed it was done by accident that way. The reason I changed it over is that it seemed more accessible and matching with the existing list implementation to have every item tabbable.
The way it was set up before, it didn't have any list-related aria-role, and screenreaders weren't even suggesting that there are more items that can be iterated through. Hence for compatibility and better a11y I didn't make it a full interactive list. If you think we should still do that; then that is as easy as extending from the base class (I kept that).
We should then also consider adding a role to the nav and action list, right? Though that is contradictory to what we mention in the list overview. i.e. that we do not apply any special accessibility treatment to lists (except selection-list). Let me know what you prefer here. Happy to make the change.
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 hadn't gotten to polishing the accessibility yet, but I did test the MDC demos with chromevox at least and it seemed to work well. I think if you have a list with many items having the list be a single tab stop is definitely the way to go. Otherwise the user might have to press tab quite a few times to escape the list.
I think role=nav
makes sense for the navlist at least, not actionlist though. Unless @jelbourn had a specific reason for not doing that? I can't remember, but like I said, I didn't finish polishing the a11y, so I probably just didn't get to adding it
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.
Yeah, I'd agree with the point you made for lists with lots of items. I'm unsure if role=nav
would be suitable for the navigation list as it doesn't suggest screenreader users that they are within a list. Hence it's not clear to them that there are actually more items they could iterate through, if they'd use the arrow keys. This is something that is well-defined for a listbox but not for a nav
as it seems. That is the primary reason why I went with keeping individual items tabbable. That seems to match best with the accessibility experience one would get when using a standard <nav>
element with a list of anchors.
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 also tested with ChomeVox and VoiceOver. Also tested the "recommended" pattern of a checkbox selection list in MDC to see if it's suitable for the selection-list. This "recommendation" seemed to have a serious accessibility issue though, that has the same root cause as if we'd go with role="nav"
for nav-list. I sent an issue for the MDC team about this: Issue for MDC.
Yeah, would love some feedback from @jelbourn on this too. I'm confident that the current accessibility treatment (i.e. none for lists) is most suitable and would work best with the migration (matching the non-MDC one and fulfilling what we explain in our list overview). We only apply special accessibility treatment to the selection-list currently.
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.
@jelbourn Yeah, that sounds reasonable to me. The key part is making the navigation-list
use a list/menu/tree role so that the keyboard interaction and context is clear to screenreader users. I'm happy to make that change, but want to say again that this could be an incompatibility change with the non-MDC navigation list (though that could be acceptable; are we good on it from your perspective?)
Also, what are we doing for the action-list? should we do the changes here are part of this PR?
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.
We should do one of two things:
- If MDC implements a different pattern by default for nav or for action list, we should use that
- They don't don't (or just don't have anything), we should keep things how we had them before and track something for changing them at some point in the future.
Basically we should delegate to MDC if it's better than what we have now, but otherwise not change anything.
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.
MDC doesn't seem to be opinionated about this. There are no concrete examples in the readme on how they'd use the list with anchors or buttons. On their example page I found a list using anchors. That one uses a list w/ arrow keys, but wrapped it inside a <nav>
element. That's not great as it doesn't suggest that the keyboard shortcuts work, or that there are more items.
I also found another case of a navigation list in their docs app (not an actual example). They just had individual anchor list-items wrapped in an ul
. TL;DR: They don't seem opinionated and don't have a great solution to this; their example is lacking good accessibility as it seems.
The PR currently implements your (2) bullet point, so I think we should be good for now. I can create a JIRA for revisiting a11y for nav and action lists (i.e. due to the scenario mentioned with a lot of list items being cumbersome)
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.
sgtm
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.
@devversion and I discussed a bit offline. I think there were a few cases I didn't consider before. I agree that its best to go with this approach for now
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.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Implements the MDC-based selection list. Accessibility and various
other concepts/features of the MDC list have been audited. Issues
have been reported upstream and workarounds where possible and
reasonable have been applied w/ proper comments.
https://github.com/material-components/material-components-web/issues?q=is%3Aissue+author%3Adevversion+mdc-list+.
Also various other remaining tasks for the MDC-based list have
been completed/resolved, except for implementing the dense attribute.
It's unclear yet how that can work out.