Skip to content

Commit

Permalink
[BatchUpload] Fix accessibility issues #part 1
Browse files Browse the repository at this point in the history
Screencast: https://screencast.googleplex.com/cast/NTEyMzkzMjg1NTIwNTg4OHxiZGQ4YWFkZC1kZA

Fixes in part #1 (this CL):
- Batch Upload entry point properly reads the title, subtitle and
the button content.
- Account icon is not navigable.
- Items icons are not navigable.
- Checkbox reads it's content and is not navigable.
- Toggle and Expand button reads the section title without the count
for clearer context.
- When unselecting last checkbox of a section, the focus goes back to
the section's toggle since it got disabled.

Fixes to be done in part #2 (follow CL):
- Make focusing the last checked checkbox announce text that mentions
that unchecking it will be disable the section.
- Announce the number of selected items after checking or unchecking
an item.
- Make the toggle say a more specific action: "Select all/none"

All changes in part #2 requires adding new strings.

Fixed: b:378720472, b:378718895, b:378711564
Bug: b:378746667
Change-Id: I98251bdd5d3adcbe1a7ed3e2a7a62c2327964b58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6032478
Reviewed-by: David Roger <[email protected]>
Code-Coverage: [email protected] <[email protected]>
Commit-Queue: Ryan Sultanem <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1385535}
  • Loading branch information
Ryan Sultanem authored and Chromium LUCI CQ committed Nov 20, 2024
1 parent e952b87 commit f85a34e
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,9 @@ <h2 class="page-title">$i18n{settings}</h2>
restamp>
<cr-link-row class="cr-row" non-clickable label="[[movePasswordsLabel_]]"
sub-label="$i18n{movePasswordsInSettingsSubLabel}" hide-icon>
<cr-button id="movePasswordsButton" on-click="onMovePasswordsClicked_">
$i18n{moveSinglePasswordButton}
<cr-button id="movePasswordsButton" on-click="onMovePasswordsClicked_"
aria-label="[[getAriaLabelMovePasswordsButton_(movePasswordsLabel_)]]">
$i18n{moveSinglePasswordButton}
</cr-button>
</cr-link-row>
</template>
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/resources/password_manager/settings_section.ts
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,14 @@ export class SettingsSectionElement extends SettingsSectionElementBase {
return this.isAccountStoreUser && this.passwordsOnDevice_.length > 0;
}

private getAriaLabelMovePasswordsButton_(): string {
return [
this.movePasswordsLabel_,
this.i18n('movePasswordsInSettingsSubLabel'),
this.i18n('moveSinglePasswordButton'),
].join('. ');
}

// This updates the local password count coming from the Sync Service API.
private async updateLocalPasswordCount_(localPasswordCount: number) {
this.localPasswordCount_ = localPasswordCount;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ export function getHtml(this: BatchUploadAppElement) {
<div id="subtitle">${this.dialogSubtitle_}</div>
<div id="account-info-row">
<img id="account-icon" alt="Account icon"
src="${this.accountInfo_.dataPictureUrl}">
<img id="account-icon" alt=""
src="${this.accountInfo_.dataPictureUrl}">
<div id="email">${this.accountInfo_.email}</div>
</div>
</div>
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/resources/signin/batch_upload/data_section.css
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@
.item-checkbox {
padding-inline-end: 12px;
width: 16px;
flex-grow: 0;
}

/* Visually hide the label but still allow the screen reader to pick it up. */
cr-checkbox::part(label-container) {
clip: rect(0,0,0,0);
display: block;
position: fixed;
}

.item-icon-container {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@ export function getHtml(this: DataSectionElement) {
<cr-expand-button id="expandButton" no-hover
?hidden="${this.disabled_}"
?expanded="${this.expanded_}"
@expanded-changed="${this.onExpandChanged_}">
@expanded-changed="${this.onExpandChanged_}"
aria-label="${this.titleWithoutCount_}">
</cr-expand-button>
<div id="separator" ?hidden="${this.disabled_}"></div>
<cr-toggle id="toggle"
@checked-changed=${this.onToggleChanged_}
?checked="${!this.disabled_}">
?checked="${!this.disabled_}"
aria-label="${this.titleWithoutCount_}">
</cr-toggle>
</div>
<cr-collapse id="collapse" .opened="${this.expanded_}">
Expand All @@ -32,18 +34,19 @@ export function getHtml(this: DataSectionElement) {
data-id="${item.id}"
?checked="${this.isCheckboxChecked_(item.id)}"
@change="${this.onCheckedChanged_}">
${item.title}, ${item.subtitle}
</cr-checkbox>
<div class="data-item-content">
<div class="item-icon-container"
?hidden="${this.isStrEmpty_(item.iconUrl)}">
<img class="item-icon"
alt="Item icon" src="${this.getFaviconUrl_(item.iconUrl)}">
<img class="item-icon" alt=""
src="${this.getFaviconUrl_(item.iconUrl)}">
</div>
<div class="item-info">
<div class="item-title text-elide">
<div class="item-title text-elide" aria-hidden="true">
${item.title}
</div>
<div class="item-subtitle text-elide">
<div class="item-subtitle text-elide" aria-hidden="true">
${item.subtitle}
</div>
</div>
Expand Down
17 changes: 15 additions & 2 deletions chrome/browser/resources/signin/batch_upload/data_section.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export class DataSectionElement extends CrLitElement {
return {
dataContainer: {type: Object},
title_: {type: String},
titleWithoutCount_: {type: String},
expanded_: {type: Boolean},
disabled_: {type: Boolean},
dataSelectedCount_: {type: Number},
Expand All @@ -80,6 +81,8 @@ export class DataSectionElement extends CrLitElement {
// Title of the section, updated on each item checkbox selection based on the
// number of selected items.
protected title_: string = '';
// Computed once on page load as it does not contain the selected item count.
protected titleWithoutCount_: string = '';

// If the collapse section is exapnded.
protected expanded_: boolean = false;
Expand All @@ -98,10 +101,18 @@ export class DataSectionElement extends CrLitElement {
private intervalDurationOfUpdateHeightRequests_: number|null = null;
private collapseAnimationDuration_: number = 0;

override connectedCallback() {
override async connectedCallback() {
super.connectedCallback();

this.initializeSectionOutput_();

// In tests this id may be empty.
if (this.dataContainer.sectionTitle &&
this.dataContainer.sectionTitle.length > 0) {
this.titleWithoutCount_ =
await PluralStringProxyImpl.getInstance().getPluralString(
this.dataContainer.sectionTitle, 0);
}
}

override firstUpdated() {
Expand Down Expand Up @@ -227,9 +238,11 @@ export class DataSectionElement extends CrLitElement {
// Checkbox off.
this.dataSelected.delete(itemId);
this.dataSelectedCount_ = this.dataSelected.size;
// If this is the last item unchecked then disable and reset the section.
// If this is the last item unchecked then disable and reset the section
// and focus the toggle since its value changed indirectly.
if (this.dataSelectedCount_ === 0) {
this.resetWithState_(/*disabled=*/ true);
this.$.toggle.focus();
}
}

Expand Down

0 comments on commit f85a34e

Please sign in to comment.