Skip to content

Introduce ui.popup - #5613

Merged
falkoschindler merged 12 commits into
zauberzeug:mainfrom
evnchn:popup-edit
Jul 16, 2026
Merged

Introduce ui.popup#5613
falkoschindler merged 12 commits into
zauberzeug:mainfrom
evnchn:popup-edit

Conversation

@evnchn

@evnchn evnchn commented Jan 1, 2026

Copy link
Copy Markdown
Collaborator

Motivation

A popup edit is a long-requested feature #1125, previously blocked by #2149.

Now that #2149 is resolved on main, this element can finally see the light.

Implementation

  • The new ui.popup element is based on Quasar's QPopupProxy: placed inside an anchor element, it opens when the user clicks that element and is displayed as a menu or, below the 450px breakpoint, as a dialog.
  • Like ui.menu and ui.dialog, it is a boolean ValueElement. The identical open/close/toggle methods and markdown rendering of these three elements are extracted into a new OpenableElement mixin. As a side effect, ui.dialog gains a documented toggle() method.
  • Screen.should_contain_input now retries when input elements are re-rendered while iterating (StaleElementReferenceException), matching existing retry patterns in screen.py.

Progress

  • The PR title is a short phrase starting with a verb like "Add ...", "Fix ...", "Update ...", "Remove ...", etc.
  • The implementation is complete.
  • This PR does not address a security issue.
  • Pytest has been added.
  • Documentation has been added.
  • No breaking changes to the public API.

@evnchn evnchn added feature Type/scope: New or intentionally changed behavior blocked Status: Blocked by another issue or dependency labels Jan 1, 2026
@evnchn

evnchn commented Jan 2, 2026

Copy link
Copy Markdown
Collaborator Author

question 1 is it better to simply use https://nicegui.io/documentation/menu, since we don't care about the quasar JS-side modal values and stuff.

question 2 do we really don't care about JS-side modal values? It may be useful if we are putting this functionality in a table cell, for example.

@falkoschindler falkoschindler added this to the 3.x milestone Jan 12, 2026
@phifuh

phifuh commented Feb 11, 2026

Copy link
Copy Markdown

Without value sync, it kinda defeats the purpose?

why not create an awaitable singleton pattern? It could be anchored like the ui.menu, or for tables emitting some sort of coordinates?

@evnchn evnchn removed the blocked Status: Blocked by another issue or dependency label Jul 2, 2026
@evnchn

evnchn commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Drafted by Claude Code working with @evnchn — rebased the PR onto main, dropped the stale blocked label, and empirically verified the value round-trip in a browser before replying.

TL;DR:

