Uh oh!
There was an error while loading. Please reload this page.
PkDialog Esc layering + component routing (JS) · image-selector quality sweep - #777
Merged
Conversation
…component-aware
Chromium groups the close watchers of dialogs whose showModal() ran
without user activation (LiveView-patch opens), so one Esc fires
cancel on EVERY stacked dialog — the catalogue's item selector closed
together with the product-details popup on top of it ('Esc closes both
popups, but only top one needs to go', 2026-08-31). A dialog whose DOM
contains another open dialog now swallows its own cancel; the child
handles itself, and the next Esc reaches the parent.
InfiniteScroll's sentinel pushed with pushEvent, which always routes
to the LiveView — inside a LiveComponent (the item selector's list) it
was unusable. pushEventTo with the sentinel element routes to the
owning component and falls back to the LV when there is none, a strict
superset of the old behaviour.Follow-up to the previous commit: preventDefault on the parent's grouped cancel aborts Chromium's whole close request, so the child stayed open too — Esc then closed NEITHER popup (verified live). The parent now swallows its own close and explicitly closes the deepest stacked child; the child's own grouped cancel no-ops on an already-closed dialog, and the next Esc reaches the parent.
Refines the previous fix: closing the child directly skipped ITS cancel semantics (a non-closeable or server-driven child must get a say). The parent now dispatches a cancelable synthetic 'cancel' to the deepest stacked child and closes it only when not prevented — the browser's own Esc contract, one dialog down.
pushEventTo with the hook's own element resolves to the LiveView — the
element carries no data-phx-component — so a component-owned modal's
close event landed on the HOST instead of the component. That is the
real anatomy of 'Esc closes both popups': the stacked details dialog's
close_detail hit the host LV and died there (the details never closed
by Esc at all), while the selector's misrouted cancel produced the
all-closed look. Both hooks now target closest('[data-phx-component]')
with the element as the LV fallback, so component- and LV-owned
dialogs both route correctly.An element passed to pushEventTo is resolved through ITS phx-target
attribute — which a dialog/sentinel doesn't carry — so both the
component-root-element and raw-element forms still landed on the
LiveView. withinTargets accepts a numeric CID as a first-class target;
read it from closest('[data-phx-component]') and push to that, falling
back to pushEvent for LV-owned elements.The child's queued 'close' event was observed never firing in this stack, so relaying through it left the details popup a client-closed zombie the server still rendered. The parent now reads the child's data-close-event, pushes it straight to the child's owning component (by CID), and closes the child element for the instant visual. If the close echo does fire somewhere, the duplicate push is idempotent by the close-event contract.
Two hardenings on the stacked-cancel relay from external review: - The parent's direct push of the child's close event raced the child's own 'close' echo — when that event does fire (it was observed not to in the shipped stack, which is why the direct push exists), the same close event reached the server twice, double-firing any handler that is not idempotent. The parent now flags the child element and the child's close handler skips the echo once. - The stacked-child lookup matched 'dialog[open]', but morphdom can strip the open attribute between a patch and the next _sync — the relay would miss the child in that window. Match by isDialogOpenInBrowser (the :modal truth source) instead.
The boss's low-quality report (2026-08-31): the media selector modal's grid rendered aspect-square card tiles (~150-300px CSS, double on retina) from the DEFAULT :small tier — the 150px thumbnail stretched across the whole card. The media browser's grid already asked for :card; the selector (and its user-scoped wrapper) now does too. The sweep also caught two inverted chains in the non-image resolve_url/2 clauses: a :card request preferred the 150px thumbnail over the 800px medium whenever small was missing, and a :medium request preferred the thumbnail over the actual medium variant. Both now mirror the image-typed clauses' ordering. Everything else checked clean: browser list rows use :small in 40px slots, site-icon previews are icon-sized, the grid/stack cards were already :card. Pinned in media_thumbnail_test (the generic clauses had no pins — which is how the inversions survived).
mdon
commented
Aug 31, 2026
ContributorAuthor
|
Pages that publish their full URL into :url_path (so the language switcher can rebuild locale links without dropping state - the catalogue's ?category= drill) hand tab matching a path with a query attached. normalize_path now strips query and fragment before the trailing-slash/prefix/locale normalization, so exact and prefix matches see the path alone.
The notification digest built its text through Gettext.dgettext(...) - the runtime API, a plain function call the gettext extractor never sees - so its five strings never entered the pot and no locale could translate them. The worker now uses the macro form via use Gettext, backend: PhoenixKitWeb.Gettext. The catalogues were also regenerated and completed: Estonian had 48 untranslated strings and Russian 129 (the digest strings, integration key-store diagnostics, login greetings, timezone copy, notification counts, storage settings and the auth/OAuth/magic-link flash set). Both now carry a full set; the other locales picked up the merge's reference updates and new empty entries only.
mdon
commented
Aug 31, 2026
ContributorAuthor
Two more commits riding this PR:
precommit green; deployed to max-dev, hand-patched into the tim-dev preview. |
ddon pushed a commit
that referenced
this pull request
Aug 31, 2026
PR #777's diagnoses were all correct; four defects sat around them. Digest notifications: the PR made the strings extractable and shipped Russian for them, but gettext/2 resolves the locale from the calling process and an Oban worker starts on the default one — so every recipient would have got English, including the one whose locale the envelope resolves on the line above. Both digest surfaces (channel envelope, persisted in-app row) now build their text inside Gettext.with_locale/3. AdminNav.parse_admin_path/1, the sibling of the matcher the PR fixed, stripped only the query, so a fragment-carrying current_path highlighted the tab and not the sidebar item; its ?tab= extraction also read "?tab=files#top" as the tab "files#top". Both now cut on ? and #. PkDialog's stacked-close marker was a boolean cleared only by the child's own close handler — the handler the PR's comment says was observed not to run there. Left set, it suppressed that child's next genuine close for the life of the element. It is a timestamp with a one-second window now. The CID-routing block was copy-pasted three times and walked past nested LiveView roots (a component on the far side belongs to another view, and its cid means nothing to the pushing socket). One shared ownerComponentCid / pushToOwner pair, exported through the bundle's existing test seam. Review: dev_docs/pull_requests/2026/777-pkdialog-esc-layering-component-routing/CLAUDE_REVIEW.md Tests: 4 digest (DB-free), 5 admin-nav render, 8 JS routing — the admin-nav fragment pair fails on the pre-fix tree. Gate: mix precommit green; mix test 4342 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J9kiQnNDtxwHzdDbHDbZHz
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What this does
JS-only changes to the hand-maintained
priv/static/assets/phoenix_kit.jsbundle — no version bump, no Elixir changes. Companion to catalogue PR BeamLabEU/phoenix_kit_catalogue#90 (the item selector's stacked details popup is the shipped case), but both fixes are generic core behaviour.1. Esc closes only the TOP stacked dialog
Chromium groups the close watchers of dialogs whose
showModal()ran without user activation (ours run from LiveView patches), so a single Esc firedcancelon EVERY open dialog — both popups closed when only the top one should.PkDialog._onCancelnow detects an open descendant dialog, keeps itself open, drives the child's server close directly (itsdata-close-event, pushed to the owning component by CID) and closes the child element.preventDefaultalone is not enough — Chromium stops the grouped chain at the first prevented watcher, so the child's own cancel never fires (verified: neither popup closed).2. Component-aware routing
pushEventfrom a hook always lands on the root LiveView, andpushEventTo(element)resolves via the element'sphx-targetattribute (absent on these elements) — so a component-owned modal's close event and a component-owned InfiniteScroll sentinel'sload_moreboth hit the host LV instead of the component. Both now push by numeric CID fromclosest("[data-phx-component]"), falling back to the LV when there is no component ancestor (unchanged behaviour for LV-owned dialogs/sentinels).3. Review hardenings (external panel, 2026-08-31)
close()could double-fire the child's close event on browsers where the child's owncloseecho does fire — the parent now flags the child so its echo is suppressed once.dialog[open], which misses a dialog whoseopenattribute morphdom stripped between a patch and the next_sync— it now matches by the:modaltruth source (isDialogOpenInBrowser).Browser-verified on max-dev: Esc #1 closes only the details popup, Esc #2 the selector; the selector's list state survives.
Notes for release
Hosts get this via
mix phoenix_kit.updaterefreshing the vendoredphoenix_kit.js— the catalogue popup's Esc layering and its legacy sentinel routing degrade gracefully until then.🤖 Generated with Claude Code
https://claude.ai/code/session_018bbQqpwP4bRCYWNtsJAksH