diff --git a/.changeset/popout-widget-hide-restore.md b/.changeset/popout-widget-hide-restore.md new file mode 100644 index 00000000000..49b879bc7b5 --- /dev/null +++ b/.changeset/popout-widget-hide-restore.md @@ -0,0 +1,7 @@ +--- +"@itwin/appui-react": patch +--- + +Fixed a popped-out (child window) widget docking back to its panel instead of remaining popped out after it was hidden (`WidgetState.Hidden`) and then shown again. + +Also enforced a minimum popout window size (200x200), matching the existing minimum used for floating widgets, to prevent popout windows from progressively shrinking to an unusably small size across repeated pop-out/dock cycles. diff --git a/docs/storybook/src/widget/PopoutRestore.stories.tsx b/docs/storybook/src/widget/PopoutRestore.stories.tsx new file mode 100644 index 00000000000..33ab6acef60 --- /dev/null +++ b/docs/storybook/src/widget/PopoutRestore.stories.tsx @@ -0,0 +1,34 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Bentley Systems, Incorporated. All rights reserved. + * See LICENSE.md in the project root for license terms and full copyright notice. + *--------------------------------------------------------------------------------------------*/ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { PopoutRestoreStory } from "./PopoutRestore"; +import { AppUiDecorator } from "../Decorators"; +import { Page } from "../AppUiStory"; + +const meta = { + title: "Widget/PopoutRestore", + component: PopoutRestoreStory, + tags: ["autodocs"], + decorators: [AppUiDecorator], + parameters: { + docs: { + page: () => , + }, + }, +} satisfies Meta; + +export default meta; +type Story = StoryObj; + +/** Steps to reproduce AB#2024472: + * 1. Pop the widget out (drag its tab out of the panel, or use the tab's + * "..." menu and click "Pop out active widget tab"). + * 2. Click "Hide widget". + * 3. Click "Show widget". + * + * Expected: the widget re-appears in its popout window. + * Before the fix it would instead dock back into the left panel. + */ +export const Default: Story = {}; diff --git a/docs/storybook/src/widget/PopoutRestore.tsx b/docs/storybook/src/widget/PopoutRestore.tsx new file mode 100644 index 00000000000..3f3ddaec0be --- /dev/null +++ b/docs/storybook/src/widget/PopoutRestore.tsx @@ -0,0 +1,90 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Bentley Systems, Incorporated. All rights reserved. + * See LICENSE.md in the project root for license terms and full copyright notice. + *--------------------------------------------------------------------------------------------*/ +import React from "react"; +import { + StagePanelState, + UiItemsProvider, + WidgetState, + useSpecificWidgetDef, +} from "@itwin/appui-react"; +import { Button } from "@itwin/itwinui-react"; +import { AppUiStory } from "../AppUiStory"; +import { createFrontstage, createWidget } from "../Utils"; + +/** Demonstrates that a popped-out widget remains popped out after being hidden + * (e.g. because it has no content to display) and then shown again. + * + * The hide/show controls live outside of the widget (as `AppUiStory` children) + * since hiding the widget unmounts its content, which would otherwise take the + * "Show widget" button down with it. + * + * To reproduce: pop the widget out (drag its tab out, or use the "..." menu), + * then click "Hide widget" and "Show widget". Before the fix (AB#2024472) the + * widget would dock back into the panel instead of staying popped out. + */ +export function PopoutRestoreStory() { + const provider = { + id: "widgets", + getWidgets: () => [ + createWidget(1, { + canPopout: true, + content:
Widget content
, + }), + ], + } satisfies UiItemsProvider; + return ( + + + + ); +} + +function WidgetVisibilityControls() { + const widgetDef = useSpecificWidgetDef("w1"); + const [visible, setVisible] = React.useState(true); + + const hide = () => { + setVisible(false); + widgetDef?.setWidgetState(WidgetState.Hidden); + }; + const show = () => { + setVisible(true); + widgetDef?.setWidgetState(WidgetState.Open); + }; + + return ( +
+
Widget state: {visible ? "Visible" : "Hidden"}
+
+ + +
+
+ ); +} diff --git a/ui/appui-react/src/appui-react/frontstage/FrontstageDef.tsx b/ui/appui-react/src/appui-react/frontstage/FrontstageDef.tsx index 3e00e605d94..84c52ec1c23 100644 --- a/ui/appui-react/src/appui-react/frontstage/FrontstageDef.tsx +++ b/ui/appui-react/src/appui-react/frontstage/FrontstageDef.tsx @@ -63,6 +63,7 @@ import { FRONTSTAGE_SETTINGS_NAMESPACE, getFrontstageStateSettingName, } from "../widget-panels/Frontstage.js"; +import { MIN_POPOUT_WINDOW_SIZE } from "../layout/state/WidgetRestoreState.js"; /** FrontstageDef class provides an API for a Frontstage. * @public @@ -746,8 +747,8 @@ export class FrontstageDef { const popoutWidget = state.popoutWidgets.byId[location.popoutWidgetId]; const bounds = Rectangle.create(popoutWidget.bounds); const position: ChildWindowLocationProps = { - width: bounds.getWidth(), - height: bounds.getHeight(), + width: Math.max(bounds.getWidth(), MIN_POPOUT_WINDOW_SIZE.width), + height: Math.max(bounds.getHeight(), MIN_POPOUT_WINDOW_SIZE.height), left: bounds.left, top: bounds.top, }; @@ -764,9 +765,11 @@ export class FrontstageDef { // Use outer size if available to avoid inner size + browser zoom issues: https://github.com/iTwin/appui/issues/563 const savedTab = state.savedTabs.byId[tabId]; if (childWindow && savedTab?.popout?.size) { + // Enforce a minimum size so a popout window that shrunk over repeated pop-out cycles + // can never become invisible or unusable. childWindow.resizeTo( - savedTab.popout.size.width, - savedTab.popout.size.height + Math.max(savedTab.popout.size.width, MIN_POPOUT_WINDOW_SIZE.width), + Math.max(savedTab.popout.size.height, MIN_POPOUT_WINDOW_SIZE.height) ); } diff --git a/ui/appui-react/src/appui-react/layout/state/NineZoneStateReducer.ts b/ui/appui-react/src/appui-react/layout/state/NineZoneStateReducer.ts index 487316b6306..2968e8abb08 100644 --- a/ui/appui-react/src/appui-react/layout/state/NineZoneStateReducer.ts +++ b/ui/appui-react/src/appui-react/layout/state/NineZoneStateReducer.ts @@ -75,6 +75,7 @@ import { import { getUniqueId } from "../base/NineZone.js"; import { isPanelWidgetRestoreState, + MIN_POPOUT_WINDOW_SIZE, type PanelWidgetRestoreState, } from "./WidgetRestoreState.js"; import { @@ -670,6 +671,25 @@ export function NineZoneStateReducer( if (size) preferredBounds = preferredBounds.setSize(size); if (position) preferredBounds = preferredBounds.setPosition(position); + // Enforce a minimum size so a popout window can never shrink to an invisible or + // unusable size across repeated pop-out cycles (see AB#2024472). Matches the + // minimum size used for floating widgets (see `minWidth`/`minHeight` in `Widget.tsx`). + if ( + preferredBounds.getWidth() < MIN_POPOUT_WINDOW_SIZE.width || + preferredBounds.getHeight() < MIN_POPOUT_WINDOW_SIZE.height + ) { + preferredBounds = preferredBounds.setSize({ + width: Math.max( + preferredBounds.getWidth(), + MIN_POPOUT_WINDOW_SIZE.width + ), + height: Math.max( + preferredBounds.getHeight(), + MIN_POPOUT_WINDOW_SIZE.height + ), + }); + } + const popoutWidgetId = getUniqueId(); let home: PopoutWidgetState["home"] | undefined; if (location && isPanelTabLocation(location)) { @@ -1032,6 +1052,15 @@ function hideTab(state: NineZoneState, id: TabState["id"]) { floatingWidget, }; }); + } else if (isPopoutTabLocation(location)) { + const popoutWidget = state.popoutWidgets.byId[widgetId]; + state = updateSavedTabState(state, id, (draft) => { + draft.home = { + widgetId, + tabIndex, + popoutWidget, + }; + }); } else if (isPanelTabLocation(location)) { const side = location.side; const widgetIndex = state.panels[side].widgets.indexOf(widgetId); diff --git a/ui/appui-react/src/appui-react/layout/state/SavedTabState.ts b/ui/appui-react/src/appui-react/layout/state/SavedTabState.ts index f096aec9de3..b479626de0a 100644 --- a/ui/appui-react/src/appui-react/layout/state/SavedTabState.ts +++ b/ui/appui-react/src/appui-react/layout/state/SavedTabState.ts @@ -8,11 +8,11 @@ import type { XAndY } from "@itwin/core-geometry"; import type { TabState } from "./TabState.js"; -import type { WidgetRestoreState } from "./WidgetRestoreState.js"; +import type { TabRestoreState } from "./WidgetRestoreState.js"; import type { SizeProps } from "../../utils/SizeProps.js"; /** @internal */ -export type TabHomeState = WidgetRestoreState & { +export type TabHomeState = TabRestoreState & { readonly tabIndex: number; }; diff --git a/ui/appui-react/src/appui-react/layout/state/WidgetRestoreState.ts b/ui/appui-react/src/appui-react/layout/state/WidgetRestoreState.ts index 6556daa4116..8897a9ee7c5 100644 --- a/ui/appui-react/src/appui-react/layout/state/WidgetRestoreState.ts +++ b/ui/appui-react/src/appui-react/layout/state/WidgetRestoreState.ts @@ -7,7 +7,18 @@ */ import type { PanelSide } from "../widget-panels/PanelTypes.js"; -import type { FloatingWidgetState, WidgetState } from "./WidgetState.js"; +import type { SizeProps } from "../../utils/SizeProps.js"; +import type { + FloatingWidgetState, + PopoutWidgetState, + WidgetState, +} from "./WidgetState.js"; + +/** Minimum size enforced for a popped-out widget window. */ +export const MIN_POPOUT_WINDOW_SIZE: SizeProps = { + width: 200, + height: 200, +}; /** @internal */ export interface FloatingWidgetRestoreState { @@ -15,6 +26,12 @@ export interface FloatingWidgetRestoreState { floatingWidget: FloatingWidgetState; } +/** @internal */ +export interface PopoutWidgetRestoreState { + widgetId: WidgetState["id"]; + popoutWidget: PopoutWidgetState; +} + /** @internal */ export interface PanelWidgetRestoreState { widgetId: WidgetState["id"]; @@ -27,16 +44,29 @@ export type WidgetRestoreState = | FloatingWidgetRestoreState | PanelWidgetRestoreState; +/** Restore state for a tab, which (unlike {@link WidgetRestoreState}) can also + * restore into a popout window. + * @internal + */ +export type TabRestoreState = WidgetRestoreState | PopoutWidgetRestoreState; + /** @internal */ export function isFloatingWidgetRestoreState( - state: WidgetRestoreState + state: TabRestoreState ): state is FloatingWidgetRestoreState { return "floatingWidget" in state; } +/** @internal */ +export function isPopoutWidgetRestoreState( + state: TabRestoreState +): state is PopoutWidgetRestoreState { + return "popoutWidget" in state; +} + /** @internal */ export function isPanelWidgetRestoreState( - state: WidgetRestoreState + state: TabRestoreState ): state is PanelWidgetRestoreState { return "side" in state; } diff --git a/ui/appui-react/src/appui-react/layout/state/internal/TabStateHelpers.ts b/ui/appui-react/src/appui-react/layout/state/internal/TabStateHelpers.ts index dac93b85b99..30967f8b399 100644 --- a/ui/appui-react/src/appui-react/layout/state/internal/TabStateHelpers.ts +++ b/ui/appui-react/src/appui-react/layout/state/internal/TabStateHelpers.ts @@ -16,6 +16,7 @@ import type { SavedTabState, TabHomeState } from "../SavedTabState.js"; import { getTabLocation } from "../TabLocation.js"; import { addFloatingWidget, + addPopoutWidget, assertWidgetState, getWidgetState, removeWidget, @@ -23,7 +24,10 @@ import { updateWidgetState, } from "./WidgetStateHelpers.js"; import type { WidgetState } from "../WidgetState.js"; -import { isFloatingWidgetRestoreState } from "../WidgetRestoreState.js"; +import { + isFloatingWidgetRestoreState, + isPopoutWidgetRestoreState, +} from "../WidgetRestoreState.js"; import { Rectangle } from "@itwin/core-react/internal"; import { getUniqueId } from "../../base/NineZone.js"; import { insertPanelWidget } from "./PanelStateHelpers.js"; @@ -231,6 +235,13 @@ export function addRemovedTab( }); } + // Add to a popout widget (i.e. re-open its child window). + if (isPopoutWidgetRestoreState(home)) { + return addPopoutWidget(state, widgetId, [tabId], { + ...home.popoutWidget, + }); + } + // Add to a panel section. const panel = state.panels[home.side]; diff --git a/ui/appui-react/src/test/frontstage/FrontstageDef.test.tsx b/ui/appui-react/src/test/frontstage/FrontstageDef.test.tsx index 885d93b5ac9..50f63ef8e41 100644 --- a/ui/appui-react/src/test/frontstage/FrontstageDef.test.tsx +++ b/ui/appui-react/src/test/frontstage/FrontstageDef.test.tsx @@ -699,6 +699,44 @@ describe("FrontstageDef", () => { ); expect(spy).toHaveBeenCalledOnce(); }); + + it("should enforce a minimum popout window size (AB#2024472)", async () => { + const frontstageDef = new FrontstageDef(); + await frontstageDef.initializeFromConfig({ + ...defaultFrontstageConfig, + leftPanel: { + sections: { + start: [ + { + id: "t1", + }, + ], + }, + }, + }); + initializeNineZoneState(frontstageDef); + + const spy = vi.spyOn(window, "open").mockReturnValue({ + addEventListener: () => {}, + removeEventListener: () => {}, + } as unknown as Window); + frontstageDef.popoutWidget("t1", undefined, { + width: 10, + height: 10, + }); + + spy.mockReset(); + const popoutWidgets = frontstageDef.nineZoneState!.popoutWidgets; + const popoutWidget = popoutWidgets.byId[popoutWidgets.allIds[0]]; + frontstageDef.openPopoutWidgetContainer( + popoutWidget.id, + frontstageDef.nineZoneState + ); + expect(spy).toHaveBeenCalledOnce(); + const features = spy.mock.calls[0][2] as string; + expect(features).to.include("width=200"); + expect(features).to.include("height=200"); + }); }); describe("dockWidgetContainer", () => { diff --git a/ui/appui-react/src/test/layout/state/NineZoneStateReducer.test.ts b/ui/appui-react/src/test/layout/state/NineZoneStateReducer.test.ts index b5bffcf03b2..70908e8c3ed 100644 --- a/ui/appui-react/src/test/layout/state/NineZoneStateReducer.test.ts +++ b/ui/appui-react/src/test/layout/state/NineZoneStateReducer.test.ts @@ -29,6 +29,8 @@ import { updateSavedTabState, } from "../../../appui-react/layout/state/internal/TabStateHelpers.js"; import { getUniqueId } from "../../../appui-react/layout/base/NineZone.js"; +import { getTabLocation } from "../../../appui-react/layout/state/TabLocation.js"; +import type { PopoutWidgetRestoreState } from "../../../appui-react/layout/state/WidgetRestoreState.js"; describe("NineZoneStateReducer", () => { it("should not update for unhandled action", () => { @@ -1686,6 +1688,41 @@ describe("NineZoneStateReducer", () => { }); }); + it("should enforce a minimum size when saved bounds are too small", () => { + let state = createNineZoneState({ + savedTabs: { + allIds: ["t1"], + byId: { + t1: { + id: "t1", + popout: { + position: { + x: 20, + y: 10, + }, + contentSize: { + height: 40, + width: 30, + }, + }, + }, + }, + }, + }); + state = addTabs(state, ["t1"]); + state = addPanelWidget(state, "right", "rightStart", ["t1"]); + + const newState = NineZoneStateReducer(state, { + type: "WIDGET_TAB_POPOUT", + id: "t1", + }); + const popoutWidgetId = newState.popoutWidgets.allIds[0]; + const popoutWidget = newState.popoutWidgets.byId[popoutWidgetId]; + const bounds = Rectangle.create(popoutWidget.bounds); + expect(bounds.getWidth()).to.be.gte(200); + expect(bounds.getHeight()).to.be.gte(200); + }); + it("should popout with default size and location", () => { let state = createNineZoneState({ size: { height: 1000, width: 1600 } }); state = addTabs(state, ["t1"]); @@ -1704,7 +1741,7 @@ describe("NineZoneStateReducer", () => { }); }); - it("should popout with specified size and location", () => { + it("should popout with specified size and location (clamped to minimum width)", () => { let state = createNineZoneState({ size: { height: 1000, width: 1600 } }); state = addTabs(state, ["t1", "ta", "tb"]); state = addPanelWidget(state, "right", "rightStart", ["t1", "ta", "tb"], { @@ -1725,11 +1762,13 @@ describe("NineZoneStateReducer", () => { }); expect(newState.popoutWidgets.allIds).lengthOf(1); const popoutWidgetId = newState.popoutWidgets.allIds[0]; + // Requested width (100) is below the minimum popout window size and gets clamped to + // 200 (AB#2024472); height (200) is already at the minimum. expect(newState.popoutWidgets.byId[popoutWidgetId].bounds).toEqual({ left: 5, top: 10, bottom: 10 + 200, - right: 5 + 100, + right: 5 + 200, }); }); @@ -1812,7 +1851,7 @@ describe("NineZoneStateReducer", () => { }); }); - it("should popout a tab and fit to preferredFloatingWidgetSize if bounds are not set", () => { + it("should popout a tab and fit to preferredFloatingWidgetSize if bounds are not set (clamped to minimum size)", () => { let state = createNineZoneState(); state = addTab(state, "t1", { preferredFloatingWidgetSize: { width: 50, height: 50 }, @@ -1825,15 +1864,16 @@ describe("NineZoneStateReducer", () => { }); expect(newState.popoutWidgets.allIds).toHaveLength(1); const popoutWidgetId = newState.popoutWidgets.allIds[0]; + // A preferred size smaller than the minimum popout window size is clamped up (AB#2024472). expect(newState.popoutWidgets.byId[popoutWidgetId].bounds).toEqual({ left: 0, top: 0, - bottom: 50, - right: 50, + bottom: 200, + right: 200, }); }); - it("should popout a tab and fit to content container if preferredFloatingWidgetSize is not set", () => { + it("should popout a tab and fit to content container if preferredFloatingWidgetSize is not set (clamped to minimum size)", () => { let state = createNineZoneState(); const blankHTML = document.createElement("div"); @@ -1850,11 +1890,13 @@ describe("NineZoneStateReducer", () => { expect(newState.popoutWidgets.allIds).toHaveLength(1); const popoutWidgetId = newState.popoutWidgets.allIds[0]; + // A blank content container measures 20x20 but gets clamped to the minimum popout + // window size (AB#2024472). expect(newState.popoutWidgets.byId[popoutWidgetId].bounds).toEqual({ left: 0, top: 0, - bottom: 20, - right: 20, + bottom: 200, + right: 200, }); }); }); @@ -1941,7 +1983,7 @@ describe("NineZoneStateReducer", () => { }); }); - it("should hide tab in a popout widget", () => { + it("should hide tab in a popout widget and remember its popout home", () => { let state = createNineZoneState(); state = addTab(state, "t1"); state = addPopoutWidget(state, "w1", ["t1"]); @@ -1951,7 +1993,35 @@ describe("NineZoneStateReducer", () => { id: "t1", }); expect(newState.popoutWidgets.allIds).lengthOf(0); - expect(newState.savedTabs.byId.t1).toEqual(undefined); + expect(newState.savedTabs.byId.t1?.home).to.deep.include({ + widgetId: "w1", + tabIndex: 0, + }); + expect( + (newState.savedTabs.byId.t1?.home as PopoutWidgetRestoreState) + .popoutWidget.id + ).to.eq("w1"); + }); + + it("should restore a hidden tab back into a popout widget when shown", () => { + let state = createNineZoneState(); + state = addTab(state, "t1"); + state = addPopoutWidget(state, "w1", ["t1"]); + state = NineZoneStateReducer(state, { + type: "WIDGET_TAB_HIDE", + id: "t1", + }); + expect(state.popoutWidgets.allIds).lengthOf(0); + + const newState = NineZoneStateReducer(state, { + type: "WIDGET_TAB_OPEN", + id: "t1", + }); + expect(newState.popoutWidgets.allIds).lengthOf(1); + expect(getTabLocation(newState, "t1")).to.deep.eq({ + widgetId: newState.popoutWidgets.allIds[0], + popoutWidgetId: newState.popoutWidgets.allIds[0], + }); }); it("should return correct index for first tab", () => {