Rebased & unblocked. Rebased onto main and dropped the stale blocked label — the blocker (#5612 → superseded by #5652, now merged) fixed the underlying remount problem (#2149) this was waiting on. Local gates green.

On your two points:

  1. "Without value sync it kinda defeats the purpose" — I think Write to props beforeUnmount for LOOPBACK = False elements #5652 landing is exactly what closes this gap, and I verified it in a browser. This element deliberately ignores QPopupEdit's own JS value and lets child elements sync through NiceGUI binding. Before Write to props beforeUnmount for LOOPBACK = False elements #5652 that broke when the popup remounted (Re-Opening dialog doesn't update() inputs #2149); now the client-originated value survives a close→reopen cycle. So the value does sync — Python-side through the child, not through Quasar's modal value.
  2. Awaitable singleton pattern — I like it, and it directly targets the table-cell case I raised in my own question above. But I'd frame it as a higher-level convenience layered on top of this primitive rather than a replacement: one shared instance repositioned per cell + value = await popup.edit(...), emitting the cell coordinates as you suggest. This PR stays the minimal container; the awaitable table-helper builds on it later.

Ask: does the primitive-now / awaitable-helper-later split work for you — or do you think the awaitable singleton should be this element from the start? Happy to go the singleton route if that's the more useful shape.

Browser verification of the value round-trip

Ran the doc demo (a ui.input bound to app.storage.client['name'] inside the popup, plus a label bound to the same key) and drove it with Playwright against this branch (de6214a):

Step Action Observed
1 Load page label = NiceGUI User
2 Open popup (on_show) Shown! notify fires; input = NiceGUI User
3 Edit input → Edited In Popup bound label updates live to Edited In Popup (value syncs out)
4 Close via Escape (on_hide) Hidden! notify fires; storage retains Edited In Popup (cancel does not revert, since we don't use Quasar's model value)
5 Reopen popup remounted input shows Edited In Popupvalue persisted across unmount/remount

This is the concrete #2149 scenario that would have shown a stale value before #5652. Scope: proves value persistence across close/reopen with Python-side binding; it does not exercise Quasar's own save/cancel model value (intentionally unused).

Rebase details (range-diff proof)

Old head 73e23d34 → new de6214a. Only two deltas vs the reviewed commit:

  • Registered popup_edit in the lazy-import structure ui.py was refactored into on main (_LAZY_IMPORTS + __all__ + TYPE_CHECKING).
  • Optional[...]X | None with from __future__ import annotations (ruff UP045 is now enforced repo-wide).

Local gates: pre-commit ✅ · mypy ✅ (244 files) · pylint 10.00/10 ✅ · from nicegui import ui; ui.popup_edit resolves ✅.

@falkoschindler falkoschindler added the review Status: PR is open and needs review label Jul 4, 2026
@falkoschindler falkoschindler modified the milestones: Next, 3.15 Jul 4, 2026
@falkoschindler
falkoschindler self-requested a review July 4, 2026 13:56
@evnchn

evnchn commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Drafted by Claude Code, working with @evnchn — built a runnable prototype of the awaitable-singleton shape and verified it in a browser before replying.

TL;DR:

To make the "primitive vs awaitable-singleton" question concrete, I prototyped the awaitable shape alongside the current primitive and ran both.

The load-bearing finding: the awaitable-singleton can't be a thin layer over the primitive — it's genuinely a different component, built on q-menu (not q-popup-edit) with its own value protocol. So this isn't one element with two syntaxes; it's a real fork between a composable container and a purpose-built control.

My lean: ship the container primitive now, add the awaitable cell-editor as a separate convenience later — but the prototype shows that "later" piece is a real second surface, not a free wrapper.

The two shapes, side by side (ran both)

Built PopupEditAwaitable on a prototype branch + a demo running both; Playwright-driven — cell-anchoring exact, await resolves to None on ESC/click-away (no hang).

(a) Container primitive — current PR (b) Awaitable singleton — prototype
Ergonomics Declarative, idiomatic — nest any editor, children bind their own values. Zero new concepts. Imperative, linear — value = await popup.edit(initial, target=…, cell=…). Confirm/cancel/dismiss→value/None built in. Reads like ui.dialog.
Composability High — any element nests; ordinary binding; no invented value protocol. Lower — owns its editor + value protocol; a different editor per cell means extending edit(), not nesting.
Table-cell case Awkward at scale — one popup per anchor → N cells = N elements; "which cell" isn't first-class; no free cancel. Home turf — one instance serves every cell, repositioned per call, carries cell coords to the return + on_submit.
dev API == user API Strong — thin honest wrapper over QPopupEdit-as-container; a user could rebuild it. Mixed — lovely await API, heavier underneath (asyncio.Event, QMenu retargeting); not trivially reconstructable.
Honest rough edges of the awaitable shape
  1. It's really a different componentq-menu, not q-popup-edit. If it wins, "popup_edit" becomes a menu-based cell editor and the Quasar lineage muddies.
  2. Editor extensibility — the prototype hardcodes one ui.input. Accepting an arbitrary child editor reintroduces a value-extraction protocol — it starts pulling the primitive's problem back in.
  3. Concurrency — one shared instance = one edit at a time; overlapping edit() needs a guard (not added).
  4. Not orthogonal — the primitive is a container, the awaitable is a control. "Awaitable from the start" means shipping the control and not the general container, or shipping both.
Run it yourself

Prototype pushed to evnchn:proto-5613-awaitable (branched off this PR's head):

  • Element: nicegui/elements/popup_edit_awaitable.py
  • Side-by-side demo: proto_demo.pypython proto_demo.py (port 8123)

It's a prototype for comparison, deliberately not merge-shaped (hardcoded input editor, no concurrency guard).

The decision — pick a shape:

  • A — Primitive only (current PR): ship ui.popup_edit as the composable container; table-cell editing stays a build-it-yourself pattern. Truest to dev==user API.
  • B — Awaitable-singleton as the element: make popup_edit the await-based cell editor. Best UX for the headline case; heavier, opinionated, q-menu-based — arguably no longer "just a popup edit".
  • C — Both: container primitive now + a separate awaitable cell-editor convenience later. Two surfaces (the prototype shows the awaitable one can't be a thin layer).

Which do you want this to be?

@falkoschindler

Copy link
Copy Markdown
Contributor

Thanks for the thorough side-by-side — and even a runnable prototype! That made the decision easy.

Let's go with option A: ui.popup_edit stays the composable container. It is truest to our "dev API == user API" philosophy, introduces zero new concepts, and keeps the honest QPopupEdit lineage. Your prototype nicely shows that the awaitable shape is really a second, q-menu-based control rather than a thin wrapper — so if the table-cell use case generates enough demand, we can discuss it later as a separate element without blocking this one.

I'll follow up with a regular review covering some API consistency details.

@falkoschindler falkoschindler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finally getting this long-requested element off the ground! The overall shape is right (as discussed above: option A, the composable container): a thin wrapper around QPopupEdit that deliberately leaves value handling to child elements and NiceGUI's data binding, with show/hide events and methods. Registration in ui.py is complete and correctly placed in all three spots, and the demo with doc.reference is in place. A few findings, mostly about API consistency:

Major

  1. show()/hide() break the naming convention — use open()/close()ContextMenu wraps the very same Quasar show/hide methods as open()/close() (see nicegui/elements/context_menu.py), and Menu and Dialog use open()/close() as well. show/hide also collides conceptually with the existing Element.visible/set_visibility(), which mean something different. Renaming later would be a breaking change, so this is worth settling before merge. They should also return Self like their counterparts.

  2. on_show/on_hide should return Self — every other event-registration method (Button.on_click, Notification.on_dismiss, Upload.on_begin_upload, ...) returns Self for chaining; these two return None.

  3. Warn about model-dependent props — Quasar's built-in Set/Cancel buttons (buttons), auto-save, validate, label-set and label-cancel all operate on QPopupEdit's own model value, which this element intentionally doesn't use — a user adding .props('buttons') would get a Cancel that doesn't revert anything. We have a mechanism for exactly this: self._props.add_warning(...) as in ui.menu's touch-position warning (nicegui/elements/menu.py). Please register warnings for these props, pointing users to data binding on the child elements instead.

  4. Missing tests — the checklist already acknowledges this. A Screen test seems feasible: click the label, type into the input, assert the bound storage/label updates, and verify the show/hide events fire. Re-opening the popup and checking the input still reflects the current value would pin down the #2149 scenario from your browser verification as a regression test.

  5. Missing new-element boilerplate — the class docstring needs an *Added in version 3.15.0* note between the description and the :param list (cf. ui.time_input), and ui.popup_edit needs an entry in the alphabetical element list in website/documentation/content/overview.py (right after ui.plotly).

Minor

  1. Documentation section — right now the intro lands between textarea and codemirror in Controls, where it reads like yet another text editor. Since ui.popup_edit is a popup container attached to its parent (just like ui.menu and ui.context_menu), let's move popup_edit_documentation to Page Layout instead, right after context_menu_documentation in website/documentation/content/section_page_layout.py.

  2. Roundabout import pathfrom ..elements.mixins.disableable_element import ... should be from .mixins.disableable_element import ...; this is the only file in nicegui/elements/ using the ..elements. form.

  3. Unnecessary from __future__ import annotations — the project targets Python 3.10+, where X | None annotations work natively; sibling files like menu.py write them without the import.

  4. Docstring/parameter mismatchon_show and on_hide document :param handler: but the parameter is named callback.

  5. Signature formattingon_hide: Handler[UiEventArguments] | None = None,) -> None: glues a trailing comma to the closing paren; house style puts ) -> None: on its own line (cf. MenuItem.__init__) or drops the comma.

  6. Docstring wording — "NOTE: We only use the popup edit as a container for other elements." renders on the website, where "we" reads oddly; something like "This element only serves as a container for other elements; ..." would fit the docs voice better (cf. the "Note:" style in ui.dialog's docstring).

Question

  1. PR motivation references #5612, which was closed unmerged. Since #2149 is resolved on main anyway, could you update the Motivation to point at what actually unblocked this, so the history reads correctly?

- Rename show()/hide() to open()/close() and return Self, matching
  ui.context_menu and ui.menu; on_show/on_hide now return Self too.
- Warn (and strip) on QPopupEdit's model-dependent props (buttons,
  auto-save, validate, label-set, label-cancel) via _props.add_warning,
  pointing users to child data binding.
- Add a Screen test covering open/edit/close events, live binding, and
  re-open reflecting the current value (the zauberzeug#2149 scenario).
- Docstring: add "Added in version 3.15.0", reword the container note,
  fix the :param name, drop the unneeded __future__ import, use the
  local mixins import path, and fix the signature formatting.
- Move the demo from Controls to Page Layout (after context_menu) and
  list ui.popup_edit in the overview map.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@evnchn

evnchn commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Drafted by Claude Code working with @evnchn — review addressed, all findings resolved, CI green.

TL;DR:

All 5 Major, 6 Minor, and the Question are resolved in the follow-up commit, and CI is green (including the new Screen test on Linux).

Ready for re-review.

Major 1 & 2 — open()/close()/on_*() naming + Self

show()/hide() are now open()/close(), both returning Self, matching ContextMenu.open/close (which wrap the same Quasar show/hide) and Menu. on_show/on_hide now return Self for chaining like Button.on_click. The demo's button now calls popup.open (and its label reads "Force open popup edit").

Major 3 — warnings for model-dependent props

Registered in __init__ via self._props.add_warning(...) (same mechanism as ui.menu's touch-position and ui.tree's default-expand-all), for buttons, auto-save, validate, label-set, label-cancel.

Confirmed (ran it) that the warning fires and the prop is stripped:

>>> pe = ui.popup_edit(); pe.props('buttons')
The prop "buttons" operates on QPopupEdit's own model value, which `ui.popup_edit`
does not use. Use data binding on the child elements instead.
>>> 'buttons' not in pe._props
True
Major 4 — tests (+ CI evidence)

Added tests/test_popup_edit.py. It clicks the anchor label to open the popup (asserts the show event fired), types into the bound child input (asserts the bound data and the live label update), presses Escape to close (asserts the hide event fired and that cancel did not revert — the whole point of not using QPopupEdit's model), then re-opens and asserts the input still reflects the current value — the #2149 regression scenario.

  • Local: 1 passed in 5.39s.
  • CI (quick-test / 3.10 / pytest, Linux): pass in 23m19s — so it's not a macOS-only green. check / mypy, check / pre-commit, check / pylint all pass too.

This proves the interaction and the binding round-trip end-to-end; it does not exercise every Quasar prop, which is why the model-dependent props are handled by warnings (Major 3) rather than tests.

Major 5 & Minor 1–6 — boilerplate, docs, style
  • *Added in version 3.15.0* note added between the description and the :param list (milestone is 3.15).
  • ui.popup_edit added to the alphabetical element list in overview.py, right after ui.plotly.
  • Demo moved from Controls to Page Layout, right after context_menu_documentation.
  • Import path ..elements.mixins.disableable_element.mixins.disableable_element.
  • Dropped the unneeded from __future__ import annotations.
  • :param handler::param callback: (matches the actual parameter name).
  • Signature: ) -> None: now on its own line.
  • Docstring NOTE reworded to "This element only serves as a container for other elements; …".
Question — Motivation reference

Updated the PR Motivation to point at #2149 (resolved on main) as the actual unblocker, rather than the closed-unmerged #5612, so the history reads correctly.

Review provenance

Fixes verified locally (pre-commit / mypy ./nicegui / pylint), then run past a different-lineage skeptic (Codex, prompted to refute) before pushing — it independently re-confirmed the add_warning(name, message) semantics against nicegui/props.py, that the Screen test is non-vacuous, and the convention match vs context_menu.py; it found no blockers. Then CI was confirmed green on the pushed commit before this "ready" claim.


Changelog:

  • 2026-07-16: edited from the initial post — added the top-line disclosure I had omitted, folded in the now-confirmed CI-green result and a "ready for re-review", and added the review-provenance block.

@falkoschindler
falkoschindler self-requested a review July 16, 2026 06:05
falkoschindler and others added 4 commits July 16, 2026 13:35
Focus on the core use case: an autofocused input bound to storage,
closing on Enter. Drop the on_show/on_hide notifications and the
extra "force open" button.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Match the open()/close() methods and avoid mixing two vocabularies for the same state transition; the raw Quasar "show"/"hide" events remain reachable via .on().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Use screen.wait_for() and should_contain_input() instead of screen.wait(0.5) so each step waits exactly as long as needed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pure container based on QPopupEdit required neutering all of its edit features and added little over the existing ui.menu. QPopupProxy is built for exactly this job: it renders a menu on wide screens and a dialog below its breakpoint (default 450px), accepting the props of both. As a ValueElement[bool] the element inherits value binding and open()/close()/toggle() from the same family as ui.menu and ui.dialog, replacing the hand-rolled methods, event handlers, and prop warnings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@falkoschindler

Copy link
Copy Markdown
Contributor

Sorry for reshaping this PR yet again, @evnchn — but working with the element some more raised a question we should settle before merge: is QPopupEdit even the right base?

The observation: the current element deliberately bypasses everything QPopupEdit is actually about — model value, Set/Cancel buttons, validate, auto-save, ESC-revert; we even warn about five of its props now. What remains is "a padded QMenu that opens on anchor click". And that is almost exactly what ui.menu already does on main today: with ui.label(...): with ui.menu(): ui.input(...) works. A new element should offer something ui.menu can't.

The alternative: Quasar has a component for precisely the "just a popup container" job: QPopupProxy. It renders a QMenu on wide screens and a full QDialog below a breakpoint (default 450px), accepting and passing through the props of both — so an inline editor becomes a proper dialog on mobile for free. That's a genuine feature beyond ui.menu, and it means the element isn't named after a use case it doesn't implement.

I pushed this reshape to the branch: ui.popup, based on q-popup-proxy, implemented as a ValueElement[bool] — structurally a twin of Menu:

  • value binding plus open()/close()/toggle() come from the family; the hand-rolled open/close, on_open/on_close, and the prop warnings from the last review round all disappear (that work was solid — it's just no longer needed, sorry!)
  • demo and test carry over nearly unchanged; the test now also covers the server-side value round-trip and programmatic open/close
  • browser-verified: menu at 1000px, dialog at 400px; binding and server-triggered close work in both render modes

One Quasar behavior worth knowing rather than fixing: QPopupProxy force-sets cover/max-height when its first child is a QDate/QTime/QCarousel/QColor — which conveniently makes with ui.popup(): ui.date() behave like a proper date-picker popup out of the box.

If you see a problem with the direction, happy to discuss — the popup_edit shape is one revert away.

@falkoschindler falkoschindler changed the title Introduce ui.popup_edit Introduce ui.popup Jul 16, 2026
falkoschindler and others added 2 commits July 16, 2026 15:26
The helper iterates all input elements and reads their values; if the UI re-renders in between (like QPopupProxy remounting its content on reopen), get_attribute() raises StaleElementReferenceException and fails the test. Retry within the deadline instead, like Screen.wait_for already does.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QPopupProxy re-mounts its teleported input on open, so a handle captured
after open (or across a reopen) goes stale before send_keys. Re-find on
each interaction, mirroring the retry Screen.should_contain_input already does.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@evnchn

evnchn commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed by Claude Code on @evnchn's behalf.

TL;DR: Pushed a follow-up (e7d66cd) for the residual CI flake — the test reused selenium handles across QPopupProxy's on-open input remount; it now re-finds the input on each interaction. Your should_contain_input fix + this = 60/60 locally (CI will confirm).

Root cause & evidence

Root cause (instrumented, not guessed): a MutationObserver on the child <input> shows Quasar re-creates the node on every open (add→remove→add during the open transition; only ever one present at a time), and removes it on close. So a handle grabbed right after open, or held across a close→reopen, goes stale before send_keys. Confirmed ui.menu behaves identically → generic teleport churn, not QPopupProxy-specific and not introduced by this PR; the binding is not involved (no remount while typing).

Why your fix alone wasn't enough: should_contain_input was one racy spot, but the dominant one is the raw element.send_keys(Keys.ESCAPE) after reopen — on 5aa317c8 that still flaked 1/15 there.

Fix: a small send_to_name() that re-finds the input and retries on StaleElementReferenceException, mirroring the retry your should_contain_input / Screen.wait_for already use. Test-only, no product change.

Repro rates (single test in a loop; macOS, chromedriver 150):

state result
before any fix ~2/8 fail (≈20%)
5aa317c8 (your fix, original test) 1/15 fail (raw-handle send_keys)
+ this commit 0/60

pre-commit / mypy / pylint(./nicegui) unaffected.

A MutationObserver in the live browser shows QPopupProxy does not remount its content once opening has settled, so re-finding the input on every interaction is unnecessary. Instead, use the stale-tolerant should_contain_input() as a settle-barrier before send_keys and run the final close/open cycle from the server, where no stale handle can exist. Verified with 60 consecutive green runs; the test also gets ~2x faster.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@falkoschindler

Copy link
Copy Markdown
Contributor

Thanks for the instrumented flake analysis, @evnchn — your add→remove→add observation during the open transition was the missing piece. Building on it, I simplified the test once more in 8ef7ade, superseding the send_to_name helper.

Why: our two measurements are consistent once you split them by phase — the input churns during the open transition (your data), but is stable once the popup has settled (mine, via MutationObserver after open). So instead of retrying every interaction, the test now (a) uses the stale-tolerant should_contain_input() as a settle-barrier before any send_keys and (b) runs the final close/re-open cycle from the server, where no client handle exists at all. Same coverage (both binding directions, ESC, the #2149 reopen scenario, programmatic open/close), 21 lines less, and ~2× faster since it never idles in retry loops. Verified with 60/60 consecutive local runs — matching your sample size; at the 1/15 flake rate you measured for raw send_keys, 60 clean runs would have a ~1.6% probability.

One housekeeping note: you may have noticed a force-push (889f7228ef7ade). That wasn't planned — a late cleanup (dropping the popups list for a nonlocal) got amended into an already-pushed commit by mistake. Only my own test commit was rewritten; your commits are untouched. Sorry.

@falkoschindler falkoschindler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a final touch, ec2c17e extracts the now-triplicated open()/close()/toggle() and markdown handling of Menu, Dialog, and the new Popup into a shared OpenableElement mixin — with ui.dialog gaining a documented toggle() method as a small bonus. I also refreshed the PR description to match the final shape.

Ready to merge. Thanks for pushing this through all the reshapes, @evnchn!

@falkoschindler
falkoschindler enabled auto-merge July 16, 2026 17:53
@falkoschindler
falkoschindler added this pull request to the merge queue Jul 16, 2026
Merged via the queue into zauberzeug:main with commit d1cf251 Jul 16, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Type/scope: New or intentionally changed behavior review Status: PR is open and needs review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants