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

[Autocomplete] Use Popper with when disablePortal #23263

Merged
merged 3 commits into from
Nov 12, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 26, 2020

By using the Popper we can use <Autocomplete disablePortal /> in a flex container and resolve the misleading naming issue i.e. disablePortal now only disables the portal instead of disabling the Popper.

disablePortal is important for a11y on VO

Other minor fixes:

  1. Avoid role in label
    The UI already communicates what kind of interactions are allowed. We don't need to repeat that in the label.
    It's especially annoying with screen readers which announced "combobox combobox"

Preview: https://deploy-preview-23263--material-ui.netlify.app/components/autocomplete/

Closes #23471

@eps1lon eps1lon added docs Improvements or additions to the documentation accessibility a11y labels Oct 26, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 26, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 9a5c7b8

@eps1lon eps1lon force-pushed the fix/Autocomplete/owner branch from e062b2b to 261854d Compare November 3, 2020 11:44
@eps1lon eps1lon changed the title [docs] Improve a11y for initial Autocomplete demo [Autocomplete] Use Popper with when disablePortal Nov 5, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

A couple of thoughts:

  • I wonder about the tradeoff of not using the default prop for the first demo. It sends mixed signals. It could be better to pick a side. Maybe we should add a new demo under iOS VoiceOver instead?
  • Otherwise, maybe we should document the downsides of disabling the portal? And maybe add a warning below the first demo about the usage of disablePortal that links to the downside, so developers know about it once they face it?

@eps1lon
Copy link
Member Author

eps1lon commented Nov 8, 2020

I wonder about the tradeoff of not using the default prop for the first demo. It sends mixed signals.

I'm just completely lost what or if you have a principled opinion on a11y. Should components pass A (AA, AAA) by default? Should demos pass A (AA, AAA)?

Your statements in this PR are not reconcilable with #22382 (comment).

Right now Autocomplete is not passing A by default in mobile VO. But you don't even want to document a passable version on the demo page.

Otherwise, maybe we should document the downsides of disabling the portal? And maybe add a warning below the first demo about the usage of disablePortal that links to the downside, so developers know about it once they face it?

We should do that regardless of this PR. As far as I know we don't explain anywhere why you should/would want to use disablePortal and what downsides it has.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 8, 2020

I'm just completely lost what or if you have a principled opinion on a11y. Should components pass A (AA, AAA) by default? Should demos pass A (AA, AAA)?

This is an interesting topic, I have never looked into yet. How do the levels depend on the supported screen readers? Is it based on the "most popular" ones (where is the threshold?), on what's the intrinsic implementation delivers regardless of the bugs screen readers could have (specification based), or else?

We should do that regardless of this PR.

👍

we don't explain anywhere why you should/would want to use disablePortal and what downsides it has.

We only have: https://material-ui.com/components/autocomplete/#ios-voiceover

@eps1lon
Copy link
Member Author

eps1lon commented Nov 9, 2020

This is an interesting topic

We already discussed this. You changed your opinion now. Or maybe you never had one and I assumed that the things you wrote in #22382 (comment) were your opinion? Could you give me your opinion?

How do the levels depend on the supported screen readers?

Just like any other feature? A level can only ever makes sense with a device. You can't say that a UI passes level X without specifying on what device you verified that. For example, it doesn't make any sense to say that your UI is keyboard accessible if you didn't test it with a keyboard device.

I assumed it was obvious that a feature should work with the most popular device. So I'm making it explicit now: If we support mobile and we want AA by default then our components have to pass VO on iOS since that is by far the most popular screen reader on mobile (source). Otherwise it would be like saying that our components are interactive unless used with google chrome.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 9, 2020

We already discussed this.

What I can see in #22382 (comment) is about the need to reach a specific level. However, it doesn't cover what screen readers or which versions of a screereader is supported.

A level can only ever makes sense with a device

This is the discussion I want to have :). As #14187, maybe we should document the screen readers and the versions we support?

If we take Kendo-UI as an example. They don't seem to mention any specific screen readers supported. However, they mention the limitations: of the combobox but it doesn't stop them to label it AA. Does it work with iOS VoiceOver?

@eps1lon
Copy link
Member Author

eps1lon commented Nov 10, 2020

However, it doesn't cover what screen readers or which versions of a screereader is supported.

Sure. I implicitly assumed this at least includes the most popular screen reader. I assumed that it would make no sense if we decide to reach a level on a rarely used screen reader. Just like I wouldn't expect that a feature discussion targets IE 10 but rather Chrome.

It seems like you want to change the approach with screen readers and not go by popularity. What would you like to do and why not use the same approach we take with browsers? That would be the consistent variant which is what you prefer oftentimes.

If we take Kendo-UI as an example. They don't seem to mention any specific screen readers supported.

What is the normative statement here? You're making a bunch of descriptive statements. These cannot tell us what we should do. Unless we are just copy-cats that copy code from other repositories.

@oliviertassinari
Copy link
Member

I implicitly assumed this at least includes the most popular screen reader.

Thanks for sharing the survey on the most popular screen readers. This definitely makes sense. Would it be worth documenting each screen reader and version (e.g only latest) under platforms? This way, there is no room for interpretation?

I agree that it wouldn't make sense for Material-UI to say, that we support version x of the HTML & version y of the JavaScript specification, go figure if the browser you want to use support it. It has to be more end-to-end driven. So in the realm of accessibility, we have to include which screen readers we support specifically and which version. The screenreaders should follow the spec, but there are always edge cases & workaround to consider.

If we take Kendo-UI as an example.

I wasn't able to draw any conclusion from what Kendo-UI is doing. I was wondering if they were describing the Combobox as reaching AA even though there are some important screenreaders that are no supporting, which would give us a precedent to justify our Combobox as AAA even though iOS needs an opt-in prop.

@eps1lon
Copy link
Member Author

eps1lon commented Nov 12, 2020

Would it be worth documenting each screen reader and version (e.g only latest) under platforms?

Definitely. Though this falls generally under "a11y statement" which I'd like to get in at some point. It's on my TODO but I haven't gotten around to it.

@eps1lon eps1lon merged commit c1b3e33 into mui:next Nov 12, 2020
@eps1lon eps1lon deleted the fix/Autocomplete/owner branch November 12, 2020 11:52
@gnowland
Copy link
Contributor

Otherwise, maybe we should document the downsides of disabling the portal? And maybe add a warning below the first demo about the usage of disablePortal that links to the downside, so developers know about it once they face it?

We should do that regardless of this PR.

👍

we don't explain anywhere why you should/would want to use disablePortal and what downsides it has.

We only have: https://material-ui.com/components/autocomplete/#ios-voiceover

I can't find any information about under what circumstances one would or would not want to disable the portal, and downsides using disablePortal has. Specifically looking at the Autocomplete component.

It seems like you two understand it - would one of you @oliviertassinari, @eps1lon please point me towards some resources? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocomplete disablePortal breaks Popper behavior
4 participants