microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
964 stars 309 forks source link

[BUG] Accessibility - Person focus #1909

Closed Rafaelki closed 10 months ago

Rafaelki commented 2 years ago

Describe the bug The react "Person" component, doesn't get the focus.

To Reproduce Steps to reproduce the behavior:

  1. Add the following code:
    <input type="text" />
      <Person
        personQuery="me"
        view={PersonViewType.twolines}
        personCardInteraction={PersonCardInteraction.click}
      />
  2. Put the focus in the input and then press tab in the keyboard. Check the focus skips the Person component.

I can see that it works in Storybook, but it doesn't work with the react component.

Expected behavior I would expect the same behaviour than in Storybook, i.e. it gets the focus, and by pressing enter the card opens. image

Environment (please complete the following information):

ghost commented 2 years ago

Hello Rafaelki, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

musale commented 2 years ago

Hello @Rafaelki thank you for bringing this up. I would like a little more clarity on how the component is composed. I have this reproduction that seems to work as expected with the snippet you have provided. Do you mind sharing more details?

https://user-images.githubusercontent.com/8081536/197520865-2f93101a-7a3f-44d2-a67e-af4758936fe7.mp4

ghost commented 2 years ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

Rafaelki commented 2 years ago

Hi @musale, thanks for looking into this, and sorry for the late response.

I am working in a SharePoint environment. I have the MGT SPFx library in the app catalog.

image

This is my code:

import * as React from 'react';
import { Person } from '@microsoft/mgt-react/dist/es6/spfx';

const TestPerson: React.FunctionComponent<any> = () => {
  return (
    <>
      <input type="text" />
      <Person
        personQuery="me"
        view={5}
        personCardInteraction={2}
      />
    </>
  );
};
export default TestPerson;

I really don't get why is not getting the focus, as I can see the tabindex in the shadow DOM image

sebastienlevert commented 2 years ago

@Rafaelki do you see this only on SPFx? Are you using the mgt-spfx package? I'm wondering if this is specific the way it's loaded on the page.

ghost commented 2 years ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

Rafaelki commented 2 years ago

I am using the mgt-spfx library. Let me try to use mgt-react directly...

gavinbarron commented 1 year ago

@Mnickii can you please retest this in a SPFx web part using both the mgt-spfx package and again with using mgt-react + disambiguation (i.e. our sample web part)

Rafaelki commented 1 year ago

Am I right in thinking that the mgt-spfx library in the app catalog is not needed anymore when using only v3?

I have taken your example with disambiguation and I am seeing the same issue:

report

PACKAGE.JSON

    "@microsoft/mgt-element": "*",
    "@microsoft/mgt-react": "*",
    "@microsoft/mgt-spfx-utils": "*",
    "@microsoft/mgt-sharepoint-provider": "*",
    "@microsoft/sp-component-base": "1.17.4",

WEBPART

// set the disambiguation before initializing any web part
customElementHelper.withDisambiguation('fresh');

export default class HelloWorldWebPart extends BaseClientSideWebPart<IHelloWorldWebPartProps> {
  public render(): void {
    const element = lazyLoadComponent(HelloWorld, {});
    ReactDom.render(element, this.domElement);
  }

  protected onInit(): Promise<void> {
    if (!Providers.globalProvider) {
      Providers.globalProvider = new SharePointProvider(this.context);
    }
    return Promise.resolve();
  }

COMPONENT

import { Person } from '@microsoft/mgt-react/dist/es6/generated/react';
import { PersonViewType, PersonCardInteraction } from '@microsoft/mgt-react/dist/es6/index';

export default class HelloWorld extends React.Component<IHelloWorldProps, {}> {
  public render(): React.ReactElement<IHelloWorldProps> {
    return (
      <section>
        <input type="text" />

        <Person
          personQuery="me"
          view={PersonViewType.twolines}
          personCardInteraction={PersonCardInteraction.hover}
          showPresence={true}
        />

        <Person
          personQuery="me"
          view={PersonViewType.twolines}
          personCardInteraction={PersonCardInteraction.hover}
          showPresence={true}
        />
        <input type="text" />
      </section>
    );
  }
}
Mnickii commented 1 year ago

I've also been able to replicate the bug in both scenarios @gavinbarron. Still investigating this, but I have not yet tracked down the cause.

gavinbarron commented 1 year ago

@Rafaelki thank you so much for your help on this. Yes, when using MGT v3 and disambiguation the intent is that you no longer need to have the mgt-spfx library. This does increase the package size but should provide a more robust experience with multiple solutions in a single page.

gavinbarron commented 1 year ago

Based on testing this issue is not present when the same react componentry is used outside of a SharePoint context. It appears that there is an event listener that is intercepting the tab keydown and attempting to manage focus but failing to look inside shadowroot nodes correctly for interactive elements.

I'm working on finding where to route this issue to internally as this is an issue for anyone building web parts with web components and not just MGT

sebastienlevert commented 10 months ago

As this is external to us and that the linked issue is attached on the thread, I'll go ahead and close the issue. Feel free to re-open if you think we should re-consider! Thanks!