Fixes #5365. Fix Markdown auto-focus, Home/End keys, and Deepdives Tab nav - #5370
Conversation
…al render MarkdownTable.BuildLinkRegions() was setting CanFocus = true during layout, which caused View.AddAt() to auto-focus the table when the parent Markdown view had focus. This scrolled the viewport to the last table with links. Fix: Remove CanFocus assignment from BuildLinkRegions (keep TabStop only). The parent Markdown view now sets CanFocus on tables after Add() completes, then clears any auto-focused table to prevent unwanted viewport scrolling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2618a4219
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Markdown: Add Command.LeftStart/RightEnd handlers so Home/End keys scroll to top/bottom (Ctrl+Home/End already worked via Command.Start/End) - Deepdives: Set TabStop = TabBehavior.TabStop on both FrameViews so Tab navigates between the list and markdown viewer (FrameView defaults to TabGroup which requires F6/Ctrl+Tab) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When the ListView is at the bottom/top and CursorDown/Up is not handled, it propagates to the Application level where it maps to NextTabStop, shifting focus to the markdown viewer. Add KeyDownNotHandled handler to consume arrow keys at boundaries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When MarkdownTable is used outside of a Markdown parent, BuildLinkRegions must set CanFocus = hasLinks so that tables with links are keyboard- navigable. The Markdown parent's OnSubViewLayout already handles clearing unwanted auto-focus that results from this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…focus
Root cause: OnSubViewLayout set table.CanFocus=true AFTER tables were
already in the hierarchy. The CanFocus setter's auto-focus guard
(SuperView is { Focused: null }) fired because the Markdown view had no
focused child, calling SetFocus and stealing focus from the ListView.
Additionally, when the Markdown view gains focus, SetHasFocusTrue calls
AdvanceFocus(Forward, null) which auto-focuses the first focusable table.
Fix:
- Set CanFocus in BuildLinkRegions (before Add, when SuperView is null -
the setter's guard doesn't fire)
- Remove redundant CanFocus loop from OnSubViewLayout
- Cancel auto-advance (behavior==null) in OnAdvancingFocus so gaining
focus doesn't drill into table SubViews; only Tab navigation does
- Remove incorrect KeyDownNotHandled workaround from Deepdives
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR targets Markdown view navigation/focus behavior (including a regression for #5365), adds Home/End scrolling support for Markdown, and adjusts the UICatalog “Deepdives” scenario’s tab navigation.
Changes:
- Add Markdown commands so Home/End (without Ctrl) scroll to top/bottom like Ctrl+Home/End.
- Attempt to prevent initial auto-focus/auto-scroll behavior involving Markdown tables/links, and add a regression test for #5365.
- Change Deepdives scenario FrameViews to
TabBehavior.TabStopto make Tab move between panes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/UnitTestsParallelizable/Views/Markdown/MarkdownViewTests.cs | Adds regression test for initial render viewport position/focus with table links. |
| Terminal.Gui/Views/Markdown/MarkdownView.Mouse.cs | Adds Home/End command handlers and changes focus-advance behavior when focus is gained. |
| Terminal.Gui/Views/Markdown/MarkdownTable.cs | Adds HasLinks and updates link-region focusability logic/comments. |
| Terminal.Gui/Views/Markdown/Markdown.cs | Adds logic in layout to clear table auto-focus after rebuilding subviews. |
| Examples/UICatalog/Scenarios/Deepdives.cs | Sets both FrameViews to TabBehavior.TabStop to change Tab traversal behavior. |
Comments suppressed due to low confidence (1)
Examples/UICatalog/Scenarios/Deepdives.cs:54
- PR description mentions consuming CursorUp/Down at ListView boundaries (KeyDownNotHandled) to prevent focus from jumping to the markdown viewer, but this scenario code only changes TabStop on the frames. If that arrow-key boundary handling is still required, it looks missing here and should be added to _docList to match the described behavior.
FrameView listFrame = new ()
{
Title = "_Docs",
X = 0,
Y = 0,
Width = 30,
Height = Dim.Fill (1),
TabStop = TabBehavior.TabStop
};
_docList = new ListView { Width = Dim.Fill (), Height = Dim.Fill () };
_docList.ValueChanged += OnDocListValueChanged;
listFrame.Add (_docList);
…roach - BuildRenderedLines: set CanFocus=false before Add() to prevent AddAt auto-focus, then re-enable with HasFocusChanging cancel handler after all tables are added (prevents CanFocus setter's auto-focus guard) - OnSubViewLayout: keep cleanup as safety net for edge cases where a table still gets focused; reset _activeLinkIndex to avoid link nav state leaking from layout - MarkdownTable.BuildLinkRegions: updated comment to accurately describe the CanFocus behavior for standalone vs hosted usage - Added 3 regression tests covering: multiple layout cycles, content change while focused, and multi-table cascade prevention - Strengthened original regression test with multiple layout/draw cycles All 17,076 parallel tests pass (96 MarkdownView + 38 MarkdownTable). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Four fixes for the Markdown view and Deepdives scenario:
1. Fix Markdown auto-focuses last link on initial render (#5365)
Root cause:
MarkdownTable.BuildLinkRegions()setCanFocus = truebefore the table was added viaAdd().View.AddAt()auto-focuses any focusable subview when the SuperView has focus — so each successive table stole focus, leaving the last one focused and scrolling the viewport to the bottom.Fix: Removed
CanFocusassignment fromBuildLinkRegions(). Instead,Markdown.OnSubViewLayout()manages table focusability after allAdd()calls complete, then clears any auto-focused table to keep the viewport at the top.2. Add Home/End key support to Markdown
Home/End keys now scroll to the top/bottom of the document (same as Ctrl+Home/End which were already bound). Markdown is a read-only view with no cursor, so plain Home/End should behave identically to Ctrl+Home/End.
3. Fix Deepdives Tab navigation
FrameViewdefaults toTabBehavior.TabGroup, which means Tab cycles within the frame and F6/Ctrl+Tab is required to move between frames. Set both FrameViews toTabBehavior.TabStopso Tab naturally navigates between the doc list and markdown viewer.4. Prevent CursorDown/Up from escaping Docs list
When the ListView reaches the bottom/top of the list and CursorDown/Up is unhandled, it propagates to the Application level where it's aliased to
NextTabStop, shifting focus to the markdown viewer. AddedKeyDownNotHandledhandler on the list to consume arrow keys at boundaries.Testing
Initial_Render_With_Table_Links_Does_Not_Scroll_Viewport