Skip to content

Commit

Permalink
shortcuts: Add Edit-button for editable accelerator.
Browse files Browse the repository at this point in the history
* Add edit icon at the end of accelerator-row if the accelerator is
  editable and clicking edit-icon will open the edit-dialog.
* Update and add unit tests for this behavior.
* video: http://go/scrcast/NTU3NDIyNzEyNzE3MzEyMHxmMGM1ZTFhMy02OQ

Bug: b/216049298
Test: browser_tests --gtest_filter=ShortcutCustomizationApp*
Change-Id: Id4eada5cf30979c755cf1af926d632fc6294b840
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4539941
Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Commit-Queue: Longbo Wei <longbowei@google.com>
Cr-Commit-Position: refs/heads/main@{#1146739}
  • Loading branch information
Longbo Wei authored and Chromium LUCI CQ committed May 19, 2023
1 parent 9b350ca commit 5a84d20
Show file tree
Hide file tree
Showing 7 changed files with 178 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
grid-template-columns: minmax(min-content, 286px) 50%;
}

#container:hover accelerator-view::part(edit-icon) {
opacity: 1;
}

accelerator-view,
text-accelerator {
line-height: 3.2;
Expand Down Expand Up @@ -53,7 +57,8 @@
<template is="dom-if" if="[[isDefaultLayout(layoutStyle)]]">
<template is="dom-repeat" items="[[getFilteredAccelerators(acceleratorInfos)]]">
<accelerator-view class="accelerator-item" accelerator-info="[[item]]"
action="[[action]]" source="[[source]]" source-is-locked="[[isLocked]]">
action="[[action]]" source="[[source]]" source-is-locked="[[isLocked]]"
show-edit-icon="true">
</accelerator-view>
</template>
<div id="noShortcutAssigned" hidden="[[!isEmptyList(acceleratorInfos)]]">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class AcceleratorRowElement extends AcceleratorRowElementBase {
override disconnectedCallback(): void {
super.disconnectedCallback();
if (!this.isLocked) {
this.removeEventListener('click', () => this.showDialog());
this.removeEventListener('edit-icon-clicked', () => this.showDialog());
}
}

Expand All @@ -100,7 +100,7 @@ export class AcceleratorRowElement extends AcceleratorRowElementBase {
.then(({isMutable}) => {
this.isLocked = !isMutable;
if (!this.isLocked) {
this.addEventListener('click', () => this.showDialog());
this.addEventListener('edit-icon-clicked', () => this.showDialog());
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,29 @@
padding-inline-end: 5px;
}


iron-icon[icon='shortcut-customization:lock'] {
--iron-icon-height: 16px;
--iron-icon-width: 16px;
}

#editIconContainer {
align-items: center;
display: flex;
margin-inline-start: 8px;
opacity: 0;
transition: opacity 300ms, transform 100ms;
}

#editIconContainer[hidden] {
display: none; /* Required for flexbox hidden. */
}

#editButton {
--cr-icon-button-icon-size: 16px;
--cr-icon-button-fill-color: var(--cros-text-color-secondary);
--cr-icon-button-icon-start-offset: 3px;
padding-top: 3px;
}
</style>

<div id="container" class="flex-row" tabindex$="[[getTabIndex()]]">
Expand All @@ -40,9 +57,15 @@
</input-key>
</div>
<div class="lock-icon-container"
hidden="[[!shouldShowLockIcon(acceleratorInfo.locked, sourceIsLocked)]]">
hidden="[[!shouldShowLockIcon(acceleratorInfo.locked, sourceIsLocked)]]">
<iron-icon icon="shortcut-customization:lock"></iron-icon>
</div>
<div id="editIconContainer" part="edit-icon" on-click="onEditIconClicked"
hidden="[[!shouldShowEditIcon(acceleratorInfo.locked, sourceIsLocked)]]">
<cr-icon-button id="editButton" class="clickable-button"
iron-icon="shortcut-customization:edit">
</cr-icon-button>
</div>
</template>
<template is="dom-if" if="[[showEditView(viewState)]]">
<div id="editContainer" class="flex-row">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ export class AcceleratorViewElement extends AcceleratorViewElementBase {
type: Boolean,
value: false,
},

/**
* Conditionally show the edit-icon-container in `accelerator-view`, true
* for `accelerator-row`, false for `accelerator-edit-view`.
*/
showEditIcon: {
type: Boolean,
value: false,
},
};
}

