priority binding order is not respected in the Footer

[darrenburns]

The images in #4637 show the problem.

If you have some priority bindings, the order they appear in the BINDINGS is not respected in the Footer widget.

[TomJGooding]

Initially I assumed this would be related to the recent changes to the bindings and/or footer, but actually this seems to predate those changes.

Try running the example app below before and after Textual v0.56.0 (specifically commit 6c459a5).

Unfortunately that's where the trail goes cold, as this changed App.namespace_bindings property no longer exists after dynamic bindings were added in v0.61.0.

from textual.app import App, ComposeResult, RenderResult
from textual.binding import Binding
from textual.widget import Widget
from textual.widgets import Footer


class TestWidget(Widget, can_focus=True):
    DEFAULT_CSS = """
    TestWidget {
        border: tall $primary-background-lighten-3;
        width: 50%;
        height: 50%;
        align: center middle;
    }

    TestWidget:focus {
        border: tall $accent;
    }
    """

    def on_mount(self) -> None:
        self.border_title = self.__class__.__name__

    def render(self) -> RenderResult:
        return ""


class InnerWidget(TestWidget):
    BINDINGS = [
        Binding("3", "overridden", "OVERRIDDEN"),
        Binding("4", "fourth", "Fourth"),
    ]


class OuterWidget(TestWidget):
    BINDINGS = [
        Binding("2", "overridden", "OVERRIDDEN"),
        Binding("3", "third", "Third", priority=True),
    ]

    def compose(self) -> ComposeResult:
        yield InnerWidget()


class PriorityBindingsApp(App):
    CSS = """
    Screen {
        align: center middle;
    }
    """

    BINDINGS = [
        Binding("1", "first", "First"),
        Binding("2", "second", "Second", priority=True),
    ]

    AUTO_FOCUS = "InnerWidget"

    def compose(self) -> ComposeResult:
        yield OuterWidget()
        yield Footer()


if __name__ == "__main__":
    app = PriorityBindingsApp()
    app.run()

[TomJGooding]

This quick and dirty fix seems to work for the example app above and passes (almost) all tests¹.

Unfortunately it looks like no regression test was added with 6c459a5, so I'm not sure whether this also accounts for the issue this commit intended to fix.

diff --git a/src/textual/screen.py b/src/textual/screen.py
index 07dc23571..d746c00fb 100644
--- a/src/textual/screen.py
+++ b/src/textual/screen.py
@@ -324,7 +324,7 @@ class Screen(Generic[ScreenResultType], Widget):
         """
 
         bindings_map: dict[str, ActiveBinding] = {}
-        for namespace, bindings in self._modal_binding_chain:
+        for namespace, bindings in reversed(self._modal_binding_chain):
             for key, binding in bindings.keys.items():
                 action_state = self.app._check_action_state(binding.action, namespace)
                 if action_state is False:

Footnotes

1. The only failing test is test_example_five_by_five from the snapshot tests, where actually I think the footer bindings are in the wrong order? ↩

[TomJGooding]

Unfortunately it looks like no regression test was added with 6c459a5, so I'm not sure whether this also accounts for the issue this commit intended to fix.

Sorry it looks like my quick and dirty fix wouldn't account for issue #4382 this intended to resolve, where the visibility of bindings in the app should be overridden by bindings defined on a screen.

Test App for Issue 4382

from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.screen import Screen
from textual.widgets import Footer, Placeholder


class HideBindingScreen(Screen):
    def compose(self) -> ComposeResult:
        yield Placeholder(self.__class__.__name__)
        yield Footer()


class ShowBindingScreen(Screen):
    BINDINGS = [
        Binding("p", "app.pop_screen", "SCREEN BINDING SHOWN"),
    ]

    def compose(self) -> ComposeResult:
        yield Placeholder(self.__class__.__name__)
        yield Footer()


class ShowBindingApp(App):
    BINDINGS = [
        Binding("p", "pop_screen", "APP BINDING HIDDEN", show=False),
    ]

    def on_mount(self) -> None:
        self.push_screen(HideBindingScreen())
        self.push_screen(ShowBindingScreen())


if __name__ == "__main__":
    app = ShowBindingApp()
    app.run()

[TomJGooding]

What is the 'correct' order for bindings even putting aside those marked as priority?

The screenshots below show the difference before and after Textual v0.56.0.

I think this really needs clarifying first before trying to tackle the priority bindings.

Example code

from textual import __version__
from textual.app import App, ComposeResult, RenderResult
from textual.binding import Binding
from textual.widget import Widget
from textual.widgets import Footer, Header


class TestWidget(Widget, can_focus=True):
    DEFAULT_CSS = """
    TestWidget {
        border: tall $primary-background-lighten-3;
        width: 50%;
        height: 50%;
        align: center middle;
    }

    TestWidget:focus {
        border: tall $accent;
    }
    """

    def on_mount(self) -> None:
        self.border_title = self.__class__.__name__

    def render(self) -> RenderResult:
        return ""


class InnerWidget(TestWidget):
    BINDINGS = [
        Binding("3", "inner_widget_binding", "Inner Widget Binding"),
    ]


class OuterWidget(TestWidget):
    BINDINGS = [
        Binding("2", "outer_widget_binding", "Outer Widget Binding"),
    ]

    def compose(self) -> ComposeResult:
        yield InnerWidget()


class PriorityBindingsApp(App):
    CSS = """
    Screen {
        align: center middle;
    }
    """
    TITLE = f"Textual {__version__}"

    AUTO_FOCUS = "InnerWidget"

    BINDINGS = [
        Binding("1", "app_binding", "App Binding"),
    ]

    def compose(self) -> ComposeResult:
        yield Header()
        yield OuterWidget()
        yield Footer()


if __name__ == "__main__":
    app = PriorityBindingsApp()
    app.run()

[darrenburns]

@TomJGooding Good question. I think it depends very much on context and I don't think there's really a correct answer here.

You could argue that the innermost focused widget's bindings are more contextually relevant since that's what the user has focused, but that's not always necessarily the case.

I think I'd vote for the behaviour in your second screenshot if I had to though.

@willmcgugan what do you think?

[willmcgugan]

Innermost on the left makes sense to me, as it places them in place you would naturally read first. It also makes them less likely to be clipped.

But some apps may have other requirements. I think the ultimate solution would be to expose the sorting to the Footer class, so app authors can decide for themselves.