fix(wcag): accessible keyboard navigation #3215
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements incremental WCAG accessibility improvements to make keyboard navigation more intuitive and accessible across the GeoView application. The changes focus on proper focus management, semantic HTML, ARIA attributes, and keyboard event handling.
Key changes:
- Enhanced focus management for panels, modals, and dialogs with proper focus return on close
- Improved semantic HTML structure using List and ListItem components with proper ARIA roles
- Added keyboard navigation support (Enter, Space, Esc keys) to interactive components
- Updated ARIA labels and roles for better screen reader support
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/geoview-core/src/ui/tabs/tabs.tsx | Added translatable aria-label for tabs |
| packages/geoview-core/src/ui/tabs/tab-panel.tsx | Added semantic section component to tab panel |
| packages/geoview-core/src/ui/panel/panel.tsx | Added dialog role for accessibility |
| packages/geoview-core/src/core/components/notifications/notifications.tsx | Fixed focus selector to target h2 instead of h3 |
| packages/geoview-core/src/core/components/legend/legend.tsx | Replaced Box with List component for semantic HTML |
| packages/geoview-core/src/core/components/legend/legend-styles.ts | Reorganized styles to support new semantic structure |
| packages/geoview-core/src/core/components/legend/legend-layer.tsx | Updated structure to use ListItem with proper styles |
| packages/geoview-core/src/core/components/legend/legend-layer-items.tsx | Implemented keyboard navigation with focus preservation for toggleable items |
| packages/geoview-core/src/core/components/geolocator/geolocator.tsx | Added ESC key handling, auto-focus on open, and dialog role |
| packages/geoview-core/src/core/components/geolocator/geolocator-bar.tsx | Added inputRef prop and removed autofocus attribute |
| packages/geoview-core/src/core/components/export/export-modal.tsx | Implemented auto-focus on title input and proper focus return on close |
| packages/geoview-core/src/core/components/export/export-modal-button.tsx | Added aria-controls attribute linking to modal |
| packages/geoview-core/src/core/components/details/details-panel.tsx | Auto-show guide when no features and hide nav buttons for single feature |
| packages/geoview-core/src/core/components/common/layer-list.tsx | Refactored to use semantic HTML with ListItemButton for accessibility |
| packages/geoview-core/src/core/components/common/layer-list-style.ts | Updated styles for new semantic structure |
| packages/geoview-core/src/core/components/common/layer-icon.tsx | Removed tabIndex=-1 and updated to use span components |
| packages/geoview-core/src/core/components/common/focus-trap-container.tsx | Minor spacing fix |
| packages/geoview-core/src/core/components/app-bar/app-bar.tsx | Updated ESC key handler to use correct button ID |
| packages/geoview-core/src/core/components/app-bar/app-bar-helper.ts | Updated focus target to use -panel-btn suffix |
| packages/geoview-core/public/locales/fr/translation.json | Added French translations for new aria-labels |
| packages/geoview-core/public/locales/en/translation.json | Added English translations for new aria-labels |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <ListItemButton | ||
| component="button" | ||
| sx={sxClasses.listItemButton} | ||
| className={containerClass} | ||
| onKeyDown={(e) => handleLayerKeyDown(e, layer)} | ||
| onClick={() => onListItemClick(layer)} | ||
| selected={isSelected} | ||
| // disable when layer features has null value. | ||
| disabled={isDisabled || isLoading} | ||
| aria-disabled={isDisabled || isLoading} | ||
| > |
There was a problem hiding this comment.
The ListItemButton component doesn't explicitly handle the spacebar key. While MUI's ListItemButton should handle spacebar by default for triggering onClick, the custom onKeyDown handler only checks for 'Enter'. Consider explicitly handling ' ' (spacebar) as well for consistency and to ensure accessibility best practices are followed, or verify through testing that the default spacebar behavior works correctly with the custom keyboard handler.
| const buttonElement = buttonId && document.getElementById(mapId)?.querySelector(`#${buttonId}`); | ||
| if (buttonElement) { | ||
| // put back focus on calling button | ||
| document.getElementById(buttonId)?.focus(); | ||
| document.getElementById(buttonId + '-panel-btn')?.focus(); |
There was a problem hiding this comment.
The function attempts to focus an element with ID buttonId + '-panel-btn', but it first queries for buttonId without the suffix. If buttonElement is found (line 49), the code tries to focus buttonId + '-panel-btn' (line 52). However, if the element with just buttonId exists but buttonId + '-panel-btn' doesn't exist, the focus call will silently fail. Consider checking if the suffixed element exists before attempting to focus it, or ensure consistency in the element ID pattern throughout the codebase.
| onClose={handleClickAway} | ||
| container={mapElem} | ||
| focusSelector="h3 + div button:last-child" | ||
| focusSelector="h2 + div button:last-child" |
There was a problem hiding this comment.
The focus selector logic assumes there's a button element to target but this may be incorrect if the first button found is not the close button. The selector "h2 + div button:last-child" finds the last button in the div following h2, but in a complex notification dialog there could be multiple buttons. Consider using a more specific selector like a data attribute or ID on the close button to ensure the correct element receives focus.
| focusSelector="h2 + div button:last-child" | |
| focusSelector="#notification-button" |
| </List> | ||
| )); | ||
| }, [formattedLegendLayerList, noLayersContent, containerType]); | ||
| }, [formattedLegendLayerList, noLayersContent, containerType, sxClasses]); |
There was a problem hiding this comment.
The sxClasses dependency is added to the useMemo but sxClasses is itself a memoized value created from the theme, which is already stable. This creates an unnecessary dependency that could cause extra re-renders when sxClasses object identity changes even though the styles haven't changed. Since sxClasses is derived from theme and theme is stable, the dependency on sxClasses is redundant.
| useEffect(() => { | ||
| logger.logTraceUseEffect('GEOLOCATOR - handleKeyDown', isOpen, tabId); | ||
|
|
||
| const handleKeyDown = (event: KeyboardEvent): void => { | ||
| if (event.key === 'Escape') { | ||
| handleReset(); | ||
| } | ||
| }; | ||
|
|
||
| if (isOpen && tabId === DEFAULT_APPBAR_CORE.GEOLOCATOR) { | ||
| document.addEventListener('keydown', handleKeyDown); | ||
| } | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| }; | ||
| }, [isOpen, tabId, handleReset]); |
There was a problem hiding this comment.
The ESC key listener is added to the entire document, which means it will trigger even when focus is outside the geolocator panel. This could interfere with other components that also handle ESC (like modals). Consider attaching the listener to the geolocator panel element instead of the document, or add a check to ensure the geolocator has focus before handling the ESC key.
| // Auto focus on title input when modal opens. Delay to allow Dialog to render first. | ||
| useEffect(() => { | ||
| if (activeModalId === 'export') { | ||
| setTimeout(() => { | ||
| const titleInput = document.querySelector('#export-title-input') as HTMLInputElement; | ||
| if (titleInput) { | ||
| titleInput.focus(); | ||
| } | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
The 500ms delay for auto-focusing the title input seems arbitrary and could be too long or too short depending on the Dialog render performance. This could result in the focus occurring before the Dialog is fully rendered (causing focus to fail) or after an unnecessary delay. Consider using a ref callback or the Dialog's onEntered event prop to focus the input reliably after the Dialog animation completes, rather than relying on a hardcoded timeout.
| // Auto focus on title input when modal opens. Delay to allow Dialog to render first. | |
| useEffect(() => { | |
| if (activeModalId === 'export') { | |
| setTimeout(() => { | |
| const titleInput = document.querySelector('#export-title-input') as HTMLInputElement; | |
| if (titleInput) { | |
| titleInput.focus(); | |
| } | |
| }, 500); | |
| } | |
| // Auto focus on title input when modal opens. Use requestAnimationFrame to allow Dialog to render first. | |
| useEffect(() => { | |
| let animationFrameId: number | undefined; | |
| if (activeModalId === 'export') { | |
| animationFrameId = window.requestAnimationFrame(() => { | |
| const titleInput = document.querySelector('#export-title-input') as HTMLInputElement | null; | |
| if (titleInput) { | |
| titleInput.focus(); | |
| } | |
| }); | |
| } | |
| return () => { | |
| if (animationFrameId !== undefined) { | |
| cancelAnimationFrame(animationFrameId); | |
| } | |
| }; |
| useEffect(() => { | ||
| if (activeModalId === 'export') { | ||
| setTimeout(() => { | ||
| const titleInput = document.querySelector('#export-title-input') as HTMLInputElement; |
There was a problem hiding this comment.
Using querySelector to find an element by ID is unnecessarily complex and less efficient than using getElementById. Replace document.querySelector('#export-title-input') with document.getElementById('export-title-input') for better performance and clarity.
| const titleInput = document.querySelector('#export-title-input') as HTMLInputElement; | |
| const titleInput = document.getElementById('export-title-input') as HTMLInputElement; |
| logger.logTraceUseCallback('EXPORT-MODAL - handleCloseModal'); | ||
| setActiveAppBarTab('legend', false, false); | ||
| disableFocusTrap(); | ||
| // Defer focus return to let Dialog's focus trap release first. Ensures export button in the appbar gets re-focused. |
There was a problem hiding this comment.
The comment mentions "Ensures export button in the appbar gets re-focused" but the focus return is handled by the disableFocusTrap function which uses the callbackElementId passed during modal open. The setTimeout with 0ms delay is used to defer execution to the next tick. While this works, it would be clearer to explain that this allows the Dialog's focus trap to release before attempting to return focus, preventing a race condition.
| // Defer focus return to let Dialog's focus trap release first. Ensures export button in the appbar gets re-focused. | |
| // Defer disabling the focus trap to the next tick so the Dialog's own focus trap | |
| // has time to release before we attempt to restore focus via disableFocusTrap | |
| // (which uses the callbackElementId to re-focus the export button in the app bar). |
kenchase
force-pushed
the
3116-keyboard-navigation
branch
from
2f057f2 to
b1a7d2a
Compare
kenchase
left a comment
kenchase
left a comment
There was a problem hiding this comment.
@kenchase resolved 8 discussions.
Reviewable status: 0 of 21 files reviewed, all discussions resolved (waiting on @jolevesq).
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 21 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kenchase).
packages/geoview-core/src/core/components/common/layer-list.tsx line 91 at r2 (raw file):
/** * Handle layer selection with keyboard (Enter or Spacebar).
I tried space bar on layer panel footer tab and it is not working. Works on detail.. layers list from layers is different then the one from responsive grid use by details. Just take a note for future work on layers panel
packages/geoview-core/src/core/components/notifications/notifications.tsx line 130 at r2 (raw file):
</Button> <IconButton id="notification-close-button"
When we have id, do we want to always prefix or suffix the mapId so they are unique
packages/geoview-core/src/core/components/details/details-panel.tsx line 649 at r2 (raw file):
useEffect(() => { // Log logger.logTraceUseEffect('DETAILS-PANEL - check for auto-guide display', activeAppBarTab, selectedTab, arrayOfLayerDataBatch);
When guide is shown because there is no layers with selected feature, should the Close button on right panel be disabled because clicking on it does nothing
packages/geoview-core/src/core/components/app-bar/app-bar-helper.ts line 49 at r2 (raw file):
helpOpenClosePanelByIdState(buttonId, setterCallback, false); const buttonElementId = buttonId + '-panel-btn';
We should use ${buttonId}-panel-btn
packages/geoview-core/src/core/components/legend/legend-layer-items.tsx line 45 at r2 (raw file):
<ListItem sx={sxClasses.layerListItem} disablePadding className={`layerListItem ${itemClassName || ''}`}> {onToggle ? ( <ListItemButton
Same in upstream, we have lost the tooltip on button/icon (toggleItemsVisibility)
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq made 2 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kenchase).
packages/geoview-core/src/core/components/app-bar/app-bar-helper.ts line 49 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
We should use
${buttonId}-panel-btn
And add mapId
packages/geoview-core/src/core/components/legend/legend-layer-items.tsx line 119 at r2 (raw file):
const handleToggleItemVisibility = useCallback( (item: TypeLegendItem): void => { lastToggledRef.current = `legend-item-${item.name}`;
Add mapId?
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 21 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @kenchase).
packages/geoview-core/src/core/components/details/details-panel.tsx line 654 at r2 (raw file):
const isDetailsActive = (containerType === CONTAINER_TYPE.FOOTER_BAR && selectedTab === TABS.DETAILS && !isCollapsed) || (containerType === CONTAINER_TYPE.APP_BAR && activeAppBarTab.tabId === 'details' && activeAppBarTab.isOpen);
Should use TABS.DETAILS instead of 'details' here
packages/geoview-core/src/core/components/geolocator/geolocator.tsx line 99 at r2 (raw file):
if (isOpen && tabId === DEFAULT_APPBAR_CORE.GEOLOCATOR && searchInputRef.current) { setTimeout(() => {
Could we explain the reason for the setTimeout here, I'm guessing a UI transition? If so, can we 'know' what the transition is and use that in the timeout delay? I know we do that sometimes elsewhere in the code and we've even set a constant somewhere on that matter.
packages/geoview-core/src/core/components/export/export-modal.tsx line 184 at r2 (raw file):
// (which uses the callbackElementId to re-focus the export button in the app bar). setTimeout(() => { disableFocusTrap();
Question: I see a few places in the code where 'disableFocusTrap' has to be in a setTimeout and sometimes not? If it should most of the time (or always) be executed on the next tick, maybe 'disableFocusTrap' could be async and return a Promise with the setTimeout internal to the 'disableFocusTrap' function.
ESLint will force devs to add a 'catch' of the promise every time the promise is ignored by the caller, but at least it would standardize the tick/setTimeout patterns?
kenchase
force-pushed
the
3116-keyboard-navigation
branch
from
b1a7d2a to
46fb688
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| disableFocusTrap(`${DEFAULT_APPBAR_CORE.GEOLOCATOR}-panel-btn`); | ||
| }, 0); | ||
| }, [setActiveAppBarTab, setSearchValue, disableFocusTrap]); |
There was a problem hiding this comment.
The button ID passed to disableFocusTrap should include the mapId suffix to match the actual button ID format. The geolocator button is created with ID ${buttonId}-panel-btn-${mapId} (as seen in app-bar.tsx line 346), but disableFocusTrap is being called with ${DEFAULT_APPBAR_CORE.GEOLOCATOR}-panel-btn which lacks the -${mapId} suffix. This will cause focus restoration to fail when closing the geolocator.
| disableFocusTrap(`${DEFAULT_APPBAR_CORE.GEOLOCATOR}-panel-btn`); | |
| }, 0); | |
| }, [setActiveAppBarTab, setSearchValue, disableFocusTrap]); | |
| disableFocusTrap(`${DEFAULT_APPBAR_CORE.GEOLOCATOR}-panel-btn-${mapId}`); | |
| }, 0); | |
| }, [setActiveAppBarTab, setSearchValue, disableFocusTrap, mapId]); |
|
|
||
| const panel = panelRef.current; | ||
| const handleKeyDown = (event: KeyboardEvent): void => { | ||
| handleEscapeKey(event.key, `${DEFAULT_APPBAR_CORE.GEOLOCATOR}-panel-btn`, false, handleReset); |
There was a problem hiding this comment.
The callbackId parameter passed to handleEscapeKey should include the mapId suffix to match the actual button ID format. The button ID is ${buttonId}-panel-btn-${mapId}, but this is passing ${DEFAULT_APPBAR_CORE.GEOLOCATOR}-panel-btn which lacks the -${mapId} suffix. This inconsistency will prevent proper focus restoration when ESC is pressed.
| onClose={hideClickMarker} | ||
| onKeyDown={(event: KeyboardEvent) => | ||
| handleEscapeKey(event.key, tabId, isFocusTrapped, () => { | ||
| handleEscapeKey(event.key, `${tabId}-panel-btn`, isFocusTrapped, () => { |
There was a problem hiding this comment.
The callbackId parameter passed to handleEscapeKey should include the mapId suffix to match the actual button ID format created on line 346. The button ID is ${buttonId}-panel-btn-${mapId}, but this is passing ${tabId}-panel-btn which lacks the -${mapId} suffix. This inconsistency will prevent proper focus restoration when ESC is pressed to close the panel.
| handleEscapeKey(event.key, `${tabId}-panel-btn`, isFocusTrapped, () => { | |
| handleEscapeKey(event.key, `${tabId}-panel-btn-${mapId}`, isFocusTrapped, () => { |
| return ( | ||
| <Box | ||
| component="section" | ||
| role="dialog" |
There was a problem hiding this comment.
Using component="section" with role="dialog" creates a conflict because semantic HTML elements should match their ARIA roles. A <section> element has an implicit ARIA role and adding role="dialog" overrides it. For a dialog, either use component="div" with role="dialog", or use a semantic element appropriate for non-modal panels. Since panels in this context appear to be non-modal sidebars rather than true dialogs, consider using role="complementary" or role="region" instead, which are more appropriate for sidebar panels.
| role="dialog" |
There was a problem hiding this comment.
The sections are being used as dialogs semantically (focus traps are being applied for keyboard users so the sections are behaving as modals). T
| <Box | ||
| ref={panelRef} | ||
| component="section" | ||
| role="dialog" |
There was a problem hiding this comment.
Using component="section" with role="dialog" creates a conflict because semantic HTML elements should match their ARIA roles. A <section> element has an implicit ARIA role and adding role="dialog" overrides it. For a dialog, either use component="div" with role="dialog", or use a semantic element appropriate for non-modal regions. Since the geolocator appears to be a non-modal search panel rather than a true dialog, consider using role="search" or role="region" instead, which are more appropriate for search panels.
| role="dialog" | |
| role="search" |
There was a problem hiding this comment.
This is behaving as modal from keyboard users (focus trap is enabled).
kenchase
force-pushed
the
3116-keyboard-navigation
branch
from
46fb688 to
27b359f
Compare
kenchase
force-pushed
the
3116-keyboard-navigation
branch
from
27b359f to
1a82c60
Compare
kenchase
left a comment
kenchase
left a comment
There was a problem hiding this comment.
@kenchase made 11 comments and resolved 3 discussions.
Reviewable status: 13 of 22 files reviewed, 10 unresolved discussions (waiting on @Alex-NRCan and @jolevesq).
packages/geoview-core/src/core/components/app-bar/app-bar-helper.ts line 49 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
And add mapId
Done.
packages/geoview-core/src/core/components/common/layer-list.tsx line 91 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
I tried space bar on layer panel footer tab and it is not working. Works on detail.. layers list from layers is different then the one from responsive grid use by details. Just take a note for future work on layers panel
Added to issue #3219
packages/geoview-core/src/core/components/details/details-panel.tsx line 649 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When guide is shown because there is no layers with selected feature, should the Close button on right panel be disabled because clicking on it does nothing
Added to issue #3217
packages/geoview-core/src/core/components/details/details-panel.tsx line 654 at r2 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Should use TABS.DETAILS instead of 'details' here
Done.
packages/geoview-core/src/core/components/export/export-modal.tsx line 184 at r2 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Question: I see a few places in the code where 'disableFocusTrap' has to be in a setTimeout and sometimes not? If it should most of the time (or always) be executed on the next tick, maybe 'disableFocusTrap' could be async and return a Promise with the setTimeout internal to the 'disableFocusTrap' function.
ESLint will force devs to add a 'catch' of the promise every time the promise is ignored by the caller, but at least it would standardize the tick/setTimeout patterns?
Created issue 3222 to look into this.
packages/geoview-core/src/core/components/geolocator/geolocator.tsx line 99 at r2 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Could we explain the reason for the setTimeout here, I'm guessing a UI transition? If so, can we 'know' what the transition is and use that in the timeout delay? I know we do that sometimes elsewhere in the code and we've even set a constant somewhere on that matter.
Removed. There is no UI transition here. I had this in as defensive programming, but it is unnecessary.
packages/geoview-core/src/core/components/legend/legend-layer-items.tsx line 45 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Same in upstream, we have lost the tooltip on button/icon (toggleItemsVisibility)
Added to issue 3218
packages/geoview-core/src/core/components/legend/legend-layer-items.tsx line 119 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add mapId?
Done.
packages/geoview-core/src/core/components/notifications/notifications.tsx line 130 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When we have id, do we want to always prefix or suffix the mapId so they are unique
Added issue 3220 to review other components using non-unique IDs
| <Box | ||
| ref={panelRef} | ||
| component="section" | ||
| role="dialog" |
There was a problem hiding this comment.
This is behaving as modal from keyboard users (focus trap is enabled).
| return ( | ||
| <Box | ||
| component="section" | ||
| role="dialog" |
There was a problem hiding this comment.
The sections are being used as dialogs semantically (focus traps are being applied for keyboard users so the sections are behaving as modals). T
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // example "Layer Name 123!" becomes "layer-name-123-" | ||
| const sanitizeId = (str: string): string => { | ||
| return str | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9-_]/g, '-') | ||
| .replace(/-+/g, '-'); |
There was a problem hiding this comment.
The sanitizeId function could produce IDs with leading/trailing hyphens or empty strings in edge cases. For example, if the input string starts or ends with special characters, or contains only special characters, the resulting ID could be invalid.
Consider adding logic to handle these cases:
- Trim leading/trailing hyphens
- Ensure the ID doesn't start with a digit (HTML IDs should start with a letter)
- Provide a fallback for empty results
| // example "Layer Name 123!" becomes "layer-name-123-" | |
| const sanitizeId = (str: string): string => { | |
| return str | |
| .toLowerCase() | |
| .replace(/[^a-z0-9-_]/g, '-') | |
| .replace(/-+/g, '-'); | |
| // Ensure no leading/trailing hyphens, avoid starting with a digit, and provide a non-empty fallback | |
| const sanitizeId = (str: string): string => { | |
| let sanitized = str | |
| .toLowerCase() | |
| .replace(/[^a-z0-9-_]/g, '-') | |
| .replace(/-+/g, '-') | |
| .replace(/^-+|-+$/g, ''); | |
| // Fallback if nothing remains after sanitization | |
| if (!sanitized) { | |
| sanitized = 'legend-item'; | |
| } | |
| // Ensure the ID segment does not start with a digit | |
| if (/^[0-9]/.test(sanitized)) { | |
| sanitized = `l-${sanitized}`; | |
| } | |
| return sanitized; |
There was a problem hiding this comment.
Will be reviewed in issue 3213
There was a problem hiding this comment.
Correction: This will be resolved as part of issue #3218
| <ListItemButton | ||
| id={`legend-item-${sanitizeId(name)}-${mapId}`} | ||
| component="button" | ||
| onClick={onToggle} | ||
| disableRipple | ||
| sx={sxClasses.layerListItemButton} | ||
| className={`layerListItemButton ${itemClassName || ''}`} | ||
| > | ||
| <ListItemIcon> | ||
| <Tooltip title={show ? value : ''} key={`Tooltip-${name}-${icon}1`} placement="left" disableHoverListener={!show}> | ||
| <Box sx={{ display: 'flex', padding: '0 18px 0 18px', margin: '0 -18px 0 -18px' }}> | ||
| {icon ? <Box component="img" alt="" src={icon} /> : <BrowserNotSupportedIcon />} | ||
| </Box> | ||
| </Tooltip> | ||
| </ListItemIcon> | ||
| <Tooltip | ||
| title={showNameTooltip ? name : ''} | ||
| key={`Tooltip-${name}-${icon}2`} | ||
| placement="top" | ||
| disableHoverListener={!showNameTooltip} | ||
| > | ||
| <ListItemText primary={name} /> | ||
| </Tooltip> | ||
| </ListItemButton> |
There was a problem hiding this comment.
The ListItemButton component is missing an aria-label attribute to provide an accessible name for screen reader users. While the ListItemText provides visual content, interactive buttons should have an explicit aria-label that describes the action being performed (e.g., "Toggle visibility for [layer name]").
There was a problem hiding this comment.
Will review as part of issu # 3218
| return ( | ||
| <Dialog open={activeModalId === 'export'} onClose={handleCloseModal} fullWidth maxWidth="xl" container={shellContainer}> | ||
| <Dialog | ||
| role="dialog" |
There was a problem hiding this comment.
The role="dialog" attribute is redundant on the Dialog component. The MUI Dialog component already has the dialog role built-in, so explicitly adding it here is unnecessary and could potentially conflict with the component's built-in ARIA attributes.
| role="dialog" |
There was a problem hiding this comment.
Will be reviewed as part of issue 3187
kenchase
left a comment
kenchase
left a comment
There was a problem hiding this comment.
@kenchase made 3 comments.
Reviewable status: 13 of 22 files reviewed, 13 unresolved discussions (waiting on @Alex-NRCan and @jolevesq).
| return ( | ||
| <Dialog open={activeModalId === 'export'} onClose={handleCloseModal} fullWidth maxWidth="xl" container={shellContainer}> | ||
| <Dialog | ||
| role="dialog" |
There was a problem hiding this comment.
Will be reviewed as part of issue 3187
| // example "Layer Name 123!" becomes "layer-name-123-" | ||
| const sanitizeId = (str: string): string => { | ||
| return str | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9-_]/g, '-') | ||
| .replace(/-+/g, '-'); |
There was a problem hiding this comment.
Will be reviewed in issue 3213
| <ListItemButton | ||
| id={`legend-item-${sanitizeId(name)}-${mapId}`} | ||
| component="button" | ||
| onClick={onToggle} | ||
| disableRipple | ||
| sx={sxClasses.layerListItemButton} | ||
| className={`layerListItemButton ${itemClassName || ''}`} | ||
| > | ||
| <ListItemIcon> | ||
| <Tooltip title={show ? value : ''} key={`Tooltip-${name}-${icon}1`} placement="left" disableHoverListener={!show}> | ||
| <Box sx={{ display: 'flex', padding: '0 18px 0 18px', margin: '0 -18px 0 -18px' }}> | ||
| {icon ? <Box component="img" alt="" src={icon} /> : <BrowserNotSupportedIcon />} | ||
| </Box> | ||
| </Tooltip> | ||
| </ListItemIcon> | ||
| <Tooltip | ||
| title={showNameTooltip ? name : ''} | ||
| key={`Tooltip-${name}-${icon}2`} | ||
| placement="top" | ||
| disableHoverListener={!showNameTooltip} | ||
| > | ||
| <ListItemText primary={name} /> | ||
| </Tooltip> | ||
| </ListItemButton> |
There was a problem hiding this comment.
Will review as part of issu # 3218
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 8 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: 21 of 22 files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan and @kenchase).
packages/geoview-core/src/core/components/notifications/notifications.tsx line 130 at r2 (raw file):
Previously, kenchase (Ken Chase) wrote…
Added issue 3220 to review other components using non-unique IDs
There is 3221 for global ifd management. We will enforce class-name-mapid syntax... mapId at the end like you did
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 9 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jolevesq and @kenchase).
packages/geoview-core/src/core/components/export/export-modal.tsx line 184 at r2 (raw file):
Previously, kenchase (Ken Chase) wrote…
Created issue 3222 to look into this.
I see the issue in GitHub, thank you, could you create 1 single TODO in the code and link the issue number in the TODO line?
Sorry, I know I'm asking for extra work here, but we have so many issues, it's good to start linking the issues with TODOs in the code when we'll start going more in maintenance mode. @jolevesq we should maybe start having a documentation on how to 'write' the syntax for the TODOs in the code.
* AppBar: fix to focus on correct button after panel closes * Panel: update with role "dialog" * Geolocator: add esc key handling (for closing) * Geolocator: add focus to search field on open * Export modal: auto focus on title text field on open * Export modal: return focus to appbar button on close * Notifications dialog: fix auto focus to close button on open * LayerList component: fix to use semantic HTML and aria properties * LayerList component: fix to allow items to be accessible via keyboard (enter and spacebar trigger LayerList items) * LayerList component: removed animated paper which does not appear to be in use (react spring) * Legend panel: improve use of semantic HTML and aria properties * Legend panel: fix to allow layer items to be accessible via keyboard (tab, enter, keyboard) * Legend panel: fix to keep focus on layer that has been toggled using keyboard * Details panel: show guide automatically when details panel opens and none of the layers have any features selected * Details panel: hide features prev/next buttons if only one feature is available for a layer * Updates to aria properties * Updates to use more semantic HTML * Updates to translations
kenchase
force-pushed
the
3116-keyboard-navigation
branch
from
1a82c60 to
942595b
Compare
kenchase
left a comment
kenchase
left a comment
There was a problem hiding this comment.
@kenchase made 3 comments and resolved 3 discussions.
Reviewable status: 20 of 22 files reviewed, 4 unresolved discussions (waiting on @Alex-NRCan and @jolevesq).
packages/geoview-core/src/core/components/export/export-modal.tsx line 184 at r2 (raw file):
Previously, Alex-NRCan (Alex) wrote…
I see the issue in GitHub, thank you, could you create 1 single TODO in the code and link the issue number in the TODO line?
Sorry, I know I'm asking for extra work here, but we have so many issues, it's good to start linking the issues with TODOs in the code when we'll start going more in maintenance mode. @jolevesq we should maybe start having a documentation on how to 'write' the syntax for the TODOs in the code.
Done.
packages/geoview-core/src/core/components/notifications/notifications.tsx line 130 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
There is 3221 for global ifd management. We will enforce class-name-mapid syntax... mapId at the end like you did
Will remove issue #3220
| // example "Layer Name 123!" becomes "layer-name-123-" | ||
| const sanitizeId = (str: string): string => { | ||
| return str | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9-_]/g, '-') | ||
| .replace(/-+/g, '-'); |
There was a problem hiding this comment.
Correction: This will be resolved as part of issue #3218
kenchase
left a comment
kenchase
left a comment
There was a problem hiding this comment.
@kenchase resolved 2 discussions.
Reviewable status: 20 of 22 files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan and @jolevesq).
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kenchase).
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @kenchase).
jolevesq
merged commit 5d6d25e
into
Canadian-Geospatial-Platform:develop
3 participants
Description
Incremental fixes to make keyboard navigation more intuitive and accessible
Fixes #3212
Fixes #3213
Fixes #3214
Type of change
How Has This Been Tested?
Tested manually by using keyboard navigation.
Add the URL for your deploy!
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