Expand All @@ -120,6 +129,7 @@ export class AcceleratorViewElement extends AcceleratorViewElementBase {
action: number;
source: AcceleratorSource;
sourceIsLocked: boolean;
showEditIcon: boolean;
protected pendingAcceleratorInfo: StandardAcceleratorInfo;
private modifiers: string[];
private acceleratorOnHold: string;
Expand Down Expand Up @@ -451,6 +461,24 @@ export class AcceleratorViewElement extends AcceleratorViewElementBase {
this.sourceIsLocked;
}

private shouldShowEditIcon(): boolean {
// Do not show edit icon in each row if customization is disabled, category
// is locked, or the row is displayed in edit-dialog.
if (isCustomizationDisabled() || !this.showEditIcon ||
isCategoryLocked(this.lookupManager.getAcceleratorCategory(
this.source, this.action))) {
return false;
}

return !(this.acceleratorInfo && this.acceleratorInfo.locked) &&
!this.sourceIsLocked;
}

private onEditIconClicked(): void {
this.dispatchEvent(
new CustomEvent('edit-icon-clicked', {bubbles: true, composed: true}));
}

/**
* Determines whether accelerator items should be tab-focusable.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,21 @@ suite('acceleratorRowTest', function() {

await flushTasks();

const rowContainer =
rowElement.shadowRoot!.querySelector('#container') as HTMLDivElement;
rowContainer.click();
const acceleratorViewElement =
rowElement!.shadowRoot!.querySelectorAll('accelerator-view');
assertEquals(1, acceleratorViewElement.length);
const editIconContainerElement = strictQuery(
'#editIconContainer', acceleratorViewElement[0]!.shadowRoot,
HTMLDivElement);

editIconContainerElement.click();

await flushTasks();

assertTrue(showDialogListenerCalled);
});

test('DontShowDialogOnClickWhenCustomizationDisabled', async () => {
test('EditIconHiddenWhenCustomizationDisabled', async () => {
loadTimeData.overrideValues({isCustomizationEnabled: false});
rowElement = initAcceleratorRowElement();
waitAfterNextRender(rowElement);
Expand All @@ -154,50 +159,18 @@ suite('acceleratorRowTest', function() {
rowElement.action = 0;
rowElement.layoutStyle = LayoutStyle.kDefault;

let showDialogListenerCalled = false;
rowElement.addEventListener('show-edit-dialog', () => {
showDialogListenerCalled = true;
});

await flushTasks();

const rowContainer =
rowElement.shadowRoot!.querySelector('#container') as HTMLDivElement;
rowContainer.click();

await flushTasks();

assertFalse(showDialogListenerCalled);
});

test('DontShowDialogForTextAccelerators', async () => {
loadTimeData.overrideValues({isCustomizationEnabled: true});
rowElement = initAcceleratorRowElement();
waitAfterNextRender(rowElement);
const accelerators = [createTextAcceleratorInfo([{
text: stringToMojoString16('ctrl'),
type: TextAcceleratorPartType.kModifier,
}])];

rowElement.acceleratorInfos = accelerators;
rowElement.source = AcceleratorSource.kAmbient;
rowElement.action = 0;
rowElement.layoutStyle = LayoutStyle.kText;

let showDialogListenerCalled = false;
rowElement.addEventListener('show-edit-dialog', () => {
showDialogListenerCalled = true;
});

await flushTasks();
const acceleratorViewElement =
rowElement!.shadowRoot!.querySelectorAll('accelerator-view');
assertEquals(1, acceleratorViewElement.length);

const rowContainer =
rowElement.shadowRoot!.querySelector('#container') as HTMLDivElement;
rowContainer.click();
const editIconContainerElement = strictQuery(
'#editIconContainer', acceleratorViewElement[0]!.shadowRoot,
HTMLDivElement);

await flushTasks();

assertFalse(showDialogListenerCalled);
assertFalse(isVisible(editIconContainerElement));
});

test('ShowTextAccelerator', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ suite('acceleratorViewTest', function() {
'.lock-icon-container', viewElement!.shadowRoot, HTMLDivElement);
}

function getEditIcon(): HTMLDivElement {
return strictQuery(
'#editIconContainer', viewElement!.shadowRoot, HTMLDivElement);
}

test('LoadsBasicAccelerator', async () => {
viewElement = initAcceleratorViewElement();
await flushTasks();
Expand Down Expand Up @@ -302,4 +307,82 @@ suite('acceleratorViewTest', function() {
}
});

});
test('EditIconVisibilityBasedOnProperties', async () => {
// Mainly test on customizationEnabled and accelerator is not locked.
const scenarios = [
{
customizationEnabled: true,
locked: false,
sourceIsLocked: false,
isAcceleratorRow: false,
},
{
customizationEnabled: true,
locked: false,
sourceIsLocked: false,
isAcceleratorRow: true,
},
{
customizationEnabled: true,
locked: true,
sourceIsLocked: false,
isAcceleratorRow: false,
},
{
customizationEnabled: true,
locked: false,
sourceIsLocked: true,
isAcceleratorRow: true,
},
{
customizationEnabled: false,
locked: false,
sourceIsLocked: false,
isAcceleratorRow: false,
},
];

// Prepare all test cases by looping the fakeLayoutInfo.
const testCases = [];
for (const layoutInfo of fakeLayoutInfo) {
// If it's text accelerator, break the loop early.
if (layoutInfo.style !== LayoutStyle.kDefault) {
continue;
}
for (const scenario of scenarios) {
// replicate getCategory() logic.
const category = manager!.getAcceleratorCategory(
layoutInfo.source, layoutInfo.action);
// replicate shouldShowLockIcon() logic.
const expectEditIconVisible = scenario.customizationEnabled &&
scenario.isAcceleratorRow && !isCategoryLocked(category) &&
!scenario.locked && !scenario.sourceIsLocked;
testCases.push({
...scenario,
layoutInfo: layoutInfo,
expectEditIconVisible: expectEditIconVisible,
});
}
}
for (const testCase of testCases) {
loadTimeData.overrideValues(
{isCustomizationEnabled: testCase.customizationEnabled});
viewElement = initAcceleratorViewElement();
viewElement.source = testCase.layoutInfo.source;
viewElement.action = testCase.layoutInfo.action;
viewElement.showEditIcon = testCase.isAcceleratorRow;
const acceleratorInfo = createStandardAcceleratorInfo(
Modifier.CONTROL | Modifier.SHIFT,
/*key=*/ 71,
/*keyDisplay=*/ 'g');
viewElement.acceleratorInfo = acceleratorInfo;
viewElement.set('acceleratorInfo.locked', testCase.locked);
viewElement.sourceIsLocked = testCase.sourceIsLocked;

