Fixes #5358. Narrow draw pipeline to skip parent on child-only redraws

Terminal.Gui/App/ApplicationImpl.Screen.cs

@@ -280,6 +280,15 @@ public void LayoutAndDraw (bool forceRedraw = false)
 
             // Only force a complete redraw if needed (needsLayout or forceRedraw).
             // Otherwise, just redraw views that need it.
+            //
+            // NOTE (#5358): passing force=true here calls SetNeedsDraw on the top runnable, which
+            // cascades to all overlapping subviews via the existing SetNeedsDraw recursion. This
+            // is the remaining draw-fan-out source documented by TabsFanOutIntegrationTests. The
+            // proper fix requires tracking adornment thickness changes (in addition to Frame
+            // changes) so the SuperView can be invalidated precisely instead of force-redrawing
+            // the whole tree. Dropping force-on-neededLayout without that tracking exposes
+            // stale-content bugs in the shrink/move and adornment-rebalance paths (covered by
+            // ShadowTests / BorderViewTests).
             View.Draw (views.ToArray ().Cast<View> (), neededLayout || forceRedraw);
 
             Driver.Clip = new Region (clipRect);

Terminal.Gui/ViewBase/Adornment/AdornmentView.cs

@@ -15,9 +15,10 @@ namespace Terminal.Gui.ViewBase;
 ///         back-reference, making <see cref="AdornmentImpl"/> the single authoritative owner of Thickness.
 ///     </para>
 ///     <para>
-///         During the incremental migration, existing <see cref="Border"/> and <see cref="Padding"/> continue
-///         to extend <see cref="Adornment"/>. Only newly migrated adornments (starting with <c>MarginView</c>)
-///         extend <see cref="AdornmentView"/>.
+///         <see cref="MarginView"/>, <see cref="BorderView"/>, and <see cref="PaddingView"/> all extend
+///         <see cref="AdornmentView"/>. <see cref="AdornmentImpl"/> is the authoritative owner of
+///         <see cref="IAdornment.Thickness"/>; the corresponding <see cref="AdornmentView"/> hosts the
+///         render-layer behavior.
 ///     </para>
 /// </remarks>
 public class AdornmentView : View, IAdornmentView, IDesignable
@@ -151,8 +152,9 @@ protected override bool OnClearingViewport ()
             Adornment.Thickness.Draw (Driver, ViewportToScreen (Viewport), Diagnostics, ToString ());
         }
 
-        SetNeedsDraw ();
-
+        // Do NOT call SetNeedsDraw () here. The thickness has been drawn; calling
+        // SetNeedsDraw cascades to the parent's SubViewNeedsDraw mid-pass and adds
+        // churn that competes with the needsDrawSelf gate added for issue #5358.
         return true;
     }
 

Terminal.Gui/ViewBase/View.Drawing.Adornments.cs

@@ -133,9 +133,10 @@ internal void DoDrawAdornments (Region? originalClip)
         Padding.View?.SetNeedsDraw ();
         Margin.View?.SetNeedsDraw ();
 
