search

angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.37k stars 6.75k forks source link

bug(a11y): Focus styles not applying in Shadow DOM cont. #19760

Open jacobweber opened 4 years ago

jacobweber commented 4 years ago

Still seeing some issues related to #19414 after the 10.0.0 upgrade.

Reproduction

https://stackblitz.com/edit/angular-e7d8vq-nityhk

Steps to reproduce:

  1. Include some mat-buttons inside a container component.
  2. Make the container component display them with ng-content.
  3. Add an *ngIf to the ng-content, and delay the condition becoming true.
  4. Click one of the buttons and press tab. The focus styles won't be applied.

Expected Behavior

Focus styles applied to the next button

Actual Behavior

Focus styles not applied. Can be fixed by removing the delay or using ViewEncapsulation.None instead of ViewEncapsulation.ShadowDom.

Environment

crisbeto commented 4 years ago

It looks like we keep kicking the can down the road with this bug... I spent some time looking at the latest repro and it seems like the only way to resolve it would be to wrap all of the monitor calls in a setTimeout, but that won't be practical, because it'll break people's tests. We could also keep polling the DOM in ngAfterViewChecked, but that'll have other performance implications.

I think that at this point we should reconsider whether to revert the event delegation that was added by https://github.com/angular/components/commit/a85b63c19e092c32803d3ceea92cea725a9b6bd2.

crisbeto commented 4 years ago

We talked about this a couple of days ago and we agreed that we should support all cases inside the shadow DOM while keeping the performance gains from the event delegation. Since we can't defer binding the events inside a setTimeout, because it'll break people's tests, we can try to defer it until the next time the user interacts with the page (e.g. next click, mousemove or touchstart event).

jacobweber commented 4 years ago

Or a keyboard event, I assume, since the problem can start when you hit Tab. Anyway, I'll try to test my app against whatever you come up with, before you release it.

crisbeto commented 4 years ago

I think doing it on a keyboard event would be problematic, because focus would've already moved so we might miss some events. We'd probably just have to test out a few things and see what works best.

crisbeto commented 4 years ago

Sorry for the delay, we actually talked about this a while ago. I made a PR that works around the issue, but it ended up breaking a lot of people's tests. I don't think there is any other way around it so we might have to add an opt-in configuration option that you can turn on if you're using the shadow DOM.