-
Notifications
You must be signed in to change notification settings - Fork 34
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
fix(appbar-accessibility): Fix app bar accessibility #1867
fix(appbar-accessibility): Fix app bar accessibility #1867
Conversation
Observation 1- In the search list, User keep tabbing the search result and when user select arrow keys, it goes into the map and map is scrolling up and down Observation 2: Initially, the focus is not capturing the search button, instead it continue to go to map’s footer panel functionalities and then when we do Shift +TAB then focus come to the Search Icon (Left panel of map) Observation 3: |
83fb4c6
to
9e4f689
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.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @amir-azma)
packages/geoview-core/src/core/components/common/focus-trap-element.tsx
line 1 at r2 (raw file):
/* eslint-disable react/require-default-props */
No global eslint disable
packages/geoview-core/src/core/components/common/focus-trap-element.tsx
line 61 at r2 (raw file):
return ( <FocusTrap open={basic ? active : id === focusItem.activeElementId}> {!basic ? (
What does basic do, add comment
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 113 at r2 (raw file):
const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Escape') {
This whole listener on esc should be in a focus trap utility function an reuse everywhere
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 114 at r2 (raw file):
const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Escape') { setGeolocatorActive(false);
Focus goes back to geolocator but I have to press enter twice to get back in
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 119 at r2 (raw file):
if (ARROW_KEY_CODES.includes(event.code)) { // TODO stop moving the map here event.preventDefault();
Doesnnot wrk, using arrow still move themaps. This arrow, ont moving the map should be handle at the same place as the crosshair on map focus. The arrow hould move map only when focus
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 130 at r2 (raw file):
if (active) { document.addEventListener('keydown', handleKeyDown);
Listener is added every time it is active but remove only on unmount, we may end up with many listener I think...
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.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/core/components/common/focus-trap-element.tsx
line 1 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
No global eslint disable
Removed the eslint disable, added defaultProps to component
packages/geoview-core/src/core/components/common/focus-trap-element.tsx
line 61 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
What does basic do, add comment
Done
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 113 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
This whole listener on esc should be in a focus trap utility function an reuse everywhere
Once we call setGeolocatorActive(false); it set active to be false to close the search panel, then since active is false, the focus trap disables allowing to continue tab to other sections, the purpose of this Esc event listener is not disabling focus trap at the beginning, that's my idea
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 114 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Focus goes back to geolocator but I have to press enter twice to get back in
That's a little tricky, need to check
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 119 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Doesnnot wrk, using arrow still move themaps. This arrow, ont moving the map should be handle at the same place as the crosshair on map focus. The arrow hould move map only when focus
I am reviewing it
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 130 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Listener is added every time it is active but remove only on unmount, we may end up with many listener I think...
In this case in component unmount, we remove the "handleKeyDown" listener for sure, I checked that by console.log, when we press Esc, then 'active' becomes false, closing panel, component unmounts, then pressing Esc again doesn't call event "handleKeyDown" since it's cleared inside the useEffect return ()..
2941c16
to
32b3514
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @amir-azma)
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 114 at r2 (raw file):
Previously, amir-azma wrote…
That's a little tricky, need to check
Not solved
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 119 at r2 (raw file):
Previously, amir-azma wrote…
I am reviewing it
Not working
32b3514
to
5816f12
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 119 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Not working
Applied the fix please check host url
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 114 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Not solved
Once we hit Esc, the geolocator icon is active (shows the tooltip) but first hit enter doesn't open it, then the second enter opens, I still don't know why if the button is active why hitting enter doesn't open the panel
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.
Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @amir-azma)
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 157 at r5 (raw file):
const handleKeyDown = (event: KeyboardEvent) => { // disables map interactions (arrow keys won't move the map) setMapKeyboardPanInteractions(0);
This need to be in a the crosshair component. Keyboard pan interaction are on when crosshair is enable. This way it will work everything. In your case, you do notput back interaction when crosshair is enable again
cb2c2b9
to
96d5903
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.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/core/components/geolocator/geolocator.tsx
line 157 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
This need to be in a the crosshair component. Keyboard pan interaction are on when crosshair is enable. This way it will work everything. In your case, you do notput back interaction when crosshair is enable again
I moved this logic to Crosshair component
d1a3b69
to
cf8c826
Compare
We close as we will deal with this later |
Description
Fix some accessibility features in app bar:
AppBar
Fixes #1792
Type of change
How Has This Been Tested?
https://amir-azma.github.io/geoview/raw-feature-info.html
Checklist:
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksNew and existing unit tests pass locally with my changesThis change is