-        // Ensure NeedsDraw is true for the rest of the draw pipeline (DoClearViewport, DoDrawText, etc.)
-        // When adornment Views are null (lightweight), their NeedsDraw doesn't contribute to the parent's
-        // NeedsDraw property. But if we're here, the parent IS drawing, so we must set NeedsDrawRect.
+        // Keep NeedsDraw true for DoRenderLineCanvas and ClearNeedsDraw. The self-content
+        // methods (DoClearViewport, DoDrawText, DoDrawContent) are now gated on the
+        // needsDrawSelf snapshot captured in Draw() *before* this escalation, so this no
+        // longer forces a full parent redraw when only a child was dirty (issue #5358).
         if (NeedsDrawRect == Rectangle.Empty)
         {
             NeedsDrawRect = Viewport;

Terminal.Gui/ViewBase/View.Drawing.cs

@@ -91,8 +91,12 @@
 
         Region? originalClip = GetClip ();
 
-        // TODO: This can be further optimized by checking NeedsDraw below and only
-        // TODO: clearing, drawing text, drawing content, etc. if it is true.
+        // Capture whether THIS view's own content needs redrawing BEFORE DoDrawAdornments
+        // escalates NeedsDrawRect (see View.Drawing.Adornments.cs DoDrawAdornments).
+        // When only SubViewNeedsDraw is true, needsDrawSelf is false and we skip
+        // ClearViewport/DrawText/DrawContent so child-only invalidations stay narrow.
+        bool needsDrawSelf = NeedsDraw;
+
         if (NeedsDraw || SubViewNeedsDraw)
         {
             // ------------------------------------
@@ -120,10 +124,22 @@
             // SuperView's ClearViewport or peer SubViews' content.
             // This follows the same pattern as DrawAdornments(), which creates
             // per-adornment DrawContexts for the same reason.
-            _localDrawContext = new DrawContext ();
+            //
+            // Issue #5358 (review feedback item 2): only recreate _localDrawContext when
+            // we actually intend to redraw self-content this pass. On child-only passes
+            // (needsDrawSelf=false), we must preserve the prior context so DoDrawComplete
+            // doesn't overwrite CachedDrawnRegion with an empty region and break
+            // TransparentMouse hit-testing until the next full self-redraw.
+            if (needsDrawSelf)
+            {
+                _localDrawContext = new DrawContext ();
+            }
 
-            SetAttributeForRole (Enabled ? VisualRole.Normal : VisualRole.Disabled);
-            DoClearViewport (context);
+            if (needsDrawSelf)
+            {
+                SetAttributeForRole (Enabled ? VisualRole.Normal : VisualRole.Disabled);
+                DoClearViewport (context);
+            }
 
             // ------------------------------------
             // Draw the SubViews first (order matters: SubViews, Text, Content)
@@ -142,20 +158,23 @@
                 _lastClearedViewport = null;
             }
 
-            // ------------------------------------
-            // Draw the text — tracked in both shared (clip exclusion) and local (hit-testing) contexts
-            Trace.Draw (this.ToIdentifyingString (), "Text");
-            SetAttributeForRole (Enabled ? VisualRole.Normal : VisualRole.Disabled);
-            DoDrawText (_localDrawContext);
-
-            // ------------------------------------
-            // Draw the content — tracked in both shared (clip exclusion) and local (hit-testing) contexts
-            Trace.Draw (this.ToIdentifyingString (), "Content");
-            DoDrawContent (_localDrawContext);
-
-            // Merge this view's own draws into the shared context so the SuperView
-            // can track the aggregate for clip exclusion.
-            context.AddDrawnRegion (_localDrawContext.GetDrawnRegion ());
+            if (needsDrawSelf)
+            {
+                // ------------------------------------
+                // Draw the text — tracked in both shared (clip exclusion) and local (hit-testing) contexts
+                Trace.Draw (this.ToIdentifyingString (), "Text");
+                SetAttributeForRole (Enabled ? VisualRole.Normal : VisualRole.Disabled);
+                DoDrawText (_localDrawContext);
+
+                // ------------------------------------
+                // Draw the content — tracked in both shared (clip exclusion) and local (hit-testing) contexts
+                Trace.Draw (this.ToIdentifyingString (), "Content");
+                DoDrawContent (_localDrawContext);
+
+                // Merge this view's own draws into the shared context so the SuperView
+                // can track the aggregate for clip exclusion.
+                context.AddDrawnRegion (_localDrawContext.GetDrawnRegion ());
+            }
 
             // ------------------------------------
             // Draw adornment SubViews BEFORE rendering LineCanvas so their lines
@@ -221,11 +240,85 @@
             return;
         }
 
