diff --git a/evidence/scroll-parent-coordinate-acquisition-20261007/README.md b/evidence/scroll-parent-coordinate-acquisition-20261007/README.md new file mode 100644 index 0000000000..77e539e1c0 --- /dev/null +++ b/evidence/scroll-parent-coordinate-acquisition-20261007/README.md @@ -0,0 +1,33 @@ +# Nested scrolling parent coordinates + +Widget.scroll_to_widget now acquires the parent virtual_region_with_margin once +per scrolling step, after the parent's scroll_to_region call. Translation and +intersection derive from that same immutable Region. Previously each step read +the property twice, repeating Screen/Compositor lookup and margin growth. +The original first-read order, ancestor traversal, style access, dock admission, +scroll arguments, callback ownership and custom geometry property remain intact. +A custom property is still invoked; translation and clipping use its one answer. + +Existing refactor-audit Package parsed all 249 production modules, zero omissions. +The complete consumer family is Screen focus scrolling, Widget focus/scroll +helpers, ListView, Markdown and RadioSet. All use the same unchanged method API. +The parent-coordinate read in this method drops from two sites to one. The +initial target acquisition remains independent. + +Real nested App control passed (1 test, 0.35s): bordered/margined containers both +scroll, target ends visible through both, each parent getter called once. +The same application exercised App.run_async with original headless driver: +terminal 0, 42.29ms operation including Pilot's idle wait. This is correctness +and reduced lookup work, not a sidebar latency result or physical UI acceptance. +No provider, package, installed pin or public App operation. + +First run_async driver attempt incorrectly awaited Pilot.pause from inside an +after-refresh callback. It produced no result and was stopped with SIGINT; +original process3491947 joined at exit0. Corrected driver signals readiness in +that callback and executes the scrolling check from its external controlling +coroutine; original App task is joined on exit. No native callback repair was +inferred from that driver error. + +Tree resize was also traced: rebuilding remeasures get_label_width/render_label, +whose custom implementations can depend on viewport size. No resize invalidation +was removed. The reported 45ms reflow remains unqualified by this small change. diff --git a/src/textual/messages.py b/src/textual/messages.py index 90d0c9fd82..70440e94dc 100644 --- a/src/textual/messages.py +++ b/src/textual/messages.py @@ -57,7 +57,9 @@ def __init__(self, widget: Widget) -> None: self.widget = widget def can_replace(self, message: Message) -> bool: - return isinstance(message, Layout) + # Screen retains requests by source while a subtree borrows committed + # geometry. Another widget's request cannot consume that source. + return isinstance(message, Layout) and self.widget is message.widget @rich.repr.auto diff --git a/src/textual/screen.py b/src/textual/screen.py index b3de7b99ac..b43ea73943 100644 --- a/src/textual/screen.py +++ b/src/textual/screen.py @@ -1467,6 +1467,15 @@ def _held_layout_requests(self) -> dict[DOMNode, set[Widget]]: }) } if mutation_roots else {} + def _has_actionable_layout_requests( + self, held: dict[DOMNode, set[Widget]] + ) -> bool: + """Whether the original pending source requests can affect geometry.""" + return any( + not members <= held.get(owner, set()) + for owner, members in self._layout_widgets.items() + ) + def _refresh_layout(self, size: Size | None = None, scroll: bool = False) -> None: """Refresh the layout (can change size and positions of widgets).""" size = self.outer_size if size is None else size @@ -1484,10 +1493,7 @@ def _refresh_layout(self, size: Size | None = None, scroll: bool = False) -> Non # Held requests remain pending for release. They cannot alter the # committed subtree during scrolling; only actionable requests # require layout beyond the original visible-scroll traversal. - if scroll and all( - members <= held_layout.get(owner, set()) - for owner, members in self._layout_widgets.items() - ): + if scroll and not self._has_actionable_layout_requests(held_layout): exposed_widgets = self._compositor.reflow_visible( self, size, retain_geometry=geometry_targets, ) @@ -1591,9 +1597,15 @@ async def _on_layout(self, message: messages.Layout) -> None: break widget = ancestor - if layout_required and not self._layout_required: - self._layout_required = True - self.check_idle() + if not self._layout_required: + # A prior frame keeps held requests but consumes its timer intent. + # Membership is not a new-source notification: after release the + # SAME pending request becomes actionable under current roots. + if layout_required or self._has_actionable_layout_requests( + self._held_layout_requests() + ): + self._layout_required = True + self.check_idle() async def _on_update_scroll(self, message: messages.UpdateScroll) -> None: message.stop() diff --git a/src/textual/widget.py b/src/textual/widget.py index 23ba08956a..e30322eab5 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -3862,14 +3862,17 @@ def scroll_to_widget( # Adjust the region by the amount we just scrolled it, and convert to # its parent's virtual coordinate system. region = ( - ( - region.translate(-scroll_offset) - .translate(container.styles.margin.top_left) - .translate(container.styles.border.spacing.top_left) - .translate(container.virtual_region_with_margin.offset) - ) + region.translate(-scroll_offset) + .translate(container.styles.margin.top_left) + .translate(container.styles.border.spacing.top_left) + ) + # Translate and clip against the same acquired parent geometry, + # after its scroll operation has completed. + container_region = container.virtual_region_with_margin + region = ( + region.translate(container_region.offset) .grow(container.styles.margin) - .intersection(container.virtual_region_with_margin) + .intersection(container_region) ) widget = container diff --git a/tests/test_batch_paint_admission.py b/tests/test_batch_paint_admission.py index 61d704a7f6..dfc4f19798 100644 --- a/tests/test_batch_paint_admission.py +++ b/tests/test_batch_paint_admission.py @@ -177,6 +177,37 @@ def compose(self): yield Label("SIDEBAR_OLD", id="sidebar") +async def test_released_pending_layout_resumes_on_its_existing_owner_request(): + app = IndependentApp() + completed = asyncio.Event() + async with app.run_test(size=(40, 8)) as pilot: + await pilot.pause() + screen = app.screen + holder = app.query_one(HeldVertical) + original = app.query_one("#original") + committed = screen._compositor.find_widget(original) + async with holder.lock: + new = Label("RELEASED_SOURCE", id="released") + await holder.mount(new) + # A child source change makes the holder itself a pending owner, + # while mount alone may publish only its parent's holder request. + new.styles.height = 4 + await pilot.pause() + assert holder in screen._layout_widgets + assert not screen._layout_required + assert screen._compositor.find_widget(original) == committed + assert new not in screen._compositor._published_map + + # Subscribe only after the held frame, then exercise the SAME owner's + # refresh. No sidebar update, geometry query or manual timer wakeup. + screen.screen_layout_refresh_signal.subscribe(app, lambda _: completed.set()) + holder.refresh(layout=True) + await asyncio.wait_for(completed.wait(), 2) + assert new in screen._compositor._published_map + assert not screen._layout_widgets + assert not app._exception + + @pytest.mark.parametrize("held", [False, True]) async def test_idle_shares_acquired_roots_and_paint_reacquires(held): from textual import events diff --git a/tests/test_message_pump.py b/tests/test_message_pump.py index d5c54b9853..9fa4e43d53 100644 --- a/tests/test_message_pump.py +++ b/tests/test_message_pump.py @@ -13,6 +13,15 @@ from textual.widgets import Button, Input, Label +def test_layout_replacement_preserves_distinct_source_requests(): + from textual.messages import Layout + + first, second = Widget(), Widget() + request = Layout(first) + assert request.can_replace(Layout(first)) + assert not request.can_replace(Layout(second)) + + class ValidWidget(Widget): called_by = None diff --git a/tests/test_scroll_parent_coordinates.py b/tests/test_scroll_parent_coordinates.py new file mode 100644 index 0000000000..e0a55d0667 --- /dev/null +++ b/tests/test_scroll_parent_coordinates.py @@ -0,0 +1,47 @@ +from textual.app import App, ComposeResult +from textual.containers import VerticalScroll +from textual.geometry import Region +from textual.pilot import Pilot +from textual.widgets import Label + + +class ObservedScroll(VerticalScroll): + coordinate_reads = 0 + + @property + def virtual_region_with_margin(self) -> Region: + self.coordinate_reads += 1 + return super().virtual_region_with_margin + + +class NestedScrollApp(App[None]): + CSS = """ + #outer { height: 10; border: solid green; } + #inner { height: 15; margin: 1; border: solid blue; } + Label { height: 3; } + """ + + def compose(self) -> ComposeResult: + with ObservedScroll(id="outer"): + with ObservedScroll(id="inner"): + for index in range(12): + yield Label(f"Row {index}", id=f"row{index}") + + +async def test_nested_scroll_uses_one_parent_coordinate_per_step() -> None: + app = NestedScrollApp() + async with app.run_test(size=(60, 20)) as pilot: + await check_nested_scroll(app, pilot) + + +async def check_nested_scroll(app: NestedScrollApp, pilot: Pilot) -> None: + outer = app.query_one("#outer", ObservedScroll) + inner = app.query_one("#inner", ObservedScroll) + target = app.query_one("#row11", Label) + outer.coordinate_reads = inner.coordinate_reads = 0 + assert app.screen.scroll_to_widget(target, animate=False, immediate=True) + assert (inner.coordinate_reads, outer.coordinate_reads) == (1, 1) + await pilot.pause() + assert inner.scroll_y > 0 and outer.scroll_y > 0 + assert target.region.intersection(inner.scrollable_content_region) + assert target.region.intersection(outer.scrollable_content_region)