Remove activedescendant focus pattern from SelectPanel #5688

camertron · 2025-02-11T22:44:51Z

Closes https://github.com/github/primer/issues/4740

Changelog

New

Changed

Remove the activedescendant focus pattern from SelectPanel in favor of regular 'ol focus behavior with a roving tab index.

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
~~- [ ] Added/updated documentation~~
~~- [ ] Added/updated previews (Storybook)~~
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2025-02-11T22:44:54Z

🦋 Changeset detected

Latest commit: df77160

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-02-11T22:45:25Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

camertron · 2025-02-11T22:49:22Z

packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx

@@ -243,7 +205,6 @@ function MappedActionListItem(item: ItemInput & {renderItem?: RenderItemFn}) {

  return (
    <ActionList.Item
-      role="option"


The role is automatically inferred by ActionList.

camertron · 2025-02-11T22:53:30Z

packages/react/src/SelectPanel/SelectPanel.tsx

-            className={enabled ? clsx(className, classes.FilteredActionList) : className}
-            announcementsEnabled={false}
-          />
+          <ActionListContainerContext.Provider value={{container: 'SelectPanel', focusZoneFocusOutBehavior: 'wrap'}}>


This is only for the modern ActionList and shouldn't affect the deprecated one.

camertron · 2025-02-11T22:53:59Z

packages/react/src/deprecated/ActionList/Item.tsx

@@ -253,7 +253,6 @@ const StyledItem = styled.div<

  &:focus {
    background: ${({variant, item}) => getItemVariant(variant, item?.disabled).focusBg};
-    outline: none;


Allows items to appear visually focused.

packages/react/src/deprecated/ActionList/List.tsx

github-actions · 2025-02-12T20:05:13Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	105.18 KB (-0.34% 🔽)
packages/react/dist/browser.umd.js	105.54 KB (-0.32% 🔽)

primer-integration · 2025-02-12T20:22:43Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/362191

primer-integration · 2025-02-13T22:27:17Z

🟢 golden-jobs completed with status success.

francinelucca

Looking great! just a question on the new FilterActionList custom attribute

packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx

…mer/react into select_panel_remove_activedescendant

github-actions · 2025-02-14T19:41:52Z

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

github-actions · 2025-02-14T20:11:36Z

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

github-actions · 2025-02-14T20:22:19Z

👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks!

TylerJDev

Took a look and everything's looking good! Will follow up in my next review to test with a screen reader!

TylerJDev · 2025-02-15T00:00:22Z

packages/react/src/FilteredActionList/FilteredActionListWithDeprecatedActionList.tsx

+            items={items.map(item => {
+              return {
+                ...item,
+                'data-select-panel-item': container === 'SelectPanel',
+                role: 'option',
+              }
+            })}


Should we utilize useMemo here? I noticed that this was being called a few times on the initial render. I think if we utilize useMemo we could reduce it to one since we'll memorize the array.

const filteredItems: ItemInput[] = React.useMemo(() => { return items.map(item => { return { ...item, 'data-select-panel-item': container === 'SelectPanel', role: 'option', } }) }, [items, container])

Not a big concern though, so up to you! I believe this will still be called upon filtering existing options, so this only helps us when it comes to the initial render.

TylerJDev · 2025-02-15T00:02:34Z

.changeset/yellow-pears-grin.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch


Nit: I'm wondering if this should be minor instead of patch. Entirely up to you though! 😁

TylerJDev · 2025-02-15T00:10:08Z

packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx

 function MappedActionListItem(item: ItemInput & {renderItem?: RenderItemFn}) {
+  const {container} = useContext(ActionListContainerContext)
+
  // keep backward compatibility for renderItem
  // escape hatch for custom Item rendering
-  if (typeof item.renderItem === 'function') return item.renderItem(item)
+  if (typeof item.renderItem === 'function') {
+    if (container === 'SelectPanel') {
+      return React.cloneElement(item.renderItem(item), {
+        'data-select-panel-item': true,
+      })
+    }
+
+    return item.renderItem(item)
+  }


Do we have an example that utilizes this? Not a blocker, but just curious 🤔

TylerJDev · 2025-02-15T00:11:34Z

packages/react/src/SelectPanel/SelectPanel.tsx

-const getItemWithActiveDescendant = (
-  listRef: React.RefObject<HTMLElement>,
-  items: FilteredActionListProps['items'],
-) => {
-  const listElement = listRef.current
-  const activeItemElement = listElement?.querySelector('[data-is-active-descendant]')
-
-  if (!listElement || !activeItemElement?.textContent) return
-
-  const optionElements = listElement.querySelectorAll('[role="option"]')
-
-  const index = Array.from(optionElements).indexOf(activeItemElement)
-  const activeItem = items[index] as ItemInput | undefined
-
-  const text = activeItem?.text
-  const selected = activeItem?.selected
-
-  return {index, text, selected}
-}


Wow, active descendant used a lot of code! It's great that we're able to clean it all up 🥹

TylerJDev · 2025-02-15T00:24:34Z

packages/react/src/SelectPanel/SelectPanel.tsx

+  useEffect(() => {
+    if (open && inputRef) {
+      inputRef.current?.focus()
+    }
+  }, [inputRef, open])


We could potentially add the .focus() inside of onInputRefChanged, as this appears to be where we get the input ref initially, and avoids using useEffect.

Though I'm not 100% on when onInputRefChanged is called. It seems like only when the ref is set, so should be initial render? 🤔

It looks like we already focus it even without the .focus() we provide here. I'm wondering if this is because the container (AnchoredOverlay) automatically focuses the first focusable element?

I think this is set via the following on (currently) line 357:

const focusTrapSettings = { initialFocusRef: inputRef || undefined, }

We might be able to omit the useEffect entirely, (and my previous comment 😅)

Remove activedescendant focus pattern from SelectPanel

c200f4e

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 11, 2025

Add changeset

bbacc68

camertron commented Feb 11, 2025

View reviewed changes

github-actions bot requested a deployment to storybook-preview-5688 February 11, 2025 22:50 Abandoned

camertron commented Feb 11, 2025

View reviewed changes

packages/react/src/deprecated/ActionList/List.tsx Outdated Show resolved Hide resolved

camertron temporarily deployed to github-pages February 11, 2025 23:07 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5688 February 11, 2025 23:08 Inactive

Fix type issues

e47e667

camertron temporarily deployed to github-pages February 12, 2025 20:14 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5688 February 12, 2025 20:15 Inactive

camertron added the update snapshots label Feb 12, 2025

Merge branch 'main' into select_panel_remove_activedescendant

6038e45

test(vrt): update snapshots

403ce75

github-actions bot removed the update snapshots label Feb 12, 2025

github-actions bot temporarily deployed to storybook-preview-5688 February 12, 2025 20:28 Inactive

primer bot temporarily deployed to github-pages February 12, 2025 20:40 Inactive

github-actions bot temporarily deployed to storybook-preview-5688 February 12, 2025 20:41 Inactive

camertron marked this pull request as ready for review February 13, 2025 21:32

camertron requested a review from a team as a code owner February 13, 2025 21:32

camertron requested review from maximedegreve, jonrohan and francinelucca and removed request for jonrohan and maximedegreve February 13, 2025 21:32

francinelucca reviewed Feb 14, 2025

View reviewed changes

camertron added 2 commits February 14, 2025 11:41

Remove unnecessary data attribute; add test

34dae73

Merge branch 'select_panel_remove_activedescendant' of github.com:pri…

2ba7e09

…mer/react into select_panel_remove_activedescendant

github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 14, 2025

camertron temporarily deployed to github-pages February 14, 2025 19:54 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-5688 February 14, 2025 19:54 Inactive

camertron added the update snapshots label Feb 14, 2025

Merge branch 'main' into select_panel_remove_activedescendant

e10826e

github-actions bot added integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Feb 14, 2025

test(vrt): update snapshots

df77160

github-actions bot temporarily deployed to storybook-preview-5688 February 14, 2025 20:25 Inactive

primer bot deployed to github-pages February 14, 2025 20:43 View deployment

github-actions bot deployed to storybook-preview-5688 February 14, 2025 20:43 View deployment

TylerJDev self-requested a review February 14, 2025 23:43

TylerJDev reviewed Feb 15, 2025

View reviewed changes

chars157 mentioned this pull request Feb 18, 2025

 #5703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove activedescendant focus pattern from SelectPanel #5688

Remove activedescendant focus pattern from SelectPanel #5688

camertron commented Feb 11, 2025

changeset-bot bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025

camertron Feb 11, 2025

camertron Feb 11, 2025

camertron Feb 11, 2025

github-actions bot commented Feb 12, 2025 •

edited

Loading

primer-integration bot commented Feb 12, 2025

primer-integration bot commented Feb 13, 2025

francinelucca left a comment

github-actions bot commented Feb 14, 2025

github-actions bot commented Feb 14, 2025

github-actions bot commented Feb 14, 2025

TylerJDev left a comment

TylerJDev Feb 15, 2025

TylerJDev Feb 15, 2025

TylerJDev Feb 15, 2025

TylerJDev Feb 15, 2025

TylerJDev Feb 15, 2025

TylerJDev Feb 15, 2025

Remove activedescendant focus pattern from SelectPanel #5688

Are you sure you want to change the base?

Remove activedescendant focus pattern from SelectPanel #5688

Conversation

camertron commented Feb 11, 2025

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Feb 11, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 12, 2025 • edited Loading

size-limit report 📦

primer-integration bot commented Feb 12, 2025

primer-integration bot commented Feb 13, 2025

francinelucca left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 14, 2025

github-actions bot commented Feb 14, 2025

github-actions bot commented Feb 14, 2025

TylerJDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 12, 2025 •

edited

Loading