await flush();
assertEquals(
testCase.expectEditIconVisible,
!getEditIcon().hasAttribute('hidden'));
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,13 @@ suite('shortcutCustomizationAppTest', function() {
subSections[subsectionIndex]!.shadowRoot!.querySelectorAll(
'accelerator-row') as NodeListOf<AcceleratorRowElement>;

// Click on the first accelerator, expect the edit dialog to open.
accelerators[0]!.click();
// Click on the first accelerator's edit icon, expect the edit dialog to
// open.
const acceleratorView =
accelerators[0]!.shadowRoot!.querySelectorAll('accelerator-view');
const editIconContainer = acceleratorView[0]!.shadowRoot!.querySelector(
'#editIconContainer') as HTMLDivElement;
editIconContainer.click();
await flushTasks();
}

Expand Down Expand Up @@ -246,8 +251,16 @@ suite('shortcutCustomizationAppTest', function() {
subSections[0]!.shadowRoot!.querySelectorAll('accelerator-row');
// Only three accelerators rows for this subsection.
assertEquals(3, accelerators.length);
// Click on the first accelerator, expect the edit dialog to open.
accelerators[0]!.click();

// Click on the first accelerator's edit button, expect the edit dialog to
// open.
const acceleratorView =
accelerators[0]!.shadowRoot!.querySelectorAll('accelerator-view');
assertEquals(1, acceleratorView.length);
const editIconContainer = acceleratorView[0]!.shadowRoot!.querySelector(
'#editIconContainer') as HTMLDivElement;
editIconContainer.click();

await flushTasks();
editDialog = getPage().shadowRoot!.querySelector('#editDialog');
assertTrue(!!editDialog);
Expand Down

0 comments on commit 5a84d20

Please sign in to comment.