Fix priority binding order in Footer on key collision (#4639)#6655
Open
gianaae-ux wants to merge 1 commit into
Open
Fix priority binding order in Footer on key collision (#4639)#6655gianaae-ux wants to merge 1 commit into
gianaae-ux wants to merge 1 commit into
Conversation
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.
Link to issue or discussion
Fixes #4639 (original report with screenshots: #4637)
Summary
When an app-level priority binding wins a key collision against a focused widget's binding, the Footer displays it out of order. This restores the app's declared binding order for that case with a one-line fix.
Problem
Priority bindings don't respect their declared order in the Footer when they win a key collision against a focused widget's binding. See #4639 for the bug report; the original report in #4637 (whose screenshots show the problem) is what motivated the suggestion to use priority=True in the first place.
Root cause
Screen.active_bindings (src/textual/screen.py) merges bindings from the focused widget, screen, and app into a single insertion-ordered dict. On a key collision where an app priority binding beats a non-priority one, it reassigns the key:
if existing_key_and_binding := bindings_map.get(key):
# This key has already been bound
# Replace priority bindings
if (
binding.priority
and not existing_key_and_binding.binding.priority
):
bindings_map[key] = ActiveBinding(
namespace, binding, enabled, binding.tooltip
)
In Python, assigning to an existing dict key updates its value but keeps the key at its original insertion position — reassignment never moves a key. So the winning priority binding inherits the position of the losing binding (the focused widget's, which was inserted earlier during the merge) instead of the position implied by the app's declared order. Since the Footer renders keys in bindings_map iteration order, the priority binding shows up in the wrong slot.
Fix
Delete the key before re-inserting it, so the winning binding lands at its correct (later) position in iteration order:
del bindings_map[key]
bindings_map[key] = ActiveBinding(
namespace, binding, enabled, binding.tooltip
)
The whole fix is the single added line del bindings_map[key].
Testing
Added tests/test_binding_priority_collision.py, which reproduces the actual collision: a focused widget and an app-level priority=True binding competing for the same key (f). The app declares its priority bindings in the order q, j, f, n, and the test asserts the winning f binding ends up after j in active_bindings (keys.index("f") > keys.index("j")). I confirmed the test fails against the pre-fix code and passes with the fix — not just that it passes now. I also ran the full suite (poetry run pytest tests/ -n 16 --dist=loadgroup) with no regressions.
Screenshots:
bug reproduction — priority binding shown out of order in the Footer

fixed behavior — priority binding shown in declared order
