Skip to content

Commit 436c0c0

Browse files
jackfranklinDevtools-frontend LUCI CQ
jackfranklin
authored and
Devtools-frontend LUCI CQ
committed
RPP: do not aria announce if the user triggers settings
This CL fixes a bug where if the user changed a setting (such as "Show custom tracks") that triggered a redraw, we would re-announce any annotations, despite the fact that they do not change and always remained on screen. The diff ended up being relatively large for this change; mostly because during my testing I also found a bug where we do not clear up event listeners on the ModificationsManager when the trace is not visible. To fix this I moved the event handler into a distinct bound method, and then we can remove the listener in the `#changeView` method. This CL also removes the debouncer for the Aria Utils, I do not remember why it was added, but in my testing today when creating a lot of annotations and navigating around the panel, I could not recreate the scenario where we spam the user. [email protected] Bug: none Change-Id: I99212373b17a2d26c5da27021b03a35ff39a6f88 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6888592 Reviewed-by: Nikolay Vitkov <[email protected]> Auto-Submit: Jack Franklin <[email protected]> Commit-Queue: Jack Franklin <[email protected]>
1 parent 90dda5c commit 436c0c0

File tree

9 files changed

+279
-219
lines changed

9 files changed

+279
-219
lines changed

‎front_end/panels/timeline/AnnotationHelpers.test.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,13 @@ describe('AnnotationHelpers', () => {
138138
assert.strictEqual(text, 'The entry label annotation has been added');
139139
});
140140