-        ClearViewport (context);
+        // Issue #5358: narrow the framework's clear to NeedsDrawRect when it's a true
+        // partial region AND this view is not itself scrolled. Narrowing in the public
+        // ClearViewport API would silently change the contract for direct callers
+        // (Code.OnClearingViewport, MarkdownCodeBlock, direct test calls) that expect
+        // a full fill of the viewport background — see review feedback items 1, 3.
+        //
+        // The "this view is not itself scrolled" guard sidesteps a separate coordinate-
+        // space inconsistency: SetNeedsDraw(Rectangle) cascades to subviews using
+        // frame-local coordinates (subtracts subview.Frame.X/Y), while the no-arg
+        // SetNeedsDraw passes Viewport (content-coord). For an unscrolled view those
+        // coincide; for a scrolled view they don't, and the narrowing math would shift
+        // the clear off-screen. Until that convention is normalized (out of scope here),
+        // only narrow when Viewport.Location is the origin.
+        if (CanNarrowClearToNeedsDrawRect (out Rectangle narrowedScreen))
+        {
+            Driver?.FillRect (narrowedScreen);
+            _lastClearedViewport = narrowedScreen;
+            SetNeedsDraw (NeedsDrawRect);
+        }
+        else
+        {
+            ClearViewport (context);
+        }
+
         OnClearedViewport ();
         ClearedViewport?.Invoke (this, new DrawEventArgs (Viewport, Viewport, null));
     }
 
+    /// <summary>
+    ///     Determines whether the framework's <see cref="DoClearViewport"/> can safely narrow
+    ///     the clear to just <see cref="NeedsDrawRect"/>. See <see cref="DoClearViewport"/> for
+    ///     the rationale.
+    /// </summary>
+    private bool CanNarrowClearToNeedsDrawRect (out Rectangle narrowedScreen)
+    {
+        narrowedScreen = Rectangle.Empty;
+
+        if (Driver is null)
+        {
+            return false;
+        }
+
+        if (NeedsDrawRect.IsEmpty)
+        {
+            return false;
+        }
+
+        Rectangle viewport = Viewport;
+
+        // Only narrow when this view is NOT itself scrolled — see DoClearViewport comment.
+        if (viewport.Location != Point.Empty)
+        {
+            return false;
+        }
+
+        // Only narrow when NeedsDrawRect is strictly smaller than the viewport. SetNeedsDraw()
+        // (no-arg) sets NeedsDrawRect to the current Viewport, meaning "everything is dirty";
+        // we don't want to narrow in that case.
+        if (NeedsDrawRect.Width >= viewport.Width && NeedsDrawRect.Height >= viewport.Height)
+        {
+            return false;
+        }
+
+        // ClearContentOnly: skip narrowing; the existing visible-content intersection is
+        // already a content-area optimization and combining the two correctly is non-trivial.
+        if (ViewportSettings.FastHasFlags (ViewportSettingsFlags.ClearContentOnly))
+        {
+            return false;
+        }
+
+        // NeedsDrawRect is in this view's coords; for an unscrolled view those equal viewport-
+        // local coords (origin (0,0) = top-left of visible area). Convert to screen.
+        Rectangle dirtyScreen = ViewportToScreen (NeedsDrawRect);
+        Rectangle toClear = ViewportToScreen (viewport with { Location = Point.Empty });
+        narrowedScreen = Rectangle.Intersect (toClear, dirtyScreen);
+
+        return !narrowedScreen.IsEmpty;
+    }
+
     /// <summary>
     ///     Called when the <see cref="Viewport"/> is to be cleared.
     /// </summary>

Terminal.Gui/ViewBase/View.Layout.cs

@@ -95,6 +95,7 @@ private bool SetFrame (in Rectangle frame)
             return false;
         }
 
+        Rectangle? oldFrame = _frame;
         var oldViewport = Rectangle.Empty;
 
         if (IsInitialized)
@@ -111,6 +112,16 @@ private bool SetFrame (in Rectangle frame)
         SetNeedsDraw ();
         SetNeedsLayout ();
 
+        // Issue #5358: when Frame shrinks or moves, the SuperView's old-frame area is now
+        // uncovered and must be cleared on the next draw. Invalidate the union of the old and
+        // new frames on the SuperView so its region-aware ClearViewport repaints just that area.
+        // SetFrame is the single source of truth for this invalidation for both direct Frame
+        // assignment and layout-driven frame updates.
+        if (oldFrame is { } prev && SuperView is { })
+        {
+            SuperView.SetNeedsDraw (Rectangle.Union (prev, frame));
+        }
+
         // BUGBUG: When SetFrame is called from Frame_set, this event gets raised BEFORE OnResizeNeeded. Is that OK?
         OnFrameChanged (in frame);
         FrameChanged?.Invoke (this, new EventArgs<Rectangle> (in frame));
