Skip to content

Conversation

@brianferry
Copy link
Collaborator

@brianferry brianferry commented Mar 28, 2023

What I did

  1. Removed auto focus on expanded for the accordion header

Closes #2454

This is so that if you have an expanded accordion down the page it doesn't automatically scroll the user down to the accordion.

Testing Instructions

  1. Check out the DP and verify the accordion docs page doesn't auto focus on the expanded header in the demo docs

@changeset-bot
Copy link

changeset-bot bot commented Mar 28, 2023

🦋 Changeset detected

Latest commit: 89cc48d

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

This PR includes changesets to release 1 package
Name Type
@patternfly/elements 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 github-actions bot added the functionality Functionality, typically pertaining to the JavaScript. label Mar 28, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2023

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 331a898
😎 Deploy Preview https://deploy-preview-2458--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the AT passed Automated testing has passed label Mar 28, 2023
@brianferry brianferry marked this pull request as draft March 29, 2023 00:06
@brianferry
Copy link
Collaborator Author

Actually not sure about this one, it seems like the focus should be on the header when it is expanded but I'm not sure if we want it to ignore focus when it's expanded on load or if we just want to update the docs for now and decide if we want to fix in a future update. Thoughts @markcaron @zeroedin @bennypowers ?

@github-actions github-actions bot added the work in progress POC / Not ready for review label Mar 29, 2023
@zeroedin
Copy link
Contributor

zeroedin commented Mar 29, 2023

Actually not sure about this one, it seems like the focus should be on the header when it is expanded

On page load focus / document.activeElement should be at the body element of the page. When a url#hash property (URI fragment) is present, then the reference of the hash should be the focused item. Is that what you are trying to portray with setting focus() here when an accordion header is expanded? Or maybe I'm misunderstanding the intent/problem, if so let me know.

From my understanding when an accordion header is expanded on a page it doesn't necessarily mean the user wants to immediately navigate to that item which is what setting focus there does. In contrast, when an url#hash is used then the intent is explicit by the user that they want to navigate to that spot.

Even still when the url#hash is used I don't think you'd set focus rather with the correct structure of the HTML page (given no iframes etc) the browser should handle that for us. @nikkimk does this sound correct / am I explaining that correctly?

Here is a gist demonstrating this: https://gist.github.com/zeroedin/a136884404105535111d09f2a0fc5c27 Run that in a browser ie something like (http://127.0.0.1:5500/index.html#expanded) given the url#hash to id="expanded" the browser itself will set initial focus on the expanded tab referenced in the url#hash document.activeElement is to the #expanded tab element.

@zeroedin
Copy link
Contributor

I'm pretty sure the only change that needs to be made is the removal of header.focus() in the expand() method in BaseAccordion.ts L:375. The click or keypress events are enough to give focus, don't need to explicitly set it on expand.

@bennypowers bennypowers force-pushed the fix/accordion/auto-focus-on-expanded branch from 14d1bf3 to 0b64048 Compare March 30, 2023 11:41
@bennypowers bennypowers marked this pull request as ready for review March 30, 2023 11:44
@bennypowers bennypowers enabled auto-merge (squash) March 30, 2023 11:44
@github-actions github-actions bot added the tests Related to testing label Mar 30, 2023
@bennypowers bennypowers merged commit 0895f50 into main Mar 30, 2023
@bennypowers bennypowers deleted the fix/accordion/auto-focus-on-expanded branch March 30, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AT passed Automated testing has passed functionality Functionality, typically pertaining to the JavaScript. tests Related to testing work in progress POC / Not ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug][docs] Accordion page load sets focus further down the page

4 participants