fix: Prevent the media grid from collapsing when its cells are unchanged (#2608)

- For: #2306

src/components-lib/media-grid-controller.ts

@@ -60,14 +60,10 @@ export class MediaGridController {
   // If the order in which the observers are declared changes, the unittest must
   // be updated in triggerResizeObserver and triggerMutationObserver.
   private _hostMutationObserver = new MutationObserver(
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    this._hostMutationHandler.bind(this),
-    (_mutations: MutationRecord[], _observer: MutationObserver) =>
-      this._calculateGridContentsFromHost(),
   );
   private _cellMutationObserver = new MutationObserver(
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
+    this._cellMutationHandler.bind(this),
-    (_mutations: MutationRecord[], _observer: MutationObserver) =>
-      this._calculateGridContentsFromHost(),
   );
   private _hostResizeObserver = new ResizeObserver(this._hostResizeHandler.bind(this));
   private _cellResizeObserver = new ResizeObserver(this._cellResizeHandler.bind(this));
@@ -88,9 +84,9 @@ export class MediaGridController {
     // Need to separately listen for slotchanges since mutation observer will
     // not be called for shadom DOM slotted changes.
     if (host instanceof HTMLSlotElement) {
-      host.addEventListener('slotchange', this._calculateGridContentsFromHost);
+      host.addEventListener('slotchange', this._setGridContentsFromHost);
     }
-    this._calculateGridContentsFromHost();
+    this._setGridContentsFromHost();
   }
 
   public destroy(): void {
@@ -101,7 +97,7 @@ export class MediaGridController {
     this._cellMutationObserver.disconnect();
 
     if (this._host instanceof HTMLSlotElement) {
-      this._host.removeEventListener('slotchange', this._calculateGridContentsFromHost);
+      this._host.removeEventListener('slotchange', this._setGridContentsFromHost);
     }
 
     this._masonry?.destroy?.();
@@ -116,7 +112,11 @@ export class MediaGridController {
   public setDisplayConfig(displayConfig: ViewDisplayConfig | null): void {
     if (!isEqual(displayConfig, this._displayConfig)) {
       this._displayConfig = displayConfig;
-      this._calculateGridContentsFromHost();
+
+      // The cells are unchanged, but the config drives both their order and
+      // their size.
+      this._sortItemsInGrid();
+      this._applyCellSize();
     }
   }
 
@@ -133,30 +133,39 @@ export class MediaGridController {
   }
 
   private _sortItemsInGrid(): void {
-    const existingItems = this._masonry?.items;
+    const masonry = this._masonry;
-    const selectedItem = existingItems?.find(
+    if (!masonry) {
-      (item) => item.element.getAttribute(this._idAttribute) === this._selected,
+      return;
+    }
+
+    // Implementation note: With the latest version of the Masonry library
+    // (4.2.2) using the prepended() and appended() methods in quick succession
+    // causes the layout to not show the newly added items. Instead, access the
+    // items in place and swap them around.
+    //
+    // Order is always derived from the grid contents rather than from the
+    // current item order, which may be the result of an earlier sort against a
+    // different selection or `grid_selected_position`.
+    const cells = [...this._gridContents.values()];
+    const sortedItems = [...masonry.items].sort(
+      (a, b) => cells.indexOf(a.element) - cells.indexOf(b.element),
     );
 
     // If `grid_selected_position` is set to 'first' or 'last', move the
     // selected item to the start or end of the list respectively.
-    if (
+    const selectedPosition = this._displayConfig?.grid_selected_position;
-      !!this._displayConfig?.grid_selected_position &&
+    const selectedItem = sortedItems.find(
-      ['first', 'last'].includes(this._displayConfig.grid_selected_position) &&
+      (item) => item.element.getAttribute(this._idAttribute) === this._selected,
-      existingItems &&
+    );
-      selectedItem &&
+
-      this._masonry
+    if (selectedItem && (selectedPosition === 'first' || selectedPosition === 'last')) {
-    ) {
+      const otherItems = sortedItems.filter((item) => item !== selectedItem);
-      // Implementation note: With the latest version of the Masonry library
+      masonry.items =
-      // (4.2.2) using the prepended() and appended() methods in quick succession
+        selectedPosition === 'first'
-      // causes the layout to not show the newly added items. Instead, access
-      // the items in place and swap them around.
-      const otherItems = existingItems?.filter((item) => item !== selectedItem);
-      const newItems =
-        this._displayConfig.grid_selected_position === 'first'
           ? [selectedItem, ...otherItems]
           : [...otherItems, selectedItem];
-      this._masonry.items = newItems;
+    } else {
+      masonry.items = sortedItems;
     }
   }
 
@@ -199,7 +208,7 @@ export class MediaGridController {
     this._masonry?.layout?.();
   }
 
-  private _calculateGridContentsFromHost = (): void => {
+  private _setGridContentsFromHost = (): void => {
     const children = getChildrenFromElement(this._host);
     const gridContents: MediaGridContents = new Map();
     for (const child of children) {
@@ -210,7 +219,25 @@ export class MediaGridController {
     this._setGridContents(gridContents);
   };
 
+  private _hasSameCells(gridContents: MediaGridContents): boolean {
+    if (gridContents.size !== this._gridContents.size) {
+      return false;
+    }
+
+    const existingCells = [...this._gridContents];
+    return [...gridContents].every(
+      ([id, element], index) =>
+        existingCells[index][0] === id && existingCells[index][1] === element,
+    );
+  }
+
   private _setGridContents(gridContents: MediaGridContents): void {
+    // Rebuilding Masonry resets the container height and clears every cell
+    // position, so only do it when the cells themselves change.
+    if (this._masonry && this._hasSameCells(gridContents)) {
+      return;
+    }
+
     this._gridContents = gridContents;
 
     if (this._selected !== null && !this._gridContents.has(this._selected)) {
@@ -222,7 +249,8 @@ export class MediaGridController {
       this._addChildEventListeners(element);
     }
 
-    this._setColumnSizeStyles();
+    // Size the cells before Masonry measures them.
+    this._setCellSizeStyles();
     this._createMasonry();
 
     // Observe grid elements for size or id changes.
@@ -238,8 +266,41 @@ export class MediaGridController {
 
     this._sortItemsInGrid();
     this._updateSelectedStylesOnElements();
-    this._updateWidthFactorStyles();
+
+    // A rebuilt grid has no cell positions at all, so lay it out immediately: a
+    // throttled layout may be deferred, leaving the card collapsed until it
+    // runs.
+    this._forceLayout();
+  }
+
+  private _setCellSizeStyles(): void {
     this._setColumnSizeStyles();
+    this._updateWidthFactorStyles();
+  }
+
+  // Apply a changed cell size to the existing grid. Masonry is updated in place
+  // rather than recreated: a destroy resets the container height to 0, which
+  // can cause an ancestor scrollbar to appear/disappear, changing the available
+  // width and triggering an infinite resize oscillation.
+  // See: https://github.com/dermotduffy/advanced-camera-card/issues/2306
+  private _applyCellSize(): void {
+    this._setCellSizeStyles();
+    this._masonry?.option?.({ columnWidth: this._getColumnSize() });
+    this._throttledLayout();
+  }
+
+  private _hostMutationHandler(): void {
+    this._setGridContentsFromHost();
+  }
+
+  private _cellMutationHandler(mutations: MutationRecord[]): void {
+    // A changed id changes which cell is which, so the grid must be rebuilt. A
+    // changed width factor only changes how the cells are sized.
+    if (mutations.some((mutation) => mutation.attributeName === this._idAttribute)) {
+      this._setGridContentsFromHost();
+    } else {
+      this._applyCellSize();
+    }
   }
 
   private _hostResizeHandler(): void {
@@ -249,16 +310,7 @@ export class MediaGridController {
     // height may change during the layout.
     if (dimensions.width !== this._hostWidth) {
       this._hostWidth = dimensions.width;
-
+      this._applyCellSize();
-      // Reset the column CSS sizes first.
-      this._setColumnSizeStyles();
-
-      // Update the column width on the existing Masonry instance rather than
-      // destroying and recreating it. A destroy resets the container height to
-      // 0, which can cause an ancestor scrollbar to appear/disappear, changing
-      // the available width and triggering an infinite resize oscillation.
-      this._masonry?.option?.({ columnWidth: this._getColumnSize() });
-      this._throttledLayout();
     }
   }
 
@@ -295,7 +347,6 @@ export class MediaGridController {
       gutter: MEDIA_GRID_HORIZONTAL_GUTTER_WIDTH,
     }) as ExtendedMasonry;
     this._masonry.addItems?.([...this._gridContents.values()]);
-    this._throttledLayout();
   }
 
   private _handleSelectGridCellEvent = (ev: Event): void => {

tests/components-lib/media-grid-controller.test.ts

@@ -65,11 +65,17 @@ const createController = (host: HTMLElement, options?: MediaGridConstructorOptio
   return new MediaGridController(host, options);
 };
 
-const triggerMutationObserver = (hostOrCell: 'cell' | 'host'): void => {
+const triggerMutationObserver = (
+  hostOrCell: 'cell' | 'host',
+  attributeName?: string,
+): void => {
   const mutationObserverTrigger = vi.mocked(global.MutationObserver).mock.calls[
     hostOrCell === 'host' ? 0 : 1
   ][0];
-  mutationObserverTrigger([], mock<MutationObserver>());
+  mutationObserverTrigger(
+    attributeName ? [mock<MutationRecord>({ attributeName: attributeName })] : [],
+    mock<MutationObserver>(),
+  );
 };
 
 const triggerResizeObserver = (hostOrCell: 'cell' | 'host'): void => {
@@ -287,7 +293,7 @@ describe('MediaGridController', () => {
     children[1].setAttribute('test-id', 'beta');
     children[2].setAttribute('test-id', 'gamma');
 
-    triggerMutationObserver('cell');
+    triggerMutationObserver('cell', 'test-id');
 
     expect(controller.getGridContents()).toEqual(
       new Map([
@@ -358,11 +364,10 @@ describe('MediaGridController', () => {
     const controller = createController(parent);
     controller.setDisplayConfig({ mode: 'grid', grid_columns: 2 });
 
-    // Will have been called once on construction, and then again when the
+    // The cells are unchanged, so the new column width is applied to the
-    // number of columns changes.
+    // existing Masonry instance rather than by constructing a new one.
-    expect(Masonry).toBeCalledTimes(2);
+    expect(Masonry).toBeCalledTimes(1);
-    expect(Masonry).toBeCalledWith(
+    expect(masonry.option).toBeCalledWith(
-      parent,
       expect.objectContaining({
         columnWidth: 1499,
       }),
@@ -372,6 +377,52 @@ describe('MediaGridController', () => {
     ).toBe('1499px');
   });
 
+  it('should not rebuild the grid when the cells are unchanged', () => {
+    const slot = createSlot();
+    createSlotHost({ slot: slot, children: createChildren() });
+    createController(slot);
+
+    expect(Masonry).toBeCalledTimes(1);
+    expect(masonry.destroy).not.toBeCalled();
+
+    slot.dispatchEvent(new Event('slotchange'));
+
+    expect(Masonry).toBeCalledTimes(1);
+    expect(masonry.destroy).not.toBeCalled();
+  });
+
+  it('should rebuild the grid and lay it out when the cells change', () => {
+    const slot = createSlot();
+    const host = createSlotHost({ slot: slot, children: createChildren() });
+    createController(slot);
+
+    expect(Masonry).toBeCalledTimes(1);
+
+    host.replaceChildren(...createChildren());
+    slot.dispatchEvent(new Event('slotchange'));
+
+    expect(Masonry).toBeCalledTimes(2);
+    expect(masonry.destroy).toBeCalledTimes(1);
+
+    // A rebuild leaves the cells unpositioned, so the layout must not be left
+    // to the throttle.
+    expect(masonry.layout).toBeCalled();
+  });
+
+  it('should rebuild the grid when the number of cells changes', () => {
+    const slot = createSlot();
+    const host = createSlotHost({ slot: slot, children: createChildren() });
+    createController(slot);
+
+    expect(Masonry).toBeCalledTimes(1);
+
+    host.append(...createChildren(['new-cell']));
+    slot.dispatchEvent(new Event('slotchange'));
+
+    expect(Masonry).toBeCalledTimes(2);
+    expect(masonry.destroy).toBeCalledTimes(1);
+  });
+
   it('should respect selected width factor', () => {
     const parent = createParent({ children: createChildren(), width: 2000 });
     const controller = createController(parent);
@@ -535,6 +586,53 @@ describe('MediaGridController', () => {
         { element: children[1] },
       ]);
     });
+
+    it('should order from the grid contents rather than the previous sort', () => {
+      const children = createChildren(['0', '1', '2']);
+      const parent = createParent({ children: children });
+
+      // Simulate wrapped children in masonry object.
+      masonry.items = children.map((child) => ({ element: child }));
+
+      const controller = createController(parent);
+      controller.setDisplayConfig({ mode: 'grid', grid_selected_position: 'last' });
+
+      controller.selectCell('0');
+      expect(masonry.items).toEqual([
+        { element: children[1] },
+        { element: children[2] },
+        { element: children[0] },
+      ]);
+
+      // The unselected cells return to their configured order rather than
+      // keeping the order the previous sort left them in.
+      controller.selectCell('1');
+      expect(masonry.items).toEqual([
+        { element: children[0] },
+        { element: children[2] },
+        { element: children[1] },
+      ]);
+
+      // Returning to 'default' restores the configured order entirely.
+      controller.setDisplayConfig({ mode: 'grid', grid_selected_position: 'default' });
+      expect(masonry.items).toEqual([
+        { element: children[0] },
+        { element: children[1] },
+        { element: children[2] },
+      ]);
+    });
+
+    it('should not sort a destroyed grid', () => {
+      const children = createChildren(['0', '1', '2']);
+      const parent = createParent({ children: children });
+      const controller = createController(parent);
+
+      controller.destroy();
+
+      expect(() =>
+        controller.setDisplayConfig({ mode: 'grid', grid_selected_position: 'first' }),
+      ).not.toThrow();
+    });
   });
 
   describe('should set width factor styles correctly', () => {
@@ -568,7 +666,7 @@ describe('MediaGridController', () => {
 
       // Set the attribute.
       children[0].setAttribute('grid-width-factor', '4');
-      triggerMutationObserver('cell');
+      triggerMutationObserver('cell', 'grid-width-factor');
 
       expect(
         children[0].style.getPropertyValue('--advanced-camera-card-grid-width-factor'),
@@ -587,7 +685,7 @@ describe('MediaGridController', () => {
 
       // Remove the attribute.
       children[0].removeAttribute('grid-width-factor');
-      triggerMutationObserver('cell');
+      triggerMutationObserver('cell', 'grid-width-factor');
 
       expect(
         children[0].style.getPropertyValue('--advanced-camera-card-grid-width-factor'),