@@ -578,6 +589,12 @@ public bool Layout (Size contentSize)
             // recomputed during dependency resolution. Draw after layout only when this view was
             // directly invalidated for layout or its resolved frame actually changed.
             SetNeedsDraw ();
+
+            // NOTE (#5358, review feedback item 4): the SuperView invalidation for frame
+            // changes is handled in SetFrame, which is the single source of truth for Frame
+            // mutation. Both direct (view.Frame = ...) and layout-driven (SetRelativeLayout
+            // → SetFrame) paths go through it, so calling SuperView.SetNeedsDraw(union) here
+            // too would be redundant and would do the cascade work twice on the hot path.
         }
 
         return true;

Terminal.Gui/ViewBase/View.NeedsDraw.cs

@@ -85,11 +85,12 @@ public void SetNeedsDraw (Rectangle viewPortRelativeRegion)
         }
         else
         {
-            int x = Math.Min (Viewport.X, viewPortRelativeRegion.X);
-            int y = Math.Min (Viewport.Y, viewPortRelativeRegion.Y);
-            int w = Math.Max (Viewport.Width, viewPortRelativeRegion.Width);
-            int h = Math.Max (Viewport.Height, viewPortRelativeRegion.Height);
-            NeedsDrawRect = new Rectangle (x, y, w, h);
+            // Union NeedsDrawRect with the incoming region. The previous formula unioned
+            // against Viewport (a bug — it widened to nearly viewport-size on every call),
+            // which made NeedsDrawRect useless for narrowing draw work. Issue #5358
+            // requires an accurate dirty rect so the region-aware ClearViewport and
+            // SetNeedsDraw cascade can stay narrow.
+            NeedsDrawRect = Rectangle.Union (NeedsDrawRect, viewPortRelativeRegion);
         }
 
         // Do not set on Margin - it will be drawn in a separate pass.

Tests/IntegrationTests/TabsFanOutIntegrationTests.cs

@@ -45,6 +45,7 @@ private sealed class Counters
         public int SubViewsLaidOut;
         public int DrawComplete;
         public int ClearedViewport;
+        public int DrawingText;
     }
 
     private static string MakeText (string prefix, int lines)
@@ -104,11 +105,13 @@ public void Integration_RealPageDown_OnActiveTab_DoesNotFanOutLayoutToInactiveTa
             codes [i].SubViewsLaidOut += (_, _) => perTab [captured].SubViewsLaidOut++;
             codes [i].DrawComplete += (_, _) => perTab [captured].DrawComplete++;
             codes [i].ClearedViewport += (_, _) => perTab [captured].ClearedViewport++;
+            codes [i].DrawingText += (_, _) => perTab [captured].DrawingText++;
         }
 
         tabs.SubViewsLaidOut += (_, _) => tabsContainer.SubViewsLaidOut++;
         tabs.DrawComplete += (_, _) => tabsContainer.DrawComplete++;
         tabs.ClearedViewport += (_, _) => tabsContainer.ClearedViewport++;
+        tabs.DrawingText += (_, _) => tabsContainer.DrawingText++;
 
         ScrollableCode active = codes [0];
 
@@ -123,11 +126,13 @@ public void Integration_RealPageDown_OnActiveTab_DoesNotFanOutLayoutToInactiveTa
                                                         perTab [i].SubViewsLaidOut = 0;
                                                         perTab [i].DrawComplete = 0;
                                                         perTab [i].ClearedViewport = 0;
+                                                        perTab [i].DrawingText = 0;
                                                     }
 
                                                     tabsContainer.SubViewsLaidOut = 0;
                                                     tabsContainer.DrawComplete = 0;
                                                     tabsContainer.ClearedViewport = 0;
+                                                    tabsContainer.DrawingText = 0;
                                                 })
                                          .KeyDown (Key.PageDown)
                                          .KeyDown (Key.PageDown)
@@ -136,12 +141,12 @@ public void Integration_RealPageDown_OnActiveTab_DoesNotFanOutLayoutToInactiveTa
         outputHelper.WriteLine ($"Driver: {driverName}");
         outputHelper.WriteLine ($"Active tab viewport Y after 3 PageDowns: {active.Viewport.Y}");
         outputHelper.WriteLine ("Per-tab counters (after 3 PageDowns on active tab):");
-        outputHelper.WriteLine ("  tab        laidOut  drawComplete  clearedViewport");
-        outputHelper.WriteLine ($"  Tabs       {tabsContainer.SubViewsLaidOut,7}  {tabsContainer.DrawComplete,12}  {tabsContainer.ClearedViewport,15}");
+        outputHelper.WriteLine ("  tab        laidOut  drawComplete  clearedViewport  drawingText");
+        outputHelper.WriteLine ($"  Tabs       {tabsContainer.SubViewsLaidOut,7}  {tabsContainer.DrawComplete,12}  {tabsContainer.ClearedViewport,15}  {tabsContainer.DrawingText,11}");
 
         for (var i = 0; i < TabCount; i++)
         {
-            outputHelper.WriteLine ($"  Code{i + 1,-6} {perTab [i].SubViewsLaidOut,7}  {perTab [i].DrawComplete,12}  {perTab [i].ClearedViewport,15}");
+            outputHelper.WriteLine ($"  Code{i + 1,-6} {perTab [i].SubViewsLaidOut,7}  {perTab [i].DrawComplete,12}  {perTab [i].ClearedViewport,15}  {perTab [i].DrawingText,11}");
         }
 
         Assert.True (
@@ -152,27 +157,34 @@ public void Integration_RealPageDown_OnActiveTab_DoesNotFanOutLayoutToInactiveTa
                      perTab [0].DrawComplete > 0,
                      $"Active tab must draw in response to real PageDown, got DrawComplete={perTab [0].DrawComplete}.");
 
-        int inactiveDraws = 0;
+        int inactiveTextDraws = 0;
         int inactiveLayouts = 0;
 
         for (var i = 1; i < TabCount; i++)
         {
-            inactiveDraws += perTab [i].DrawComplete;
+            inactiveTextDraws += perTab [i].DrawingText;
             inactiveLayouts += perTab [i].SubViewsLaidOut;
         }
 
-        outputHelper.WriteLine ($"Sum inactive DrawComplete = {inactiveDraws}");
+        outputHelper.WriteLine ($"Sum inactive DrawingText = {inactiveTextDraws}");
         outputHelper.WriteLine ($"Sum inactive SubViewsLaidOut = {inactiveLayouts}");
 
-        // CURRENT BEHAVIOR: draw fan-out still exists through the real input → command → main-loop path,
-        // but layout fan-out should now be eliminated.
+        // Issue #5358 fix narrows draw fan-out at the View.Draw pipeline level (verified by
+        // TabsFanOutDiagnosticTests at synthetic level). At integration level a separate
+        // cascade source remains: ApplicationImpl.LayoutAndDraw passes force=true to
+        // View.Draw whenever any view needed layout, which calls SetNeedsDraw on the top
+        // runnable, which cascades to overlapping subviews via the existing SetNeedsDraw
+        // recursion. Removing that force=true uncovers stale-content bugs in the shrink/move
+        // path (covered by existing ShadowTests and BorderViewTests) and is out of scope
+        // for #5358. Until that broader fix lands, inactive tab pages still receive
+        // NeedsDraw via the LayoutAndDraw force path. Layout fan-out is already fully
+        // eliminated by PR #5373.
         Assert.True (
-                     inactiveDraws > 0,
-                     $"Documents issue #4973 (integration-level): inactive_total DrawComplete={inactiveDraws}. " +
-                     "Flip to Assert.Equal(0, inactiveDraws) after fix lands.");
+                     inactiveTextDraws > 0,
+                     $"Documents the remaining draw fan-out via ApplicationImpl.LayoutAndDraw's force=true path: " +
+                     $"inactive_total DrawingText={inactiveTextDraws}. Flip to Assert.Equal(0, inactiveTextDraws) " +
+                     "after the broader LayoutAndDraw cascade is addressed (out of scope for #5358).");
 
-        Assert.Equal (
-                      0,
-                      inactiveLayouts);
+        Assert.Equal (0, inactiveLayouts);
     }
 }