141+
it('does not return text if the aria notifcations are muted', async () => {
142+
const overlay: Trace.Types.Overlays.EntryLabel = {type: 'ENTRY_LABEL', entry: FAKE_ENTRY_1, label: 'Hello world'};
143+
const event = new Timeline.ModificationsManager.AnnotationModifiedEvent(overlay, 'Add', true);
144+
const text = ariaAnnouncementForModifiedEvent(event);
145+
assert.isNull(text);
146+
});
147+
141148
it('does not return an announcement for new empty labels', async () => {
142149
const overlay: Trace.Types.Overlays.EntryLabel = {type: 'ENTRY_LABEL', entry: FAKE_ENTRY_1, label: ''};
143150
const event = new Timeline.ModificationsManager.AnnotationModifiedEvent(overlay, 'Add');

‎front_end/panels/timeline/AnnotationHelpers.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,9 @@ export function ariaDescriptionForOverlay(overlay: Trace.Types.Overlays.Overlay)
172172
}
173173

174174
export function ariaAnnouncementForModifiedEvent(event: AnnotationModifiedEvent): string|null {
175+
if (event.muteAriaNotifications) {
176+
return null;
177+
}
175178
const {overlay, action} = event;
176179
switch (action) {
177180
case 'Remove': {

‎front_end/panels/timeline/ModificationsManager.test.ts‎

Lines changed: 85 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -70,28 +70,34 @@ describeWithEnvironment('ModificationsManager', () => {
7070
const modificationsManager = Timeline.ModificationsManager.ModificationsManager.activeManager();
7171
assert.isOk(modificationsManager);
7272

73-
modificationsManager.createAnnotation({
74-
type: 'ENTRY_LABEL',
75-
entry,
76-
label: 'entry label',
77-
});
78-
79-
modificationsManager.createAnnotation({
80-
type: 'ENTRIES_LINK',
81-
state: Trace.Types.File.EntriesLinkState.CONNECTED,
82-
entryFrom: entry,
83-
entryTo: entry2,
84-
});
85-
86-
modificationsManager.createAnnotation({
87-
type: 'TIME_RANGE',
88-
bounds: {
89-
min: Trace.Types.Timing.Micro(0),
90-
max: Trace.Types.Timing.Micro(10),
91-
range: Trace.Types.Timing.Micro(10),
92-
},
93-
label: 'range label',
94-
});
73+
modificationsManager.createAnnotation(
74+
{
75+
type: 'ENTRY_LABEL',
76+
entry,
77+
label: 'entry label',
78+
},
79+
{loadedFromFile: false, muteAriaNotifications: false});
80+
81+
modificationsManager.createAnnotation(
82+
{
83+
type: 'ENTRIES_LINK',
84+
state: Trace.Types.File.EntriesLinkState.CONNECTED,
85+
entryFrom: entry,
86+
entryTo: entry2,
87+
},
88+
{loadedFromFile: false, muteAriaNotifications: false});
89+
90+
modificationsManager.createAnnotation(
91+
{
92+
type: 'TIME_RANGE',
93+
bounds: {
94+
min: Trace.Types.Timing.Micro(0),
95+
max: Trace.Types.Timing.Micro(10),
96+
range: Trace.Types.Timing.Micro(10),
97+
},
98+
label: 'range label',
99+
},
100+
{loadedFromFile: false, muteAriaNotifications: false});
95101

96102
const modifications = modificationsManager.toJSON().annotations;
97103
assert.deepEqual(modifications, {
@@ -124,18 +130,22 @@ describeWithEnvironment('ModificationsManager', () => {
124130
const modificationsManager = Timeline.ModificationsManager.ModificationsManager.activeManager();
125131
assert.isOk(modificationsManager);
126132

127-
modificationsManager.createAnnotation({
128-
type: 'ENTRIES_LINK',
129-
state: Trace.Types.File.EntriesLinkState.CONNECTED,
130-
entryFrom: entry,
131-
entryTo: entry2,
132-
});
133-
134-
modificationsManager.createAnnotation({
135-
type: 'ENTRIES_LINK',
136-
state: Trace.Types.File.EntriesLinkState.PENDING_TO_EVENT,
137-
entryFrom: entry2,
138-
});
133+
modificationsManager.createAnnotation(
134+
{
135+
type: 'ENTRIES_LINK',
136+
state: Trace.Types.File.EntriesLinkState.CONNECTED,
137+
entryFrom: entry,
138+
entryTo: entry2,
139+
},
140+
{loadedFromFile: false, muteAriaNotifications: false});
141+
142+
modificationsManager.createAnnotation(
143+
{
144+
type: 'ENTRIES_LINK',
145+
state: Trace.Types.File.EntriesLinkState.PENDING_TO_EVENT,
146+
entryFrom: entry2,
147+
},
148+
{loadedFromFile: false, muteAriaNotifications: false});
139149

140150
// Make sure only the link with both 'to' and 'from' entries in in the generated JSON
141151
const modifications = modificationsManager.toJSON().annotations;
@@ -160,12 +170,14 @@ describeWithEnvironment('ModificationsManager', () => {
160170
assert.isOk(modificationsManager);
161171

162172
// Create a connection between entry 1 and entry 2
163-
modificationsManager.createAnnotation({
164-
type: 'ENTRIES_LINK',
165-
state: Trace.Types.File.EntriesLinkState.CONNECTED,
166-
entryFrom: entry1,
167-
entryTo: entry2,
168-
});
173+
modificationsManager.createAnnotation(
174+
{
175+
type: 'ENTRIES_LINK',
176+
state: Trace.Types.File.EntriesLinkState.CONNECTED,
177+
entryFrom: entry1,
178+
entryTo: entry2,
179+
},
180+
{loadedFromFile: false, muteAriaNotifications: false});
169181

170182
// Chech if a connection between entries 1 and 3 exists
171183
const existsBetween1And3 = modificationsManager.linkAnnotationBetweenEntriesExists(entry1, entry3);
@@ -188,37 +200,43 @@ describeWithEnvironment('ModificationsManager', () => {
188200
const modificationsManager = Timeline.ModificationsManager.ModificationsManager.activeManager();
189201
assert.isOk(modificationsManager);
190202

191-
modificationsManager.createAnnotation({
192-
type: 'TIME_RANGE',
193-
bounds: {
194-
min: Trace.Types.Timing.Micro(0),
195-
max: Trace.Types.Timing.Micro(10),
196-
range: Trace.Types.Timing.Micro(10),
197-
},
198-
label: 'label',
199-
});
203+
modificationsManager.createAnnotation(
204+
{
205+
type: 'TIME_RANGE',
206+
bounds: {
207+
min: Trace.Types.Timing.Micro(0),
208+
max: Trace.Types.Timing.Micro(10),
209+
range: Trace.Types.Timing.Micro(10),
210+
},
211+
label: 'label',
212+
},
213+
{loadedFromFile: false, muteAriaNotifications: false});
200214

201215
// Create time range with empty label that shoud be removed
202-
modificationsManager.createAnnotation({
203-
type: 'TIME_RANGE',
204-
bounds: {
205-
min: Trace.Types.Timing.Micro(3),
206-
max: Trace.Types.Timing.Micro(10),
207-
range: Trace.Types.Timing.Micro(7),
208-
},
209-
label: '',
210-
});
216+
modificationsManager.createAnnotation(
217+
{
218+
type: 'TIME_RANGE',
219+
bounds: {
220+
min: Trace.Types.Timing.Micro(3),
221+
max: Trace.Types.Timing.Micro(10),
222+
range: Trace.Types.Timing.Micro(7),
223+
},
224+
label: '',
225+
},
226+
{loadedFromFile: false, muteAriaNotifications: false});
211227

212228
// Create time range with empty label that shoud be removed
213-
modificationsManager.createAnnotation({
214-
type: 'TIME_RANGE',
215-
bounds: {
216-
min: Trace.Types.Timing.Micro(5),
217-
max: Trace.Types.Timing.Micro(10),
218-
range: Trace.Types.Timing.Micro(5),
219-
},
220-
label: '',
221-
});
229+
modificationsManager.createAnnotation(
230+
{
231+
type: 'TIME_RANGE',
232+
bounds: {
233+
min: Trace.Types.Timing.Micro(5),
234+
max: Trace.Types.Timing.Micro(10),
235+
range: Trace.Types.Timing.Micro(5),
236+
},
237+
label: '',
238+
},
239+
{loadedFromFile: false, muteAriaNotifications: false});
222240

223241
modificationsManager.deleteEmptyRangeAnnotations();
224242
const modifications = modificationsManager.toJSON().annotations;

‎front_end/panels/timeline/ModificationsManager.ts‎

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ export type UpdateAction =
2323
export class AnnotationModifiedEvent extends Event {
2424
static readonly eventName = 'annotationmodifiedevent';
2525

26-
constructor(public overlay: Trace.Types.Overlays.Overlay, public action: UpdateAction) {
26+
constructor(
27+
public overlay: Trace.Types.Overlays.Overlay, public action: UpdateAction, public muteAriaNotifications = false) {
2728
super(AnnotationModifiedEvent.eventName);
2829
}
2930
}
@@ -137,7 +138,10 @@ export class ModificationsManager extends EventTarget {
137138
* Stores the annotation and creates its overlay.
138139
* @returns the Overlay that gets created and associated with this annotation.
139140
*/
140-
createAnnotation(newAnnotation: Trace.Types.File.Annotation, loadedFromFile = false): Trace.Types.Overlays.Overlay {
141+
createAnnotation(newAnnotation: Trace.Types.File.Annotation, opts: {
142+
loadedFromFile: boolean,
143+
muteAriaNotifications: boolean,
144+
}): Trace.Types.Overlays.Overlay {
141145
// If a label already exists on an entry and a user is trying to create a new one, start editing an existing label instead.
142146
if (newAnnotation.type === 'ENTRY_LABEL') {
143147
const overlay = this.#findLabelOverlayForEntry(newAnnotation.entry);
@@ -149,7 +153,7 @@ export class ModificationsManager extends EventTarget {
149153

150154
// If the new annotation created was not loaded from the file, set the annotations visibility setting to true. That way we make sure
151155
// the annotations are on when a new one is created.
152-
if (!loadedFromFile) {
156+
if (!opts.loadedFromFile) {
153157
// Time range annotation could also be used to check the length of a selection in the timeline. Therefore, only set the annotations
154158
// hidden to true if annotations label is added. This is done in OverlaysImpl.
155159
if (newAnnotation.type !== 'TIME_RANGE') {
@@ -158,7 +162,7 @@ export class ModificationsManager extends EventTarget {
158162
}
159163
const newOverlay = this.#createOverlayFromAnnotation(newAnnotation);
160164
this.#overlayForAnnotation.set(newAnnotation, newOverlay);
161-
this.dispatchEvent(new AnnotationModifiedEvent(newOverlay, 'Add'));
165+
this.dispatchEvent(new AnnotationModifiedEvent(newOverlay, 'Add', opts.muteAriaNotifications));
162166
return newOverlay;
163167
}
164168

@@ -300,12 +304,12 @@ export class ModificationsManager extends EventTarget {
300304
return [...this.#overlayForAnnotation.values()];
301305
}
302306

303-
applyAnnotationsFromCache(): void {
307+
applyAnnotationsFromCache(opts: {muteAriaNotifications: boolean}): void {
304308
this.#modifications = this.toJSON();
305309
// The cache is filled by applyModificationsIfPresent, so we clear
306310
// it beforehand to prevent duplicate entries.
307311
this.#overlayForAnnotation.clear();
308-
this.#applyStoredAnnotations(this.#modifications.annotations);
312+
this.#applyStoredAnnotations(this.#modifications.annotations, opts);
309313
}
310314

311315
/**
@@ -383,10 +387,13 @@ export class ModificationsManager extends EventTarget {
383387

384388
this.#timelineBreadcrumbs.setInitialBreadcrumbFromLoadedModifications(this.#modifications.initialBreadcrumb);
385389
this.#applyEntriesFilterModifications(hiddenEntries, expandableEntries);
386-
this.#applyStoredAnnotations(this.#modifications.annotations);
390+
this.#applyStoredAnnotations(this.#modifications.annotations, {
391+
muteAriaNotifications: false,
392+
});
387393
}
388394

389-
#applyStoredAnnotations(annotations: Trace.Types.File.SerializedAnnotations): void {
395+
#applyStoredAnnotations(annotations: Trace.Types.File.SerializedAnnotations, opts: {muteAriaNotifications: boolean}):
396+
void {
390397
try {
391398
// Assign annotations to an empty array if they don't exist to not
392399
// break the traces that were saved before those annotations were implemented
@@ -398,7 +405,10 @@ export class ModificationsManager extends EventTarget {
398405
entry: this.#eventsSerializer.eventForKey(entryLabel.entry, this.#parsedTrace),
399406
label: entryLabel.label,
400407
},
401-
true);
408+
{
409+
loadedFromFile: true,
410+
muteAriaNotifications: opts.muteAriaNotifications,
411+
});
402412
});
403413

404414
const timeRanges = annotations.labelledTimeRanges ?? [];
@@ -409,7 +419,10 @@ export class ModificationsManager extends EventTarget {
409419
bounds: timeRange.bounds,
410420
label: timeRange.label,
411421
},
412-
true);
422+
{
423+
loadedFromFile: true,
424+
muteAriaNotifications: opts.muteAriaNotifications,
425+
});
413426
});
414427

415428
const linksBetweenEntries = annotations.linksBetweenEntries ?? [];
@@ -421,7 +434,10 @@ export class ModificationsManager extends EventTarget {
421434
entryFrom: this.#eventsSerializer.eventForKey(linkBetweenEntries.entryFrom, this.#parsedTrace),
422435
entryTo: this.#eventsSerializer.eventForKey(linkBetweenEntries.entryTo, this.#parsedTrace),
423436
},
424-
true);
437+
{
438+
loadedFromFile: true,
439+
muteAriaNotifications: opts.muteAriaNotifications,
440+
});
425441
});
426442
} catch (err) {
427443
// This function is wrapped in a try/catch just in case we get any incoming

‎front_end/panels/timeline/TimelineFlameChartView.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,10 @@ describeWithEnvironment('TimelineFlameChartView', function() {
328328
const labelAnnotation = modifications.getAnnotations().find(a => a.type === 'ENTRY_LABEL');
329329
assert.isOk(labelAnnotation);
330330
// This creates an active annotation in the UI and creates the overlay.
331-
const overlay = modifications.createAnnotation(labelAnnotation);
331+
const overlay = modifications.createAnnotation(labelAnnotation, {
332+
loadedFromFile: false,
333+
muteAriaNotifications: false,
334+
});
332335
flameChartView.addOverlay(overlay);
333336
await raf();
334337
const overlayElement = flameChartView.overlays().elementForOverlay(overlay);

0 commit comments

Comments
 (0)