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

[Feature]: sharing IDREF between compound components #24163

Open
1 task done
bsunderhus opened this issue Aug 1, 2022 · 10 comments
Open
1 task done

[Feature]: sharing IDREF between compound components #24163

bsunderhus opened this issue Aug 1, 2022 · 10 comments
Assignees

Comments

@bsunderhus
Copy link
Contributor

Library

React Components / v9 (@fluentui/react-components)

Describe the feature that you would like added

Current behavior

Most of v9 components introduce a default ID by context and passing it down the line to its compound component to consume.

// MenuGroup

export function useMenuGroup_unstable(props: MenuGroupProps, ref: React.Ref<HTMLElement>): MenuGroupState {
  const headerId = useId('menu-group');

  return {
    components: {
      root: 'div',
    },
    root: getNativeElementProps('div', {
      ref,
      'aria-labelledby': headerId, // adds aria attribute with header id
      role: 'group',
      ...props,
    }),
    headerId: headerId, // passes header id to be added through context
  };
}

// MenuGroupHeader

export function useMenuGroupHeader_unstable(
  props: MenuGroupHeaderProps,
  ref: React.Ref<HTMLElement>,
): MenuGroupHeaderState {
  const { headerId: id } = useMenuGroupContext_unstable(); // consumes headerId from MenuGroup provided context

  return {
    components: {
      root: 'div',
    },
    root: getNativeElementProps('div', {
      ref,
      id,
      ...props,
    }),
  };
}

Problem Statement

Some ARIA Patterns require Id references being shared between compound components through Context to ensure functionality, for example: aria-labelledby, aria-describedby, aria-controls, etc,.

In the example usage above of MenuGroup and MenuGroupHeader, If the user provides its own id to MenuGroupHeader, and stops using the one provided by context, then MenuGroup would start pointing aria-labelledby to the wrong place. Current solution is documenting this problem and stating for the user that it is its own job to re-introduce the proper aria-labelledby attribute on its own:

export const GroupingItems = () => (
  <Menu>
    <MenuTrigger>
      <Button>Toggle menu</Button>
    </MenuTrigger>

    <MenuPopover>
      <MenuList>
        {/* aria-labelledby should be re-introduced here, otherwise MenuGroup would point to a unused id */}
        <MenuGroup aria-labelledby="custom-id">
          <MenuGroupHeader id="custom-id">Section header</MenuGroupHeader>
          <MenuItem icon={<CutIcon />}>Cut</MenuItem>
          <MenuItem icon={<PasteIcon />}>Paste</MenuItem>
          <MenuItem icon={<EditIcon />}>Edit</MenuItem>
        </MenuGroup>
        <MenuDivider />
      </MenuList>
    </MenuPopover>
  </Menu>
);

Feature Request

To solve this problem, some sort of context mechanism to add IDREF between compound components is required.

One problem with this is the extra render this would require in the case a child component updates its id reference to its parent, but so far, I can't think of a better solution.

THIS IS STILL OPEN FOR DEBATE

Have you discussed this feature with our team

teams-prague

Additional context

No response

Validations

  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
@bsunderhus bsunderhus self-assigned this Aug 1, 2022
@ling1726
Copy link
Member

ling1726 commented Aug 1, 2022

It would be a good issue to solve, it's been tracked for Menu in #18318

@ling1726
Copy link
Member

ling1726 commented Aug 1, 2022

What would be the source of truth ? the aria-xxx attribute or theid?

@bsunderhus
Copy link
Contributor Author

What would be the source of truth? the aria-xxx attribute or the id?

I'd assume the id should be the source of truth, as that's the most expected property to be introduced by user.

@ling1726
Copy link
Member

ling1726 commented Aug 1, 2022

What would be the source of truth? the aria-xxx attribute or the id?

I'd assume the id should be the source of truth, as that's the most expected property to be introduced by user.

In that case I think the forced render is fine.

Perhaps there might also be some way to do it on DOM with ref ? you could track the id through a ref and calling some kind of setUserId callback in context could just update that directly in DOM

@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Jan 5, 2023
@ling1726 ling1726 reopened this Jan 5, 2023
@layershifter layershifter removed the Resolution: Soft Close Soft closing inactive issues over a certain period label Jan 23, 2023
@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Jun 22, 2023
@ling1726 ling1726 reopened this Jun 26, 2023
@bsunderhus bsunderhus removed the Resolution: Soft Close Soft closing inactive issues over a certain period label Aug 10, 2023

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Jan 7, 2024
@ling1726 ling1726 reopened this Jan 8, 2024

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes.
Still require assistance? Please add comment - "keep open".

1 similar comment

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes.
Still require assistance? Please add comment - "keep open".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants