From 8a1347d2c899edc12b46a070b3c75f4f066cc31c Mon Sep 17 00:00:00 2001
From: Wayne Stambaugh <stambaughw@gmail.com>
Date: Tue, 23 Apr 2024 17:01:49 -0400
Subject: [PATCH] Property grid navigation improvements.

* Do not handle tab key event when committing property changes so the
  property grid tab navigation works correctly.
* Do not call commit property changes on tab key when no changes are
  pending.
* Skip event handling in the base object value change and changing event
  handlers in case they do not get overloaded in derived objects.
* Do not force focus to canvas on property change so arrow and tab key
  grid navigation work properly.  Forcing the user to click the property
  grid control to edit a single property doesn't make sense.
* Pass properties panel show event up the event stack.
* Do not set focus to canvas in tool dispatcher if it already has the
  focus.

Fixes https://gitlab.com/kicad/code/kicad/-/issues/16957
---
 common/tool/tool_dispatcher.cpp           |  5 ++++-
 common/widgets/properties_panel.cpp       |  7 ++++++-
 common/widgets/properties_panel.h         |  4 ++--
 eeschema/widgets/sch_properties_panel.cpp | 11 ++++++-----
 pcbnew/widgets/pcb_properties_panel.cpp   |  5 -----
 5 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/common/tool/tool_dispatcher.cpp b/common/tool/tool_dispatcher.cpp
index b675223293..fc1a9ce9ff 100644
--- a/common/tool/tool_dispatcher.cpp
+++ b/common/tool/tool_dispatcher.cpp
@@ -455,8 +455,11 @@ void TOOL_DISPATCHER::DispatchWxEvent( wxEvent& aEvent )
 
     if( isMouseClick( type ) )
     {
-        if( m_toolMgr->GetToolHolder() && m_toolMgr->GetToolHolder()->GetToolCanvas() )
+        if( m_toolMgr->GetToolHolder() && m_toolMgr->GetToolHolder()->GetToolCanvas() &&
+            !m_toolMgr->GetToolHolder()->GetToolCanvas()->HasFocus() )
+        {
             m_toolMgr->GetToolHolder()->GetToolCanvas()->SetFocus();
+        }
     }
 
     // Mouse handling
diff --git a/common/widgets/properties_panel.cpp b/common/widgets/properties_panel.cpp
index 7835a587d1..b69dbd9a4e 100644
--- a/common/widgets/properties_panel.cpp
+++ b/common/widgets/properties_panel.cpp
@@ -424,14 +424,19 @@ void PROPERTIES_PANEL::onShow( wxShowEvent& aEvent )
 {
     if( aEvent.IsShown() )
         UpdateData();
+
+    aEvent.Skip();
 }
 
 
 void PROPERTIES_PANEL::onCharHook( wxKeyEvent& aEvent )
 {
-    if( aEvent.GetKeyCode() == WXK_TAB && !aEvent.ShiftDown() )
+    if( aEvent.GetKeyCode() == WXK_TAB && !aEvent.ShiftDown() && m_grid->IsAnyModified() )
     {
         m_grid->CommitChangesFromEditor();
+
+        // Pass the tab key on so the default property grid tab behavior is honored.
+        aEvent.Skip();
         return;
     }
 
diff --git a/common/widgets/properties_panel.h b/common/widgets/properties_panel.h
index 7f15c48a4f..cac6cd9bec 100644
--- a/common/widgets/properties_panel.h
+++ b/common/widgets/properties_panel.h
@@ -78,8 +78,8 @@ protected:
     virtual wxPGProperty* createPGProperty( const PROPERTY_BASE* aProperty ) const = 0;
 
     // Event handlers
-    virtual void valueChanging( wxPropertyGridEvent& aEvent ) {}
-    virtual void valueChanged( wxPropertyGridEvent& aEvent ) {}
+    virtual void valueChanging( wxPropertyGridEvent& aEvent ) { aEvent.Skip(); }
+    virtual void valueChanged( wxPropertyGridEvent& aEvent ) { aEvent.Skip(); }
     void onCharHook( wxKeyEvent& aEvent );
     void onShow( wxShowEvent& aEvent );
 
diff --git a/eeschema/widgets/sch_properties_panel.cpp b/eeschema/widgets/sch_properties_panel.cpp
index c3b745a8b7..260ba62724 100644
--- a/eeschema/widgets/sch_properties_panel.cpp
+++ b/eeschema/widgets/sch_properties_panel.cpp
@@ -115,11 +115,6 @@ void SCH_PROPERTIES_PANEL::AfterCommit()
     const SELECTION& selection = selectionTool->GetSelection();
 
     rebuildProperties( selection );
-
-    CallAfter( [this]()
-               {
-                   m_frame->GetCanvas()->SetFocus();
-               } );
 }
 
 
@@ -175,6 +170,8 @@ void SCH_PROPERTIES_PANEL::valueChanging( wxPropertyGridEvent& aEvent )
         aEvent.Veto();
         return;
     }
+
+    aEvent.Skip();
 }
 
 
@@ -204,6 +201,8 @@ void SCH_PROPERTIES_PANEL::valueChanged( wxPropertyGridEvent& aEvent )
 
     // Perform grid updates as necessary based on value change
     AfterCommit();
+
+    aEvent.Skip();
 }
 
 
@@ -211,6 +210,8 @@ void SCH_PROPERTIES_PANEL::OnLanguageChanged( wxCommandEvent& aEvent )
 {
     PROPERTIES_PANEL::OnLanguageChanged( aEvent );
     updateFontList();
+
+    aEvent.Skip();
 }
 
 
diff --git a/pcbnew/widgets/pcb_properties_panel.cpp b/pcbnew/widgets/pcb_properties_panel.cpp
index cf28a63885..55826c4964 100644
--- a/pcbnew/widgets/pcb_properties_panel.cpp
+++ b/pcbnew/widgets/pcb_properties_panel.cpp
@@ -120,11 +120,6 @@ void PCB_PROPERTIES_PANEL::AfterCommit()
     const SELECTION& selection = selectionTool->GetSelection();
 
     rebuildProperties( selection );
-
-    CallAfter( [this]()
-               {
-                   static_cast<PCB_EDIT_FRAME*>( m_frame )->GetCanvas()->SetFocus();
-               } );
 }