mirror of
https://github.com/Bubberstation/Bubberstation.git
synced 2026-08-29 08:08:49 +01:00
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.
<details>
<summary>Images of Stamping Issue</summary>


</details>
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.
<details>
<summary>Images of PDA Issue</summary>

</details>
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.
<details>
<summary>Images of PDA Issues Resolved</summary>


</details>
<details>
<summary>Images of Stamping Issues Resolved</summary>



</details>
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
🆑
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.
/🆑
(cherry picked from commit 29b98b343b)
This commit is contained in:
@@ -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<HTMLDivElement>(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 && (
|
||||
<div className="Section__title">
|
||||
@@ -110,7 +108,9 @@ export const Section = forwardRef(
|
||||
<div
|
||||
className="Section__content"
|
||||
onScroll={onScroll}
|
||||
ref={contentRef}
|
||||
// For posterity: the forwarded ref needs to be here specifically
|
||||
// to actually let things interact with the scrolling.
|
||||
ref={forwardedRef}
|
||||
>
|
||||
{children}
|
||||
</div>
|
||||
|
||||
Reference in New Issue
Block a user