microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.3k stars 599 forks source link

feat: add delegation strategy to @attr mode #6358

Open yinonov opened 2 years ago

yinonov commented 2 years ago

๐Ÿ™‹ Feature Request

Add the ability to proxy attributes to an internal node and never reflect them on the DOM element.

This is useful for aria-* attributes or role which makes more sense to set on an internal element within a host's shadow rather than the host itself.

๐Ÿค” Expected Behavior

@attr mode can be leveraged and allow additional strategy like "never" (or "delegate" or "proxy").

๐Ÿ˜ฏ Current Behavior

@attr mode allow only the following "reflect" | "boolean" | "fromView"

This might be opinionated, but our code style approach is to try and avoid DOM elements from mutating at run-time and keep a consistent result to what authors write and expect, making custom elements behave like black-box services.

in the following FAST menu item, role assigned with a default value will mutate and apply a role on the host when authors aren't setting it.

https://github.com/microsoft/fast/blob/1d230aa72f4802bb309c4679bf5b2fa27d385e7b/packages/web-components/fast-foundation/src/menu-item/menu-item.ts#L115-L116

Now this might conflict with the fact that attribute set by author would be removed at run-time when @attr is set to 'never | proxy | delegate'. but it's mandatory to avoid screenreader conflicts.

๐Ÿ’ป Examples

Material team has written a decorator dedicated for this type of behavior and it is explained very well here

https://github.com/material-components/material-web/blob/5241b76dcabfe4a0d9a1010d90aaed65e3875c04/decorators/aria-property.ts#L9-L34

A property decorator that helps proxy an aria attribute to an internal node.

This decorator is only intended for use with ARIAMixin properties, such as ariaLabel, to help with screen readers.

This decorator will remove the host aria-* attribute at runtime and add it to a data-aria-* attribute to avoid screenreader conflicts between the host and internal node.

@ariaProperty decorated properties should sync with LitElement to the data-aria-* attribute, not the native aria-* attribute.

yinonov commented 2 years ago

Here's the problem in practice - setting aria-label on button will set it twice, on the host and the shadowed button

Screen Shot 2022-09-07 at 17 54 03

then it makes SR read it twice

Screen Shot 2022-09-07 at 17 57 52
EisenbergEffect commented 2 years ago

@yinonov Thanks for reporting this. We are aware of the issue. I believe we have a solution in the works. @nicholasrice was working on something related to attribute delegation a little while back as part of a performance investigation. @nicholasrice Do you think we could pick that work back up soon and add some capability for this scenario that would both help with perf and address the a11y issue?

EisenbergEffect commented 2 years ago

@nicholasrice Probably also good to investigate this as part of the ElementInternals work. Perhaps we just use the ElementInternals API and maybe the polyfill works in a way that doesn't create the above issue.

nicholasrice commented 2 years ago

That work actually went it but it works a little differently than the described desired behavior. I'm not actually sure how useful reflect-attributes is as-is but it works as @chrisdholt specced it, so maybe he has more insights? If it doesn't work for aria attr reflection in a useful way, maybe we adjust the behavior?

chrisdholt commented 2 years ago

Here's the problem in practice - setting aria-label on button will set it twice, on the host and the shadowed button

Screen Shot 2022-09-07 at 17 54 03

then it makes SR read it twice

Screen Shot 2022-09-07 at 17 57 52

Which AT is actually reading this twice? While it exists on the host in button, there is no role and focus is never brought to it. While the generics are showing up in the a11y tree, I donโ€™t know that this actually is presented twice. Can you share which AT reproโ€™s this for button?

yinonov commented 2 years ago

Mac's VoiceOver SR. @chrisdholt logically this makes sense but is what you're describing a formal written spec? cause the VoiceOver does read it twice, exactly as written in the box

chrisdholt commented 2 years ago

Mac's VoiceOver SR. @chrisdholt logically this makes sense but is what you're describing a formal written spec? cause the VoiceOver does read it twice, exactly as written in the box

~I just tried to repro this on Edge using VoiceOver and I get different results (not duplicated) though it does read "group". What browser combination are you using with this?~

