Skip to content

Conversation

@siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Feb 12, 2024

Before After
Pressing space selects the focused item but also scrolls the list Pressing space selects the focused item but does not scrolls the list, nice!
Pressing space selects the focused item but also scrolls the list Pressing space selects the focused item but does not scrolls the list, nice!

But is it safe??

The way to prevent unwanted scroll is to event.preventDefault on ActionList keypress.

const itemKeyPressHandler = (event: React.KeyboardEvent<HTMLLIElement>) => {
+ if (event.key === 'Space') {
+   event.preventDefault()  // prevent scrolling on Space
+
+   // immediately reset defaultPrevented once it's job is done
+   // so as to not disturb the functions that use that event after this
+   event.defaultPrevented = false;
+ }; 

  // Enter and Space both select item
  if (event.key === 'Enter' || event.key === 'Space') onSelect(event, afterSelect)
}

We don't want this to have any unwanted side effects. This is the code block that is affected, simplified for readability:

const onSelect = (
  event: React.KeyboardEvent<HTMLLIElement>,
  afterSelect?: Function,
) => {
  if (typeof props.onSelect === 'function') props.onSelect(event)
  // ↑ we always call props.onSelect, so that's safe 🟢 
  // product developers can still call event.preventDefault() on this event 

  if (event.defaultPrevented) return; // 👀 
  // ↓ afterSelect would still be called because we reset defaultPrevented 🟢
  // this is an internal function that is passed by ActionMenu and SelectPanel
  if (typeof afterSelect === 'function') afterSelect()
}
  • afterSelect is called when it should, closing the menu automatically 🟢
  • ActionList.LinkItem: Links don't respond to Space, so this is unaffected 🟢
  • Wanted to check if there are any onSelect in dotcom that check for event.defaultPrevented, There are none, so this breaks nothing 🟢

@siddharthkp siddharthkp self-assigned this Feb 12, 2024
@changeset-bot
Copy link

changeset-bot bot commented Feb 12, 2024

🦋 Changeset detected

Latest commit: 85e4290

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
Copy link
Contributor

github-actions bot commented Feb 12, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 113.38 KB (+0.01% 🔺)
packages/react/dist/browser.umd.js 114.03 KB (+0.02% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-4259 February 12, 2024 16:34 Inactive
@siddharthkp siddharthkp added patch release bug fixes, docs, housekeeping backward-compatible labels Feb 13, 2024
@siddharthkp siddharthkp added this pull request to the merge queue Feb 13, 2024
Merged via the queue into main with commit c9fbef6 Feb 13, 2024
@siddharthkp siddharthkp deleted the actionlist-avoid-scroll-on-selection branch February 13, 2024 16:22
@primer primer bot mentioned this pull request Feb 13, 2024
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
* prevent default on Space

* immediately reset defaultPrevented

* improve comment

* Create slow-owls-report.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch release bug fixes, docs, housekeeping

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants