-
Notifications
You must be signed in to change notification settings - Fork 1
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
Focus on main when content changes #2221
base: main
Are you sure you want to change the base?
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/app/content/components/Page/scrollToTopOrHashManager.ts
Did you find this useful? React with a 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like in principle it should work
4881908
to
a052faa
Compare
0c53803
to
a6f8511
Compare
@RoyEJohnson The CI check |
33cdd33
to
da7eaad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic is looking cleaner now, although now that i'm looking at where you changed the code in scrollToTargetOrTop i'm wondering if it would be best to focus the target instead of the whole content when linking to a specific target
also that built CI check needs to be fixed |
592d0ac
to
47251c8
Compare
b5e1c56
to
a39ed17
Compare
Some scroll-to targets are not focusable. Footnotes, for example, are simply divs. They have id's so you can scroll to them, but no tabindex, so they don't focus. |
i wonder what the intended screen reader functionality is there.... surely if its scrolling to a target on the page its not supposed to start reading from the top of the content |
4a9e6e7
to
9fc4548
Compare
9fc4548
to
a4dd422
Compare
9ff533c
to
a251118
Compare
Just do the focus in ComponentDidUpdate
a251118
to
ac14937
Compare
It would be reasonable for screenreaders to recognize that content has scrolled and start reading visible content. |
I don't think I made any net changes to what's actually here, but the build test works now. All I did was restore some old versions of files to see what caused the test to fail. Then I got rid of those changes and squashed a few commits. |
Somehow I feel like the screen reader is not going to do the right thing here with the target scrolling, but I'll approve what you have working so far |
DISCO-198