RobustToolbox icon indicating copy to clipboard operation
RobustToolbox copied to clipboard

Dirty a UserInterface.Control's arrange when performing measure

Open eoineoineoin opened this issue 3 months ago • 4 comments

The UI layout algorithm assumes that when invalidating a UserInterface.Control's Measure, it's arrange should also be invalidated. However, if a Control modifies it's own subtree in ArrangeOverride(), that invariant is violated. This PR makes a teeny-tiny-slightly-scary modification to when the arrange is invalidated.

Was observing in a live game that opening up the Ghost Roles window, it sometimes would not be scrollable:

2025-09-14_14-08

This doesn't normally reproduce locally, as it's dependent on network latency. I was able to reproduce it locally by adding artifical latency (via tc qdisc add dev lo root netem delay 250ms, and even then, it doesn't occur every time the window is opened) - to reproduce this, we need the GhostRolesEuiState to arrive exactly one render frame (one UserInterfaceManager.FrameUpdate()) after the GhostRolesEui is constructed.

The sequence of events is then:

  1. GhostRolesEui constructs a window with a ScrollContainer - no contents, as the GhostRolesEuiState hasn't arrived yet
  2. UserInterfaceManager.FrameUpdate runs measure and arrange for the ScrollContainer
  3. The ScrollContainer's ArrangeOverride determines that it's child scrollbars are unnecessary and sets their Visibility=False
  4. The scrollbar's Visibility change will InvalidateMeasure() on the parent ScrollContainer
  5. The ScrollContainer's InvalidateMeasure() will attempt to InvalidateArrange(), but since we're already inside ArrangeOverride() (i.e. _arranging==true), this will no-op
  6. We now have a ScrollContainer with IsMeasureValid==false, but IsArrangeValid==true, which is an invalid state
  7. Next frame, the GhostRolesEuiState message arrives and we add child elements to the ScrollContainer; if enough elements are added, these would require scrollbars
  8. These additional children attempt to InvalidateMeasure() on the ScrollContainer, which no-ops, as IsMeasureValid==false
  9. Next UserInterfaceManager.FrameUpdate will recalculate the ScrollContainer's Measure, but not the Arrange, leaving the window in a broken state with no scrollbars

RT's ui layout algorithm is very loosely based on WPF's layout algorithm; WPF has a crucial difference which this PR reconciles - in RT, an InvalidateMeasure() also performs an InvalidateArrange(); however, in WPF, invalidating the measure will only invalidate the measure, but when the control is actually re-measured[0], this then triggers an invalidate arrange. This change modifies RT to do the same thing as WPF in this respect.

[0] https://github.com/dotnet/wpf/blob/main/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/UIElement.cs#L631

eoineoineoin avatar Sep 20 '25 09:09 eoineoineoin

Will look into that test failure. Could have sworn everything passed.

eoineoineoin avatar Sep 20 '25 09:09 eoineoineoin

for the future, you can just change the net.fakelagmin cvar (which the devenv config also sets to 250ms or something by default for testing purposes) instead of having to do it through something like tc, unless somehow this only reveals itself with true network latency

mirrorcult avatar Sep 20 '25 10:09 mirrorcult

Does this also fix the same issue that happens in the admin menu player list, or some longer text paper documents? Both will fail to render a proper scrollbar, or the one it does made will be too short for the full list even on resize.

hobolyra avatar Sep 22 '25 21:09 hobolyra

Does this also fix the same issue that happens in the admin menu player list, or some longer text paper documents? Both will fail to render a proper scrollbar, or the one it does made will be too short for the full list even on resize.

I haven't seen either of those issues (and I've sure opened the paper window about a bajillion times), but certainly looks like they could be the same thing. PaperWindow adds the text to a ScrollContainer, so if the text message arrives one frame after the window is created, the sequence of events is as described in the PR. The admin player list is slightly different but is fundamentally the same thing; it uses a ListContainer, which can toggle the visibility of the child scrollbars inside ArrangeOverride()

Edit: I tell a lie; hacked up a fake player list and have now seen missing scrollbars on the admin panel. That fails for a different reason - the ListContainer underestimates the size of the child elements, so erroneously determines it doesn't need a scrollbar at smaller numbers of players. That's a bugfix for a different (content) PR.

Edit edit: https://github.com/space-wizards/space-station-14/pull/40525

eoineoineoin avatar Sep 23 '25 19:09 eoineoineoin

Superseded by #6323. Sorry for not reviewing this sooner.

PJB3005 avatar Dec 19 '25 22:12 PJB3005