cps-dialog: let closeOnEscape be scoped to focus inside the dialog (for non-modal dialogs)
Maintainer thường phản hồi trong vòng 1 ngày
Chưa có ai nhận issue này.
Đánh giá
- Độ khó
- 3/5
- Thời gian dự kiến
- 1-2 ngày
- Mức phù hợp với người mới
- 35/100
- Loại issue
- Tính năng
- Độ rõ ràng
- Khá rõ ràng
- Mức độ hoạt động
- Sôi nổi
- Công nghệ
- angular, typescript
- Lĩnh vực
- frontend
Hướng nghiên cứu
Start in projects/cps-ui-kit/src/lib/services/cps-dialog/utils/cps-dialog-config.ts (the closeOnEscape type, around line 102), then cps-dialog.component.ts: bindGlobalListeners() (around line 840) and bindDocumentEscapeListener() (around line 868). Done when closeOnEscape accepts 'focus-within', Esc inside the dialog (including the frame) closes only the topmost dialog, Esc outside is left unprevented, and unit tests plus a Playwright spec in playwright/cps-ui-kit/components/ cover those cases.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Mô tả
Summary
cps-dialog can only close on Esc in two ways: anywhere on the page (closeOnEscape: true, the default) or never (closeOnEscape: false). A non-modal dialog that sits next to an application (a tools or inspector panel, a side panel) needs a third behaviour: close on Esc only when focus is inside the dialog, and only when the dialog is the topmost overlay.
Today an application has to choose false and write its own Esc handler. That handler lives outside the kit's stacking logic, so it cannot follow the rule the kit applies everywhere else ("Esc closes the topmost overlay"), and it misses some cases.
Proposal: let closeOnEscape also accept a focus-scoped mode, for example 'focus-within'. true and false keep their current meaning.
Current behaviour
CpsDialogConfig.closeOnEscapeis a plain boolean: "Specifies if pressing escape key should hide the dialog." (projects/cps-ui-kit/src/lib/services/cps-dialog/utils/cps-dialog-config.ts, around line 102.)- In
cps-dialog.component.ts,bindGlobalListeners()(around line 840) bindsbindDocumentEscapeListener()whencloseOnEscape !== false. That listener (around line 868) is a document-levelkeydownlistener. It hides the dialog whenparseInt(this.container.style.zIndex) === ZIndexUtils.getCurrent(), that is, when the dialog is the topmost overlay, wherever focus is. Since 21.33.0 it also callsevent.preventDefault()for the keypress it handled. - The notification service (
cps-notification.service.ts,_onKeydown, around line 262) clears notifications on Esc unlessevent.defaultPrevented, and only if a notification containerisOnTop(). - Checked in 21.32.0, 21.33.0 and 22.3.0: the dialog's Esc logic is the same apart from the
preventDefault().
Why neither value works for a non-modal dialog
An application whose dialog stays open beside the page (it is not modal and the user keeps working in the app) sees this:
closeOnEscape: true: pressing Esc in an unrelated field of the page closes the dialog. This was measured in an application: with focus in a text field outside the dialog, Esc closed it. Because the kit also callspreventDefault(), other handlers on the page that respectdefaultPreventedtreat that Esc as already handled.closeOnEscape: false: the application has to listen for Esc itself, only inside the dialog, to close it. That has three problems:- It ignores the kit's stacking rule. If a notification (or any overlay) is on top, the kit's own rule is "dismiss that first, close the dialog on the next Esc". An application cannot read the stack, so it closes the dialog while the notification is on top, or closes both.
- The application's handler has to mark the event handled (
preventDefault()) by hand, otherwise the notification service also clears the notifications on the same keypress. This is easy to forget, and the convention is not documented for dialogs. - Focus can be on the dialog frame itself (
div.cps-dialog[tabindex="-1"]), for example after clicking the header. An application listener attached to the dialog's content does not receive the keydown, so Esc does nothing at all there. (WithcloseOnEscape: trueit closes, as expected.)
The kit already contains everything needed to get this right (the z-index stack, the handled-event convention). It only lacks a way to scope the existing dialog listener to focus.
Proposed behaviour
Extend the type:
closeOnEscape?: boolean | 'focus-within';
true(default): unchanged.false: unchanged.'focus-within': the dialog closes on Esc only if- the key event's target is inside the dialog container (the content, the header, or the frame itself), and
- the dialog is the topmost overlay, using the same z-index check as today, and
- the event is handled the way it is today (
preventDefault()when the dialog closes).
An Esc from outside the dialog is left alone: it is not prevented, so the application and other overlays handle it as they do now. Overlays opened from inside the dialog (menus and tooltips were checked) already take the Esc first through the stack check and their own handlers, so nothing changes for them.
Suggested implementation: in bindDocumentEscapeListener(), when the option is 'focus-within', add this.container.contains(event.target as Node) to the existing condition. bindGlobalListeners() already tests closeOnEscape !== false, so it keeps binding the listener for the new value.
Acceptance criteria
- With
closeOnEscape: 'focus-within':- Esc pressed in an element inside the dialog closes it.
- Esc pressed on the dialog frame itself closes it (for example after a click on the header).
- Esc pressed in a field outside the dialog does not close it, and the event is not
defaultPrevented. - With a notification on top of the dialog, the first Esc inside the dialog dismisses the notification and leaves the dialog open, and the next Esc closes the dialog.
- With a menu opened from the dialog, the first Esc closes the menu, not the dialog.
closeOnEscape: trueandfalsebehave exactly as before. The existing dialog specs still pass.- Unit tests for each case above, and a Playwright test next to the existing dialog specs (
playwright/cps-ui-kit/components/) for the "Esc in a field outside the dialog" and "Esc on the frame" cases. - The dialog documentation describes the three values, with a note that
'focus-within'is meant for non-modal dialogs.
Related
- #948: the focus ring on the dialog frame covers the dialog and blocks clicks. It is reached by the same steps (click the header, then press a key), and the frame-focus case above is where the two meet.
- #943 (draft): Esc ordering between tooltips, notifications and tree-select. This request follows the same direction, for dialogs.
Alternatives considered
- Documenting that apps should call
preventDefault()in their own handler. This avoids the double close but still cannot follow the stacking rule and still misses the frame-focus case. - Making notifications ignore an Esc that starts inside a dialog. This would change the behaviour for every dialog, including ones that have disabled closing, where Esc should still dismiss notifications.
- Ngôn ngữ chính
- TypeScript
- Star
- 9
- Fork
- 8
- Merge trung bình
- 1 ngày 6 giờ
- Pull request đã merge (30 ngày)
- 25
Chuẩn bị môi trường
Dự án này không cung cấp dev container, Dockerfile hay hướng dẫn đóng góp, nên bạn cần tự thiết lập môi trường: hãy bắt đầu từ README và xem hướng dẫn đóng góp lần đầu của chúng tôi để biết các bước chung.
Bắt đầu từ đâu
- Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
- Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
- Fork repository và làm thay đổi trên một nhánh.
- Mở pull request có tham chiếu số hiệu của issue.
Issue khác của AbsaOSS/cps-shared-ui
-
cps-dialog: focus ring on the dialog frame covers the dialog and blocks all clicks inside itĐang mởbug
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 72/100
AbsaOSS/cps-shared-ui#948 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 62/100
AbsaOSS/cps-shared-ui#875 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
InputComponent - expand type beyond text/number/passwordCó thể làm lại được Pull request cho issue này đã bị đóng mà không được merge. Đang mởenhancement library
Độ khó 1/5 Dưới một giờ Mức phù hợp với người mới 85/100
AbsaOSS/cps-shared-ui#644 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 3/5 1-2 ngày Mức phù hợp với người mới 62/100
AbsaOSS/cps-shared-ui#950 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
Độ khó 3/5 Nửa ngày Mức phù hợp với người mới 55/100
AbsaOSS/cps-shared-ui#949 ·
Maintainer thường phản hồi trong vòng 1 ngày
Tất cả issue của AbsaOSS/cps-shared-ui
Issue tương tự
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
vercel-labs/ui#540 ·
Maintainer thường phản hồi trong vòng 1 ngày
-
accepted ai-dev ai-implementing
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 74/100
livesport/homer#218 · 1 bình luận ·
Maintainer thường phản hồi trong vòng 1 ngày
-
normalizePropName strips `#` characters from Figma property names instead of only the `#id` suffixĐang mở
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 72/100
figma/code-connect#419 ·
-
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 78/100
-
Negative values skip k/M abbreviation and medium decimal format on line, stacked-area, and tooltipĐang mở
Độ khó 2/5 1-3 giờ Mức phù hợp với người mới 84/100
britecharts/britecharts#1255 ·
Maintainer thường phản hồi trong vòng 1 ngày