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.

R=ergunsh@chromium.org

Bug: none
Change-Id: I99212373b17a2d26c5da27021b03a35ff39a6f88
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6888592
Reviewed-by: Nikolay Vitkov <nvitkov@chromium.org>
Auto-Submit: Jack Franklin <jacktfranklin@chromium.org>
Commit-Queue: Jack Franklin <jacktfranklin@chromium.org>

Author: jackfranklin

front_end/panels/timeline/AnnotationHelpers.test.ts

@@ -138,6 +138,13 @@ describe('AnnotationHelpers', () => {
       assert.strictEqual(text, 'The entry label annotation has been added');
     });
 
+    it('does not return text if the aria notifcations are muted', async () => {
+      const overlay: Trace.Types.Overlays.EntryLabel = {type: 'ENTRY_LABEL', entry: FAKE_ENTRY_1, label: 'Hello world'};
+      const event = new Timeline.ModificationsManager.AnnotationModifiedEvent(overlay, 'Add', true);
+      const text = ariaAnnouncementForModifiedEvent(event);
+      assert.isNull(text);
+    });
+
     it('does not return an announcement for new empty labels', async () => {
       const overlay: Trace.Types.Overlays.EntryLabel = {type: 'ENTRY_LABEL', entry: FAKE_ENTRY_1, label: ''};
       const event = new Timeline.ModificationsManager.AnnotationModifiedEvent(overlay, 'Add');

front_end/panels/timeline/AnnotationHelpers.ts

@@ -172,6 +172,9 @@ export function ariaDescriptionForOverlay(overlay: Trace.Types.Overlays.Overlay)
 }
 
 export function ariaAnnouncementForModifiedEvent(event: AnnotationModifiedEvent): string|null {
+  if (event.muteAriaNotifications) {
+    return null;
+  }
   const {overlay, action} = event;
   switch (action) {
     case 'Remove': {

front_end/panels/timeline/ModificationsManager.test.ts

@@ -70,28 +70,34 @@ describeWithEnvironment('ModificationsManager', () => {
     const modificationsManager = Timeline.ModificationsManager.ModificationsManager.activeManager();
     assert.isOk(modificationsManager);
 
-    modificationsManager.createAnnotation({
-      type: 'ENTRY_LABEL',
-      entry,
-      label: 'entry label',
-    });
-
-    modificationsManager.createAnnotation({
-      type: 'ENTRIES_LINK',
-      state: Trace.Types.File.EntriesLinkState.CONNECTED,
-      entryFrom: entry,
-      entryTo: entry2,
-    });
-
-    modificationsManager.createAnnotation({
-      type: 'TIME_RANGE',
-      bounds: {
-        min: Trace.Types.Timing.Micro(0),
-        max: Trace.Types.Timing.Micro(10),
-        range: Trace.Types.Timing.Micro(10),
-      },
-      label: 'range label',
-    });
+    modificationsManager.createAnnotation(
+        {
+          type: 'ENTRY_LABEL',
+          entry,
+          label: 'entry label',
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
+
+    modificationsManager.createAnnotation(
+        {
+          type: 'ENTRIES_LINK',
+          state: Trace.Types.File.EntriesLinkState.CONNECTED,
+          entryFrom: entry,
+          entryTo: entry2,
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
+
+    modificationsManager.createAnnotation(
+        {
+          type: 'TIME_RANGE',
+          bounds: {
+            min: Trace.Types.Timing.Micro(0),
+            max: Trace.Types.Timing.Micro(10),
+            range: Trace.Types.Timing.Micro(10),
+          },
+          label: 'range label',
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
 
     const modifications = modificationsManager.toJSON().annotations;
     assert.deepEqual(modifications, {
@@ -124,18 +130,22 @@ describeWithEnvironment('ModificationsManager', () => {
        const modificationsManager = Timeline.ModificationsManager.ModificationsManager.activeManager();
        assert.isOk(modificationsManager);
 
-       modificationsManager.createAnnotation({
-         type: 'ENTRIES_LINK',
-         state: Trace.Types.File.EntriesLinkState.CONNECTED,
-         entryFrom: entry,
-         entryTo: entry2,
-       });
-
-       modificationsManager.createAnnotation({
-         type: 'ENTRIES_LINK',
-         state: Trace.Types.File.EntriesLinkState.PENDING_TO_EVENT,
-         entryFrom: entry2,
-       });
+       modificationsManager.createAnnotation(
+           {
+             type: 'ENTRIES_LINK',
+             state: Trace.Types.File.EntriesLinkState.CONNECTED,
+             entryFrom: entry,
+             entryTo: entry2,
+           },
+           {loadedFromFile: false, muteAriaNotifications: false});
+
+       modificationsManager.createAnnotation(
+           {
+             type: 'ENTRIES_LINK',
+             state: Trace.Types.File.EntriesLinkState.PENDING_TO_EVENT,
+             entryFrom: entry2,
+           },
+           {loadedFromFile: false, muteAriaNotifications: false});
 
        // Make sure only the link with both 'to' and 'from' entries in in the generated JSON
        const modifications = modificationsManager.toJSON().annotations;
@@ -160,12 +170,14 @@ describeWithEnvironment('ModificationsManager', () => {
     assert.isOk(modificationsManager);
 
     // Create a connection between entry 1 and entry 2
-    modificationsManager.createAnnotation({
-      type: 'ENTRIES_LINK',
-      state: Trace.Types.File.EntriesLinkState.CONNECTED,
-      entryFrom: entry1,
-      entryTo: entry2,
-    });
+    modificationsManager.createAnnotation(
+        {
+          type: 'ENTRIES_LINK',
+          state: Trace.Types.File.EntriesLinkState.CONNECTED,
+          entryFrom: entry1,
+          entryTo: entry2,
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
 
     // Chech if a connection between entries 1 and 3 exists
     const existsBetween1And3 = modificationsManager.linkAnnotationBetweenEntriesExists(entry1, entry3);
@@ -188,37 +200,43 @@ describeWithEnvironment('ModificationsManager', () => {
     const modificationsManager = Timeline.ModificationsManager.ModificationsManager.activeManager();
     assert.isOk(modificationsManager);
 
-    modificationsManager.createAnnotation({
-      type: 'TIME_RANGE',
-      bounds: {
-        min: Trace.Types.Timing.Micro(0),
-        max: Trace.Types.Timing.Micro(10),
-        range: Trace.Types.Timing.Micro(10),
-      },
-      label: 'label',
-    });
+    modificationsManager.createAnnotation(
+        {
+          type: 'TIME_RANGE',
+          bounds: {
+            min: Trace.Types.Timing.Micro(0),
+            max: Trace.Types.Timing.Micro(10),
+            range: Trace.Types.Timing.Micro(10),
+          },
+          label: 'label',
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
 
     // Create time range with empty label that shoud be removed
-    modificationsManager.createAnnotation({
-      type: 'TIME_RANGE',
-      bounds: {
-        min: Trace.Types.Timing.Micro(3),
-        max: Trace.Types.Timing.Micro(10),
-        range: Trace.Types.Timing.Micro(7),
-      },
-      label: '',
-    });
+    modificationsManager.createAnnotation(
+        {
+          type: 'TIME_RANGE',
+          bounds: {
+            min: Trace.Types.Timing.Micro(3),
+            max: Trace.Types.Timing.Micro(10),
+            range: Trace.Types.Timing.Micro(7),
+          },
+          label: '',
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
 
     // Create time range with empty label that shoud be removed
-    modificationsManager.createAnnotation({
-      type: 'TIME_RANGE',
-      bounds: {
-        min: Trace.Types.Timing.Micro(5),
-        max: Trace.Types.Timing.Micro(10),
-        range: Trace.Types.Timing.Micro(5),
-      },
-      label: '',
-    });
+    modificationsManager.createAnnotation(
+        {
+          type: 'TIME_RANGE',
+          bounds: {
+            min: Trace.Types.Timing.Micro(5),
+            max: Trace.Types.Timing.Micro(10),
+            range: Trace.Types.Timing.Micro(5),
+          },
+          label: '',
+        },
+        {loadedFromFile: false, muteAriaNotifications: false});
 
     modificationsManager.deleteEmptyRangeAnnotations();
     const modifications = modificationsManager.toJSON().annotations;

front_end/panels/timeline/ModificationsManager.ts

@@ -23,7 +23,8 @@ export type UpdateAction =
 export class AnnotationModifiedEvent extends Event {
   static readonly eventName = 'annotationmodifiedevent';
 
-  constructor(public overlay: Trace.Types.Overlays.Overlay, public action: UpdateAction) {
+  constructor(
+      public overlay: Trace.Types.Overlays.Overlay, public action: UpdateAction, public muteAriaNotifications = false) {
     super(AnnotationModifiedEvent.eventName);
   }
 }
@@ -137,7 +138,10 @@ export class ModificationsManager extends EventTarget {
    * Stores the annotation and creates its overlay.
    * @returns the Overlay that gets created and associated with this annotation.
    */
-  createAnnotation(newAnnotation: Trace.Types.File.Annotation, loadedFromFile = false): Trace.Types.Overlays.Overlay {
+  createAnnotation(newAnnotation: Trace.Types.File.Annotation, opts: {
+    loadedFromFile: boolean,
+    muteAriaNotifications: boolean,
+  }): Trace.Types.Overlays.Overlay {
     // If a label already exists on an entry and a user is trying to create a new one, start editing an existing label instead.
     if (newAnnotation.type === 'ENTRY_LABEL') {
       const overlay = this.#findLabelOverlayForEntry(newAnnotation.entry);
@@ -149,7 +153,7 @@ export class ModificationsManager extends EventTarget {
 
     // If the new annotation created was not loaded from the file, set the annotations visibility setting to true. That way we make sure
     // the annotations are on when a new one is created.
-    if (!loadedFromFile) {
+    if (!opts.loadedFromFile) {
       // Time range annotation could also be used to check the length of a selection in the timeline. Therefore, only set the annotations
       // hidden to true if annotations label is added. This is done in OverlaysImpl.
       if (newAnnotation.type !== 'TIME_RANGE') {
@@ -158,7 +162,7 @@ export class ModificationsManager extends EventTarget {
     }
     const newOverlay = this.#createOverlayFromAnnotation(newAnnotation);
     this.#overlayForAnnotation.set(newAnnotation, newOverlay);
-    this.dispatchEvent(new AnnotationModifiedEvent(newOverlay, 'Add'));
+    this.dispatchEvent(new AnnotationModifiedEvent(newOverlay, 'Add', opts.muteAriaNotifications));
     return newOverlay;
   }
 
@@ -300,12 +304,12 @@ export class ModificationsManager extends EventTarget {
     return [...this.#overlayForAnnotation.values()];
   }
 
-  applyAnnotationsFromCache(): void {
+  applyAnnotationsFromCache(opts: {muteAriaNotifications: boolean}): void {
     this.#modifications = this.toJSON();
     // The cache is filled by applyModificationsIfPresent, so we clear
     // it beforehand to prevent duplicate entries.
     this.#overlayForAnnotation.clear();
-    this.#applyStoredAnnotations(this.#modifications.annotations);
+    this.#applyStoredAnnotations(this.#modifications.annotations, opts);
   }
 
   /**
@@ -383,10 +387,13 @@ export class ModificationsManager extends EventTarget {
 
     this.#timelineBreadcrumbs.setInitialBreadcrumbFromLoadedModifications(this.#modifications.initialBreadcrumb);
     this.#applyEntriesFilterModifications(hiddenEntries, expandableEntries);
-    this.#applyStoredAnnotations(this.#modifications.annotations);
+    this.#applyStoredAnnotations(this.#modifications.annotations, {
+      muteAriaNotifications: false,
+    });
   }
 
-  #applyStoredAnnotations(annotations: Trace.Types.File.SerializedAnnotations): void {
+  #applyStoredAnnotations(annotations: Trace.Types.File.SerializedAnnotations, opts: {muteAriaNotifications: boolean}):
+      void {
     try {
       // Assign annotations to an empty array if they don't exist to not
       // break the traces that were saved before those annotations were implemented
@@ -398,7 +405,10 @@ export class ModificationsManager extends EventTarget {
               entry: this.#eventsSerializer.eventForKey(entryLabel.entry, this.#parsedTrace),
               label: entryLabel.label,
             },
-            true);
+            {
+              loadedFromFile: true,
+              muteAriaNotifications: opts.muteAriaNotifications,
+            });
       });
 
       const timeRanges = annotations.labelledTimeRanges ?? [];
@@ -409,7 +419,10 @@ export class ModificationsManager extends EventTarget {
               bounds: timeRange.bounds,
               label: timeRange.label,
             },
-            true);
+            {
+              loadedFromFile: true,
+              muteAriaNotifications: opts.muteAriaNotifications,
+            });
       });
 
       const linksBetweenEntries = annotations.linksBetweenEntries ?? [];
@@ -421,7 +434,10 @@ export class ModificationsManager extends EventTarget {
               entryFrom: this.#eventsSerializer.eventForKey(linkBetweenEntries.entryFrom, this.#parsedTrace),
               entryTo: this.#eventsSerializer.eventForKey(linkBetweenEntries.entryTo, this.#parsedTrace),
             },
-            true);
+            {
+              loadedFromFile: true,
+              muteAriaNotifications: opts.muteAriaNotifications,
+            });
       });
     } catch (err) {
       // This function is wrapped in a try/catch just in case we get any incoming

front_end/panels/timeline/TimelineFlameChartView.test.ts

@@ -328,7 +328,10 @@ describeWithEnvironment('TimelineFlameChartView', function() {
     const labelAnnotation = modifications.getAnnotations().find(a => a.type === 'ENTRY_LABEL');
     assert.isOk(labelAnnotation);
     // This creates an active annotation in the UI and creates the overlay.
-    const overlay = modifications.createAnnotation(labelAnnotation);
+    const overlay = modifications.createAnnotation(labelAnnotation, {
+      loadedFromFile: false,
+      muteAriaNotifications: false,
+    });
     flameChartView.addOverlay(overlay);
     await raf();
     const overlayElement = flameChartView.overlays().elementForOverlay(overlay);