Add ARIA roles and labelledby to branch dropdown tabs

[jacortinas]

References:

• https://github.com/github/accessibility-audits/issues/4428

Description

When opening the branches dropdown, focus is automatically shifted to the filter input of the most recently selected tab. This is either the filter input within the "Branches" tab or the "Pull Requests" tab. The issue is that without proper ARIA role and aria-labelledby attributes, screen readers do not announce any context regarding the parent elements of the input currently in focus. This PR fixes that by passing role and labelledBy properties from the branch dropdown down to the child FilterList-ish components.

This could have been solved in other ways by adding parent div elements inside of the BranchesContainer component, that contained the correct role="tabpanel" and aria-labelledby attributes. However, I think there is a benefit to the commonly used FilterList and SectionFilterList components being able to have different ARIA attributes depending on the parent components usage of the properties.

Screenshots

Release notes

Notes: [New] Improve branch dropdown tabs screen reader support

[tidy-dev]

This is working as expected. 💖

This could have been solved in other ways by adding parent div elements inside of the BranchesContainer component, that contained the correct role="tabpanel" and aria-labelledby attributes. However, I think there is a benefit to the commonly used FilterList and SectionFilterList components being able to have different ARIA attributes depending on the parent components usage of the properties.

I think I prefer that solution. Reason being is, in this particular scenario, the list is only contents of the tabpanel, but technically someday we may decide to add more controls to this tab and then this would need refactored. Also, is not a common pattern for a list itself to be a tabpanel nor have a role placed on it's parent container. The list itself may and that is already handled in the list component, but not the container. Another smaller concern is that the addition of props like this makes it seem like it is a reasonable expectation to provide an aria-labelledby to a list container which is only WCAG correct if that div has a role added to it that also requires a label association. In this use case, that is true, but we are opening the props up to any combination of usage.. which could lead a future developer to a mistake since "it can" accept it.

Does that make sense?

[tidy-dev]

This is looking great! Thank you for that change.

One small nit I promise then we can get it merged.

What do you think about reducing some of the reused logic and have a method like:

private renderSelectedTab() {
    const { selectedTab, repository } = this.props

    const ariaLabelledBy =
      selectedTab === BranchesTab.Branches || !repository.gitHubRepository
        ? 'branches-tab'
        : 'pull-requests-tab'

    return (
      <div
        role="tabpanel"
        aria-labelledby={ariaLabelledBy}
        className="branches-container-panel"
      >
        {this.renderSelectedTabContent()}
      </div>
    )
  }

and rename the current renderSelectedTab to renderSelectedTabContent?

[tidy-dev]

✨ Thank you again for the change. 🙏 LGTM!