Repository navigation
fix: open modals on the top layer instead of in document flow - #219
Merged
Merged
Conversation
…itting in it `<dialog open>` is visible but not modal: the element stays in normal document flow at its static position, with no ::backdrop, no focus containment and no Esc. Every modal is authored inside the row or page owning its state, so the launch dialog rendered wherever its Agent row happened to sit -- with many Agents installed, far below the fold, adding its own height to the list rather than covering the window. Measured in both engines the app ships against, the dialog landed at top 1310px in a 600px viewport, off-screen, where showModal() puts it at 254px and centred. Two more modals hand-rolled a fixed overlay instead, and its `z-index: 20` only tied the task centre's 20, leaving DOM order to break the tie. Both shapes are now one ModalDialog calling showModal(), which promotes the element above every stacking context and outside `.app-window`'s `overflow: hidden`, and brings the backdrop, focus trap and Esc along with it. Esc is routed through onCancel to the caller's state rather than closing the element directly, so a dialog cannot go invisible while its owner still believes it is open. jsdom 30 implements no HTMLDialogElement methods at all, so the test setup polyfills them; a source guard keeps `<dialog open>` from coming back.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
问题
点击命令行 Agent 的「启动」按钮后，选择启动目录的弹窗出现在页面最底部并把页面挤开，而不是浮在窗口中央。装的 Agent 越多越明显。
根因是
<dialog open>并不是模态框。open属性只让元素可见，元素仍留在正常文档流的静态位置上，既没有::backdrop，也没有焦点约束和 Esc。而每个弹窗都写在拥有其状态的那一行/那一页内部，所以启动弹窗渲染在它所属的 Agent 行的位置上 —— Agent 一多就落到折叠线以下，并把自身高度加进列表。.app-window的overflow: hidden又会裁掉任何只靠定位往外跑的元素。在应用实际使用的两个引擎里实测同一个元素，同一份 DOM：
<dialog open>（修复前）showModal()（修复后）positionabsolutefixedtop（视口高 600px）:modalChromium 与 WebKit 结果完全一致。
排查范围
按要求统一排查了所有浮层，结论分三类：
<dialog open>：AgentManageRow（用户报告的启动目录）、TransferPage的密码 / 导出设置 / 确认导入。均为上述缺陷。MCPPage、SkillsPage。位置本身正确，但z-index: 20与任务中心的z-index: 20相等，谁在上面只由 DOM 顺序决定；两者也都完全没有 Esc。SelectField、ModelPicker等）：贴着触发器定位、z-index: 40在任务中心之上，行为正确，不在改动范围内。改动
六处弹窗统一收敛到一个
ModalDialog，内部调用showModal()。顶层（top layer）把居中、遮盖、::backdrop、焦点约束和 Esc 一次性都给到，且位于所有 stacking context 之上、overflow: hidden的裁剪之外，因此不再需要任何z-index，相等 z-index 抢层的问题随之消失。几个值得说明的决定：
onCancel回调给调用方，而不是直接关闭元素。 渲染弹窗的 state 是唯一事实来源；若让 Esc 直接关掉元素，state 仍认为「打开」，弹窗就变成一个看不见又无法再次打开的死状态。useLayoutEffect而非useEffect。 普通 effect 会留下一帧：元素已在 DOM 中但尚未进入顶层，于是先在静态位置画一次 —— 正是本次要消除的闪动。这一点也是实测发现的：改动初期全量测试约 1/4 概率随机失败，正是这一帧造成的（详见下方）。min(620px, 100%)里的100%从原先带 padding 的容器格改为按视口解析，等于失去保护，故改为calc(100vw - 48px)。.mcp-modal补color。 UA 样式表会给<dialog>设自己的color，而 UA 声明优先于继承；不补的话深色主题下会是黑字压深色背景。已在深色 + 420px 下实测确认。验证
pnpm run test：429 passed / 56 filespnpm run build（含tsc --noEmit）、go vet ./...、go test ./...、python3 scripts/check-docs.py全部通过e2e/wails-server.mjs跑真实 Go 后端）：确认弹窗:modal为 true、position: fixed、双轴居中、在视口内、::backdrop真正绘制、焦点被限制在弹窗内；requestClose()→onCancel→ state → 卸载 的闭环正常，无残留 dialog 元素关于测试稳定性
改动过程中全量测试出现约 1/4 概率的随机失败（
TransferPage :: keeps the saved key when the file carries none）。已定位并修掉，不是遗留问题：showModal()原本放在useEffect里，会在渲染后一拍才执行，中间存在「元素已在 DOM、但还没有open属性」的窗口，getByRole("dialog")在这个窗口里取不到元素，机器负载高时断言正好落进去。改用useLayoutEffect后连续 12 次全量测试全绿（修复前约 1/4 失败）。防回归
ModalDialog.test.tsx：断言showModal()被调用、可访问名正确、Esc 只上报给调用方而不自行关闭、调用方停止渲染时元素被关闭styles/modal-layer.test.ts：源码级守卫 —— 除ModalDialog外任何文件都不得再渲染<dialog>，且不得再出现<dialog open>与竞争性 fixed 遮罩。该守卫已做变异测试验证：故意还原一处<dialog open>后确实失败。HTMLDialogElement的任何方法（show/showModal/close/requestClose全是 undefined），已在test/setup.ts中补齐，并注明位置正确性由浏览器层验证、不由 jsdom 保证。