Added support for navigating across files in multi-file diff editor and info for narrator #274466
Conversation
…nd narrator information for accessibility
There was a problem hiding this comment.
Pull Request Overview
This PR adds navigation commands to the multi-diff editor, allowing users to navigate between files using "Go to Next File" and "Go to Previous File" actions. It also improves accessibility by adding ARIA labels and keyboard navigation support.
Key changes:
- Added
goToNextFile()andgoToPreviousFile()navigation methods throughout the multi-diff editor architecture - Introduced accessibility improvements with ARIA labels and keyboard focus management for file containers
- Registered new navigation actions in the command palette
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/contrib/multiDiffEditor/browser/multiDiffEditor.ts |
Added public navigation methods goToNextFile() and goToPreviousFile() to the MultiDiffEditor class |
src/vs/workbench/contrib/multiDiffEditor/browser/multiDiffEditor.contribution.ts |
Imported and registered the new GoToNextFileAction and GoToPreviousFileAction commands |
src/vs/workbench/contrib/multiDiffEditor/browser/actions.ts |
Defined new action classes for navigating to next/previous files with command palette integration |
src/vs/editor/browser/widget/multiDiffEditor/multiDiffEditorWidgetImpl.ts |
Implemented navigation logic with scrolling and focus management, added ARIA label to main editor container |
src/vs/editor/browser/widget/multiDiffEditor/multiDiffEditorWidget.ts |
Added delegation methods to call the widget implementation's navigation functions |
src/vs/editor/browser/widget/multiDiffEditor/diffEditorItemTemplate.ts |
Added localize import, ARIA labels for individual file containers, and getContainerElement() method for focus management |
| fileAriaLabel = localize('modifiedFileHeader', '{0} file {1}', status, modifiedFileName); | ||
| } else { | ||
| fileAriaLabel = localize('modifiedFileHeader', '{0} file {1}', status, originalFileName); |
There was a problem hiding this comment.
The localization key 'modifiedFileHeader' is used for both 'Added' and 'Modified'/'Deleted' status messages (lines 276 and 278). Consider using a more generic key name like 'fileHeader' since it's not exclusively for modified files, or use distinct keys for each status type to allow for more context-specific translations.
| fileAriaLabel = localize('modifiedFileHeader', '{0} file {1}', status, modifiedFileName); | |
| } else { | |
| fileAriaLabel = localize('modifiedFileHeader', '{0} file {1}', status, originalFileName); | |
| fileAriaLabel = localize('addedFileHeader', 'Added file {0}', modifiedFileName); | |
| } else if (isDeleted) { | |
| fileAriaLabel = localize('deletedFileHeader', 'Deleted file {0}', originalFileName); | |
| } else { | |
| fileAriaLabel = localize('modifiedFileHeader', 'Modified file {0}', originalFileName); |
| const currentIndex = activeItem ? items.indexOf(activeItem) : -1; | ||
| const nextIndex = Math.min(currentIndex + 1, items.length - 1); | ||
|
|
||
| if (nextIndex !== currentIndex && nextIndex >= 0) { |
There was a problem hiding this comment.
The check nextIndex >= 0 on line 363 is redundant. Since currentIndex can be at minimum -1, and nextIndex = currentIndex + 1, the minimum value of nextIndex is 0. The condition nextIndex !== currentIndex is sufficient to ensure we're not already at the last item.
| if (nextIndex !== currentIndex && nextIndex >= 0) { | |
| if (nextIndex !== currentIndex) { |
| const currentIndex = activeItem ? items.indexOf(activeItem) : -1; | ||
| const prevIndex = Math.max(currentIndex - 1, 0); | ||
|
|
||
| if (prevIndex !== currentIndex && prevIndex >= 0) { |
There was a problem hiding this comment.
The check prevIndex >= 0 on line 382 is redundant. Since prevIndex = Math.max(currentIndex - 1, 0), it's guaranteed to be at least 0. The condition prevIndex !== currentIndex is sufficient to ensure we're not already at the first item.
| if (prevIndex !== currentIndex && prevIndex >= 0) { | |
| if (prevIndex !== currentIndex) { |
| this._elements.status.innerText = flag; | ||
|
|
||
| // Set aria-label for the individual file container | ||
| const originalFileName = data.viewModel.originalUri?.path.split('/').pop(); |
| } | ||
| } | ||
|
|
||
| private scrollToItem(index: number): void { |
There was a problem hiding this comment.
any reason we don't want to use MultiDiffEditorWidgetImpl#reveal?
Added actions for navigating across files in multi-file diff editor and added aria-label for information on file modified/added/deleted. Addresses issue: #264299
Below gif shows the changes in action.