Skip to content

Remove document key listeners on keyboard unmount - #6190

Open
evnchn wants to merge 3 commits into
zauberzeug:mainfrom
evnchn:midnight/keyboard-4
Open

Remove document key listeners on keyboard unmount#6190
evnchn wants to merge 3 commits into
zauberzeug:mainfrom
evnchn:midnight/keyboard-4

Conversation

@evnchn

@evnchn evnchn commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Motivation

Add a ui.keyboard inside a conditional/refreshable UI or a sub_page, then navigate away or toggle it off and on repeatedly. Every mount adds N document-level listeners (default keydown+keyup = 2) that are never removed; each keypress after teardown still runs the orphaned handlers and emits key events through disconnected components. Listeners and component instances accumulate without bound.

Minimal reproduction (nicegui/elements/keyboard.js:4) — the exact test/script that was run to reproduce it:

from nicegui import ui
from nicegui.testing import Screen


def test_keyboard_listener_leak(screen: Screen):
    """ui.keyboard adds document keydown/keyup listeners in mounted() but never removes them on unmount.

    We monkey-patch document.addEventListener/removeEventListener (in <head>, before Vue mounts) to
    keep a running count of live 'keydown' listeners. Mounting/unmounting the keyboard N times should
    leave the count unchanged if listeners are cleaned up. With the bug, each mount leaks one.
    """
    @ui.page('/')
    def page():
        ui.add_head_html('''
            <script>
                window.__kd = 0;  // net live 'keydown' listeners on document
                const _add = document.addEventListener.bind(document);
                const _rem = document.removeEventListener.bind(document);
                document.addEventListener = function(type) {
                    if (type === 'keydown') window.__kd++;
                    return _add.apply(document, arguments);
                };
                document.removeEventListener = function(type) {
                    if (type === 'keydown') window.__kd--;
                    return _rem.apply(document, arguments);
                };
            </script>
        ''')
        show = {'v': False}

        @ui.refreshable
        def area():
            if show['v']:
                ui.keyboard(on_key=lambda e: None)
        area()

        def toggle():
            show['v'] = not show['v']
            area.refresh()
        ui.button('TOGGLE', on_click=toggle)

    screen.open('/')
    screen.wait(1.0)
    baseline = screen.selenium.execute_script('return window.__kd')

    N = 3
    for _ in range(N):
        screen.click('TOGGLE')   # mount keyboard
        screen.wait(0.4)
        screen.click('TOGGLE')   # unmount keyboard
        screen.wait(0.4)

    leaked = screen.selenium.execute_script('return window.__kd') - baseline
    assert leaked == 0, f'BUG REPRODUCED: {leaked} orphaned document keydown listeners after {N} mount/unmount cycles (expected 0)'

→ > assert leaked == 0, f'BUG REPRODUCED: {leaked} orphaned document keydown listeners after {N} mount/unmount cycles (expected 0)'

Implementation

Added unmounted() to keyboard.js storing [event,handler] pairs at mount and removing them on unmount (mirrors fullscreen.js).

Verification
  • fail-first verified: FAIL assert 4==0 on origin/main source, PASS with fix.
  • Local gates: pre-commit · mypy ./nicegui · pylint 10.00/10 · pytest (touched) — all green.

Progress

  • The PR title is a short phrase starting with a verb.
  • The implementation is complete.
  • This PR does not address a security issue.
  • Pytests have been added/updated.
  • Documentation is not necessary.
  • No breaking changes to the public API.

evnchn and others added 2 commits July 25, 2026 00:14
@evnchn @claude
Remove dead no-parent-event prop from the date picker (#218)
64e78ad 
DateInput applied .props('no-parent-event') to the inner QDate, but
Quasar 2.18.5's QDate has no such prop (noParentEvent belongs to the
anchor-props mixin behind QMenu/QTooltip). It was an inert no-op
attribute; removing it changes no behavior (open/close is driven by the
parent QMenu).

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@evnchn @claude
Remove document key listeners when keyboard unmounts
64f941b 
`ui.keyboard` added document-level keydown/keyup listeners in `mounted()`
but never removed them, so mounting inside a refreshable/sub_page and
navigating away leaked N listeners per mount. Store the handler refs and
remove them in `unmounted()`, mirroring `fullscreen.js`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@evnchn evnchn mentioned this pull request Jul 25, 2026
Closed
6 tasks
@evnchn
evnchn marked this pull request as ready for review July 25, 2026 06:17
@evnchn evnchn added bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review labels Jul 26, 2026
@falkoschindler falkoschindler self-assigned this Jul 26, 2026
@falkoschindler
falkoschindler self-requested a review July 26, 2026 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@falkoschindler falkoschindler Awaiting requested review from falkoschindler

At least 1 approving review is required to merge this pull request.

Assignees

@falkoschindler falkoschindler

Labels

bug Type/scope: Incorrect behavior in existing functionality review Status: PR is open and needs review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@evnchn @falkoschindler