Skip to content

Property grid: Tab/Shift+Tab traverses to next/prev property (fixes #5697)#6529

Open
heffneil wants to merge 4 commits into
xLightsSequencer:masterfrom
heffneil:property-grid-tab-traversal
Open

Property grid: Tab/Shift+Tab traverses to next/prev property (fixes #5697)#6529
heffneil wants to merge 4 commits into
xLightsSequencer:masterfrom
heffneil:property-grid-tab-traversal

Conversation

@heffneil

@heffneil heffneil commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #5697.

Stock wxPropertyGrid handles Tab in an open editor by calling Navigate(IsForward), which moves keyboard focus to the next sibling widget of the grid rather than to the next property within it. The result on the Layout tab's Model pane: type a value (e.g. # Lights Top), press Tab, and focus jumps out to the 3D preview — the user has to click back into the next field every time.

Despite a long-standing assumption that this isn't fixable in wxWidgets, wxPropertyGrid does expose enough to override the behavior cleanly.

Fix

Override Tab in our existing xlPropertyGrid subclass (src-ui-wx/shared/utils/xlPropertyGrid.h) via wxEVT_CHAR_HOOK:

  • Plain Tab — advance to the next visible editable property and focus its editor, skipping category/section headers.
  • Shift+Tab — same, backward.
  • Works for every row type — text, dropdown/choice, Click-to-Edit (Start Channel, Preview, …) and disabled/read-only rows. Traversal is gated on the selected property, not on editor focus: the first cut only advanced from focused text editors, so it stalled after ~5-6 fields and couldn't get past dropdowns, Click-to-Edit elements, or down into Controller Connections (per @derwin12's testing).
  • Wrap-around — Tab at the last visible property cycles back to the first, and Shift+Tab at the first cycles to the last, so Tab walks the whole list. (Changed from the original "fall through to wx default at the boundary" per review feedback — the boundary fall-through only happens now if there is no selection / no editable property at all.)
  • Ctrl+Tab / Alt+Tab, or Tab with no property selected — falls through to wx default traversal.

Scroll no longer edits values

Spin/choice editors capture the mouse wheel and change their value, which fires the same grid rebuild (below) and bounced the selection back to the top. The active editor's wheel events are now redirected to scroll the grid, so scrolling never mutates a value or loses your place.

Survival across grid rebuilds

Committing the current edit fires wxEVT_PG_CHANGED, which LayoutPanel::OnPropertyGridChange translates into a model reload that rebuilds the entire grid. That rebuild invalidates the wxPGProperty* pointers we got from the iterator and (without this fix) leaves focus on the layout canvas, because the editor we just focused is destroyed mid-flight.

To survive that:

  1. CommitChangesFromEditor() is called explicitly so the value lands before we move on; if it fails (validation rejects the value) we stay put.
  2. The focus shift is deferred via CallAfter, so it runs after any rebuild queued by the commit has settled.
  3. The next property is looked up by name (GetPropertyByName(nextName)), not the now-stale pointer.

Test plan

  • macOS Release build (Xcode 26.5)
  • Layout tab → select a Window Frame (the model from Use "Tab" to move to next field #5697); type 13 in # Lights Top and press Tab → focus lands in the next field, value 13 is saved
  • Tab / Shift+Tab traverse the entire list, including dropdowns, Click-to-Edit, and disabled rows, and into sections like Controller Connections
  • Tab at the bottommost property wraps to the top; Shift+Tab at the top wraps to the bottom
  • Scrolling the mouse wheel over a spin/dropdown row scrolls the grid without changing its value or losing the selection
  • Ctrl+Tab / Alt+Tab still trigger their normal behaviors
  • Verify on Windows and Linux (wx-platform-neutral code; same widget, same fork)

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings June 10, 2026 19:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds custom Tab/Shift+Tab navigation behavior to xlPropertyGrid so keyboard traversal moves between visible properties (when editing) instead of jumping focus to sibling widgets.

Changes:

  • Adds wxEVT_CHAR_HOOK handler to intercept Tab/Shift+Tab while an editor is focused.
  • Implements deferred property selection after committing edits to avoid pointer invalidation after grid rebuilds.
  • Includes wx/propgrid/propgridiface.h to support newly used PropertyGrid APIs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src-ui-wx/shared/utils/xlPropertyGrid.h
Comment thread src-ui-wx/shared/utils/xlPropertyGrid.h
Comment thread src-ui-wx/shared/utils/xlPropertyGrid.h
heffneil added a commit to heffneil/xLights that referenced this pull request Jun 11, 2026
- Bail out if CommitChangesFromEditor() returns false so a validation
  rejection keeps focus on the current editor instead of silently
  moving on (Copilot comment xLightsSequencer#2).
- Move OnCharHook to private; it is only used as a Bind-target and
  was never meant to be part of the public API (Copilot comment xLightsSequencer#3).

Copilot comment xLightsSequencer#1 (iterator underflow) was investigated and
dismissed: wxPropertyGridIteratorBase::Prev() correctly sets
m_property = nullptr at the boundary and AtEnd() catches it; the
suggested GetNextVisible/GetPrevVisible helpers do not exist on
wxPropertyGridInterface in this wx fork.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@derwin12

Copy link
Copy Markdown
Contributor

I tested this, it is of very little benefit. You cant tab past start channel, you cant tab past preview etc .. all the Click to Edit elements. You cant get down to Controller Connections etc. anything that is disabled. Basically I could tab like 5-6 fields in the list. Is it more confusing to put this in or leave out?

image

heffneil and others added 3 commits June 21, 2026 16:35
Fixes xLightsSequencer#5697. Stock wxPropertyGrid handles Tab in an open editor by
calling Navigate(IsForward), which moves keyboard focus to the next
sibling widget of the grid rather than to the next property within
it - so the user had to click their way back into the grid to edit
the next field.

Override Tab in xlPropertyGrid via wxEVT_CHAR_HOOK:
- Plain Tab advances to the next visible property and focuses its
  editor; Shift+Tab moves backward.
- Ctrl+Tab / Alt+Tab and Tab from a non-editor context fall through
  to wx default traversal (so the user can still exit the grid).
- Tab at the first/last visible property also falls through.

Committing the current edit fires wxEVT_PG_CHANGED, which can queue
a model reload that rebuilds the entire grid and invalidates the
property pointers we got from the iterator. To survive that, the
focus shift is deferred via CallAfter and the next property is
looked up by name (not by stale pointer).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Bail out if CommitChangesFromEditor() returns false so a validation
  rejection keeps focus on the current editor instead of silently
  moving on (Copilot comment xLightsSequencer#2).
- Move OnCharHook to private; it is only used as a Bind-target and
  was never meant to be part of the public API (Copilot comment xLightsSequencer#3).

Copilot comment xLightsSequencer#1 (iterator underflow) was investigated and
dismissed: wxPropertyGridIteratorBase::Prev() correctly sets
m_property = nullptr at the boundary and AtEnd() catches it; the
suggested GetNextVisible/GetPrevVisible helpers do not exist on
wxPropertyGridInterface in this wx fork.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nd, wheel no longer edits

- Skip category/section headers when tabbing so focus lands on the next
  editable property
- Gate on GetSelection() instead of IsEditorFocused() so combo/choice
  editors advance instead of dropping focus out of the grid
- Wrap around at the ends (last->first, first->last) so Tab cycles
- Redirect the mouse wheel from the active editor to the grid so
  scrolling never changes a spin/combo value (which rebuilt the grid
  and lost the selection)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 22, 2026 04:06
@heffneil heffneil force-pushed the property-grid-tab-traversal branch from 9208448 to c87a21f Compare June 22, 2026 04:06
@heffneil

Copy link
Copy Markdown
Contributor Author

@derwin12 — good catch, and you were right that the first cut was nearly useless. Fixed and pushed.

The root cause: the handler only advanced when wx reported an editor as focused, which is only true for plain text editors. Click-to-Edit fields (Start Channel, Preview), choice/combo dropdowns, and disabled rows don't report editor focus, so Tab fell straight out of the grid after a few fields — exactly the "5-6 fields then stuck" you saw.

What changed:

  • Gate on the selected property, not editor focus — Tab now advances through every visible row, including Click-to-Edit elements, dropdowns, and disabled/read-only rows.
  • Skip category/section headers — so it walks down into Controller Connections and the other sections instead of stopping at the header.
  • Wrap-around — at the bottom it cycles back to the top (and Shift+Tab from the top goes to the bottom).
  • Bonus fix: scrolling the mouse wheel over a spin/dropdown row no longer changes its value and bounces you back to the top — the wheel now just scrolls.

Tested locally on the model property grid: Tab/Shift+Tab now traverses the entire list end-to-end and back. Give it another run when you get a chance.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread src-ui-wx/shared/utils/xlPropertyGrid.h
Skip only EXPANDED category headers (descend to their first child);
stop on COLLAPSED ones so the user can expand them (Right arrow) and
Tab again to enter the section. Otherwise wxPG_ITERATE_VISIBLE hides a
collapsed category's children and Tab skips the whole section. Applied
to both the forward step and the wrap-around via a shared skippable()
lambda.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@wantmoore

Copy link
Copy Markdown

Major usability win here. Built and tested on macOS 26.3.1. These bits of little polish go a long way in making xLights feel more like the world-class piece of software that it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use "Tab" to move to next field

4 participants