Point Dialog::important_area at the focused button - #884
Merged
Merged
Conversation
Dialog::important_area always returned the content's area, so a ScrollView wrapping a tall Dialog never scrolled far enough to reveal the buttons once focus moved to them. Match on the focus instead: content focus keeps the existing calculation, button focus delegates to the focused button and translates by its recorded offset, the same shape FixedLayout and LinearLayout use. Fixes gyscos#813
Owner
|
Hi, and thanks for the work! Looks good! |
gyscos
approved these changes
Sep 9, 2026
gyscos
marked this pull request as ready for review
September 9, 2026 14:00
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.
Fixes #813.
Dialog::important_area()always returned the content's area, even when a buttonhad focus - there was a
// TODO: if a button is focused, return the button position instead.sitting on that line. So aScrollViewwrapping a tallDialogscrolled to the content and stopped, and the buttons below stayedunreachable once focus moved to them.
Now it matches on
self.focus: content focus keeps the existing calculation,button focus asks the focused button for its important area and translates it by
the offset the button already records. That is the same shape
FixedLayout,LinearLayoutandListViewuse for their focused child.The test lays out and draws a 20x5 dialog with two buttons and asserts the
important area for all three focus states. Coordinates are hardcoded rather than
derived from
offset/size, so it cannot just restate the implementation - Ichecked it fails on master and on the plausible wrong versions (hardcoded button
index; adding
borders + paddingto the button arm by symmetry with the contentarm).
Two things worth flagging:
draw_buttons(), not inlayout(), so beforethe first draw they are still zero. In practice focus only reaches a button
after an event, and
run()refreshes before the firststep(), so the offsetsare current by then; the degenerate case is a scroll to the top for one frame,
not a panic. It is the same contract
check_focus_grab()already relies on formouse hit-testing. Moving the button positioning into
layout()would removethe hazard, but it duplicates the alignment math and felt like more than this
fix needed - say the word if you would prefer it.
excluded from the important area, which is consistent with the content arm but
means the bottom border row can stay clipped. Happy to include it if that is
what you want.
cargo fmt,clippyand the full test suite are clean. Two files,theme.rsand
backends/curses/n.rs, are already rustfmt-dirty on master; I left thosealone.
Written with AI assistance; I reproduced the bug, verified the fix and ran the
tests myself.