Skip to content

Commit b5ec47f

Browse files
authored
Fix overflow menu teardown convergence (#36602)
1 parent 7c4ccae commit b5ec47f

3 files changed

Lines changed: 119 additions & 1 deletion

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "fix: avoid redundant overflow recomputation after conditional menu removal",
4+
"packageName": "@fluentui/priority-overflow",
5+
"email": "bsunderhus@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

packages/react-components/priority-overflow/src/overflowManager.test.ts

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,108 @@ describe('overflowManager', () => {
224224
expect(getClientWidth).toHaveBeenCalledTimes(1);
225225
});
226226

227+
it('should not recompute when the same overflow menu is added twice', () => {
228+
const manager = createOverflowManager(createObserveOptions());
229+
const container = createContainer(100);
230+
const getClientWidth = jest.fn(() => 100);
231+
Object.defineProperty(container, 'clientWidth', { configurable: true, get: getClientWidth });
232+
const menu = createElementWithSize('button', 30);
233+
234+
manager.addItem({ element: createElementWithSize('button', 60), id: 'a', priority: 1 });
235+
manager.addItem({ element: createElementWithSize('button', 60), id: 'b', priority: 0 });
236+
manager.observe(container);
237+
manager.forceUpdate();
238+
manager.addOverflowMenu(menu);
239+
getClientWidth.mockClear();
240+
241+
const listener = jest.fn();
242+
manager.subscribe(listener);
243+
manager.addOverflowMenu(menu);
244+
245+
expect(listener).not.toHaveBeenCalled();
246+
expect(getClientWidth).not.toHaveBeenCalled();
247+
});
248+
249+
it('should not recompute when no overflow menu is registered', () => {
250+
const manager = createOverflowManager(createObserveOptions());
251+
const container = createContainer(100);
252+
const getClientWidth = jest.fn(() => 100);
253+
Object.defineProperty(container, 'clientWidth', { configurable: true, get: getClientWidth });
254+
255+
manager.addItem({ element: createElementWithSize('button', 60), id: 'a', priority: 1 });
256+
manager.addItem({ element: createElementWithSize('button', 60), id: 'b', priority: 0 });
257+
manager.observe(container);
258+
manager.forceUpdate();
259+
getClientWidth.mockClear();
260+
261+
const listener = jest.fn();
262+
manager.subscribe(listener);
263+
manager.removeOverflowMenu();
264+
265+
expect(listener).not.toHaveBeenCalled();
266+
expect(getClientWidth).not.toHaveBeenCalled();
267+
});
268+
269+
it('should not recompute when the overflow menu is removed after all items become visible', () => {
270+
const manager = createOverflowManager(createObserveOptions());
271+
const container = createContainer(110);
272+
const getClientWidth = jest.fn(() => 110);
273+
Object.defineProperty(container, 'clientWidth', { configurable: true, get: getClientWidth });
274+
const menu = createElementWithSize('button', 30);
275+
let menuAttached = false;
276+
277+
const createResponsiveItem = () => {
278+
const item = document.createElement('button');
279+
Object.defineProperty(item, 'offsetWidth', {
280+
configurable: true,
281+
get: () => (menuAttached ? 35 : 60),
282+
});
283+
return item;
284+
};
285+
286+
manager.addItem({ element: createResponsiveItem(), id: 'a', priority: 1 });
287+
manager.addItem({ element: createResponsiveItem(), id: 'b', priority: 0 });
288+
manager.observe(container);
289+
manager.forceUpdate();
290+
expect(getInvisibleIds(manager)).toEqual(['b']);
291+
292+
menuAttached = true;
293+
manager.addOverflowMenu(menu);
294+
expect(getInvisibleIds(manager)).toEqual([]);
295+
getClientWidth.mockClear();
296+
297+
const listener = jest.fn();
298+
manager.subscribe(listener);
299+
menuAttached = false;
300+
manager.removeOverflowMenu();
301+
302+
expect(listener).not.toHaveBeenCalled();
303+
expect(getClientWidth).not.toHaveBeenCalled();
304+
expect(getVisibleIds(manager)).toEqual(['a', 'b']);
305+
});
306+
307+
it('should recompute when the overflow menu is removed with hidden items', () => {
308+
const manager = createOverflowManager(createObserveOptions());
309+
const container = createContainer(140);
310+
const getClientWidth = jest.fn(() => 140);
311+
Object.defineProperty(container, 'clientWidth', { configurable: true, get: getClientWidth });
312+
313+
manager.addItem({ element: createElementWithSize('button', 60), id: 'a', priority: 1 });
314+
manager.addItem({ element: createElementWithSize('button', 60), id: 'b', priority: 0 });
315+
manager.addItem({ element: createElementWithSize('button', 60), id: 'c', priority: -1 });
316+
manager.addOverflowMenu(createElementWithSize('button', 30));
317+
manager.observe(container);
318+
manager.forceUpdate();
319+
expect(getInvisibleIds(manager)).toHaveLength(2);
320+
getClientWidth.mockClear();
321+
322+
manager.removeOverflowMenu();
323+
324+
expect(getClientWidth).toHaveBeenCalledTimes(1);
325+
expect(getVisibleIds(manager)).toHaveLength(2);
326+
expect(getInvisibleIds(manager)).toHaveLength(1);
327+
});
328+
227329
it('should remove items through removeItem', () => {
228330
const manager = createOverflowManager(createObserveOptions());
229331
const container = createContainer(100);

packages/react-components/priority-overflow/src/overflowManager.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,10 @@ export function createOverflowManager(initialOptions: Partial<OverflowOptions> =
335335
};
336336

337337
const addOverflowMenu: OverflowManager['addOverflowMenu'] = el => {
338+
if (overflowMenu === el) {
339+
return;
340+
}
341+
338342
overflowMenu = el;
339343

340344
if (observing) {
@@ -357,9 +361,14 @@ export function createOverflowManager(initialOptions: Partial<OverflowOptions> =
357361
};
358362

359363
const removeOverflowMenu: OverflowManager['removeOverflowMenu'] = () => {
364+
if (!overflowMenu) {
365+
return;
366+
}
367+
368+
const hasInvisibleItems = invisibleItemQueue.size() > 0;
360369
overflowMenu = undefined;
361370

362-
if (observing) {
371+
if (observing && hasInvisibleItems) {
363372
forceDispatch = true;
364373
update();
365374
}

0 commit comments

Comments
 (0)