From b85d0cabc09b48cfe5e70e41fa8a1b1c15cae929 Mon Sep 17 00:00:00 2001
From: _0Steven <42909981+00-Steven@users.noreply.github.com>
Date: Mon, 12 Feb 2024 01:52:33 +0100
Subject: [PATCH] Fix some Section jank disallowing stamps from working
properly and messengers from auto-scrolling (#81418)
## About The Pull Request
### Alternate title: "A spark of madness drove me to want to fix
paperwork tgui"
While doing paperwork I noticed that stamps could not be placed below a
certain point on paperwork, just reverting to being higher up. Actually
testing this, it seemed the stamps were not accounting for scroll offset
in some way.
Images of Stamping Issue
This led me to look into the code, and lo and behold, when
`PaperSheetStamper` in `interfaces/PaperSheet.tsx` was trying to get the
scroll offset from its `scrollableRef` it was simply returning 0 for
`scrollTop` and 469 for `height` regardless of what either value should
be.
I presumed _something_ was causing the reference to not be properly
forwarded from `PreviewView`'s `Section` to `PaperSheetStamper`, and
continued by testing `onScrollHandler`.
`onScrollHandler` seemed to actually have the desired values, and so I
made my first naive fix: having `PrimaryView` instead handle taking the
values from `onScrollHandler` and forwarding them to
`PaperSheetStamper`, and this worked fine!
However, I felt I didn't understand enough about _why_ the reference
method didn't work to commit to this approach.
So I went to look deeper, and found that there's actually at least one
more thing suffering from this issue.
`interfaces/NtosMessenger/ChatScreen.tsx`, the PDA messenger.
Specifically, its `scrollToBottom` method was using a `Section`
reference similarly to our problematic `interfaces/PaperSheet.tsx`, and
wasn't actually working.
Images of PDA Issue
With tens of tabs of React documentation and tg files open went through
a crash course in React references, and hey uh- wait, no, hold on. It
can't be. Right?
`components/Section.tsx` was handling two distinct references,
`forwardedRef` and `contentRef`, in two distinct places. The former, at
the highest `div`, the latter at a lower `div`... together with
`onScroll`, seemingly confirming my suspicions.
Looking through the file's history, it seems that over the last several
prs and reimplementations of parts of this file, the `div` it was
forwarding a reference from was moved from being the same as `onScroll`
to a different `div` altogether. Then, this became `contentRef`.
And to confirm this, merging `forwardedRef` and `contentRef` seemed to
fix the issues with both stamping paperwork and PDA auto-scrolling just
fine.
Images of PDA Issues Resolved
Images of Stamping Issues Resolved
Final notes:
- I recognized the issue at play in a moment of madness at 1am, it
however still took an extra day for me to feel confident enough in my
React to actually fix this without crashing everything else ever.
- Coincidentally "Ace Combat Zero Soundtrack - Zero" was playing in the
background while finalizing this.
- For some esoteric reason beyond my current comprehension, as opposed
to non-stamped paperwork, stamped paperwork does not auto-focus when
scrolled over sometimes. As it already did this before my changes, I am
considering it outside of the scope of this pr. It would also most
likely deal 3d8 psychic damage, on top of it being 1:53am as of writing
this.
## Why It's Good For The Game
Reduces paperwork jank, but broadly fixes issues with Section's
forwarded reference not actually being a reference to the scrolling
part. This includes PDA messengers' scroll to bottom actually working.
Fixes #60508. Given the issue is from 2021 I don't think it's _related_,
but it exists again and hey it works fine now!
Fixes PDA messengers' scroll to bottom actually working.
Fixes, uh, probably other stuff that used to rely on this working.
## Changelog
:cl:
fix: Fixed stamps not accounting for scroll offset. You can actually
stamp paperwork properly without using accursed knowledge again.
fix: Fixed PDA messenger not scrolling to the bottom when a new message
gets sent.
/:cl:
(cherry picked from commit 29b98b343b9b5b945ad73d7205acdd7d10286fa4)
---
tgui/packages/tgui/components/Section.tsx | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tgui/packages/tgui/components/Section.tsx b/tgui/packages/tgui/components/Section.tsx
index f84e779a6a7..a400324eadf 100644
--- a/tgui/packages/tgui/components/Section.tsx
+++ b/tgui/packages/tgui/components/Section.tsx
@@ -5,7 +5,7 @@
*/
import { canRender, classes } from 'common/react';
-import { forwardRef, ReactNode, RefObject, useEffect, useRef } from 'react';
+import { forwardRef, ReactNode, RefObject, useEffect } from 'react';
import { addScrollableNode, removeScrollableNode } from '../events';
import { BoxProps, computeBoxClassName, computeBoxProps } from './Box';
@@ -70,19 +70,18 @@ export const Section = forwardRef(
...rest
} = props;
- const contentRef = useRef(null);
-
const hasTitle = canRender(title) || canRender(buttons);
/** We want to be able to scroll on hover, but using focus will steal it from inputs */
useEffect(() => {
- if (!contentRef.current) return;
+ if (!forwardedRef?.current) return;
if (!scrollable && !scrollableHorizontal) return;
- addScrollableNode(contentRef.current);
+ addScrollableNode(forwardedRef.current);
return () => {
- removeScrollableNode(contentRef.current!);
+ if (!forwardedRef?.current) return;
+ removeScrollableNode(forwardedRef.current!);
};
}, []);
@@ -98,7 +97,6 @@ export const Section = forwardRef(
computeBoxClassName(rest),
])}
{...computeBoxProps(rest)}
- ref={forwardedRef}
>
{hasTitle && (