Now getting consistent repros :). This is new behavior here from VoiceOver (wasn't previously the case). I think the reading out could be related to the fact that generics are exposed to a11y API's for the sake of layout and bounds. @eisenbergeffect since this now is presenting actual user impact we likely should come up with something temporary as we await the fate of cross-root ARIA delegation. The work @nicholasrice did initially doesn't ultimately solve the issue as you can't set via both the attribute and the property. I think we'd need something to support both. In the case of this problem, ultimately we need to avoid having the exact attributes reflect on both the host and the actual element within the Shadow DOM.

~One workaround for now to try might be to use display: contents on the host...I'm not sure that will work even though the intent would be to remove the host from the a11y tree as well. Even if it does, that ends up being an awfully prescriptive styling nuance for folks to implement.~ (Does not work as a workaround)

KingOfTac commented 2 years ago

Using NVDA, Windows Narrator, @microsoft/fast-components@2.30.6, and @fluentui/web-components@2.5.5 on Windows 11 I couldn't repro the issue using this markup:

<fast-button aria-label="label">click me</fast-button>
<fast-button aria-label="another label">click me</fast-button>

<fast-button role="none" aria-label="label">click me</fast-button>
<fast-button role="none" aria-label="another label">click me</fast-button>

<fast-button role="presentation" aria-label="label">click me</fast-button>
<fast-button role="presentation" aria-label="another label">click me</fast-button>

For every instance of Button in the example both the NVDA SR and Narrator SR read [aria-label] button only once. In the devtools accessibility tree I was seeing the reflected label attribute

generic "label"
    button "label"
KingOfTac commented 2 years ago

The only time I could create any sort of duplication was if I used the word button in the label, but you shouldn't really use the role in the label anyway.

chrisdholt commented 2 years ago

The only time I could create any sort of duplication was if I used the word button in the label, but you shouldn't really use the role in the label anyway.

Well, and in this case the duplication isn't the label being read twice it's the label and button both being the same.

Thanks for the above @Kingoftac - Looks like we don't yet have a repro beyond VoiceOver, which is new behavior. I'll continue to do some digging here. In the meantime, I do think that a temporary workaround for voiceover could be to use role="presentation" to remove the user impact of this today. We'll take a look at the feature request and potential long-term resolution in the meantime.

yinonov commented 2 years ago

@KingOfTac would you try only assigning a svg node? this is where aria-label as the svg by its own, doesn't make a discernible text.

<fast-button aria-label="label">
  <svg>
    ...
  </svg>
</fast-button>

that's really bizarre - I have just tried to reproduce it again and the duplicate reading no longer occur with the same code (still voiceOver) ๐Ÿค” flaky

image
chrisdholt commented 2 years ago

@KingOfTac would you try only assigning a svg node? this is where aria-label as the svg by its own, doesn't make a discernible text.

<fast-button aria-label="label">
  <svg>
    ...
  </svg>
</fast-button>

that's really bizarre - I have just tried to reproduce it again and the duplicate reading no longer occur with the same code (still voiceOver) ๐Ÿค” flaky

image

Looks like โ€˜role=โ€œnoneโ€โ€™ is on the host still, which probably explains it reading once ๐Ÿ˜„

yinonov commented 2 years ago

@KingOfTac would you try only assigning a svg node? this is where aria-label as the svg by its own, doesn't make a discernible text.

<fast-button aria-label="label">
  <svg>
    ...
  </svg>
</fast-button>

that's really bizarre - I have just tried to reproduce it again and the duplicate reading no longer occur with the same code (still voiceOver) ๐Ÿค” flaky

image

Looks like โ€˜role=โ€œnoneโ€โ€™ is on the host still, which probably explains it reading once ๐Ÿ˜„

oh ๐Ÿคฆโ€โ™‚๏ธ. there you have it. your workaround proven

KingOfTac commented 2 years ago

I guess it's worth noting I also did my testing with the "icon" button scenarios in the fast component explorer, with the final test in the "icon in default slot" scenario with the text removed so there was only an icon.

I did this testing after my initial comment as a quick gut check.

rajsite commented 2 years ago

We were rounding on the idea of setting role="none" on host elements for the same issue. Does that sound like the right way forward for these cases where the element that is the actual aria node is in the shadow root and not the host? @m-akinc (for https://github.com/ni/nimble/issues/689)