Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Resolver] Update the resolver element ref on scroll events if the position of the element has changed within the page #72461

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -281,17 +281,61 @@ export function useCamera(): {
* handle that.
*/
export function useAutoUpdatingClientRect(): [DOMRect | null, (node: Element | null) => void] {
// This hooks returns `rect`.
const [rect, setRect] = useState<DOMRect | null>(null);
// Using state as ref.current update does not trigger effect hook when reset

const { ResizeObserver, requestAnimationFrame } = useContext(SideEffectContext);

// Keep the current DOM node in state so that we can create a ResizeObserver for it via `useEffect`.
const [currentNode, setCurrentNode] = useState<Element | null>(null);

// `ref` will be used with a react element. When the element is available, this function will be called.
const ref = useCallback((node: Element | null) => {
// track the node in state
setCurrentNode(node);
if (node !== null) {
setRect(node.getBoundingClientRect());
}
}, []);
const { ResizeObserver } = useContext(SideEffectContext);

/**
* Any time the DOM node changes (to something other than `null`) recalculate the DOMRect and set it (which will cause it to be returned from the hook.
* This effect re-runs when the DOM node has changed.
*/
useEffect(() => {
if (currentNode !== null) {
// When the DOM node is received, immedaiately calculate its DOM Rect and return that
setRect(currentNode.getBoundingClientRect());
}
}, [currentNode]);

/**
* When scroll events occur, recalculate the DOMRect. DOMRect represents the position of an element relative to the viewport, so that may change during scroll (depending on the layout.)
* This effect re-runs when the DOM node has changed.
*/
useEffect(() => {
// the last scrollX and scrollY values that we handled
let previousX: number = window.scrollX;
let previousY: number = window.scrollY;

const handleScroll = () => {
requestAnimationFrame(() => {
// synchronously read from the DOM
const currentX = window.scrollX;
const currentY = window.scrollY;

if (currentNode !== null && (previousX !== currentX || previousY !== currentY)) {
setRect(currentNode.getBoundingClientRect());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future improvement: you could check that the new DOMRect is different before setting it. this would prevent useless rerenders.

}

previousX = currentX;
previousY = currentY;
});
};

window.addEventListener('scroll', handleScroll, { passive: true });
return () => {
window.removeEventListener('scroll', handleScroll);
Copy link
Contributor

@bkimmel bkimmel Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(edit) ❔ I think you might also need to supply { passive: true } here for it to treat it as a "matching" signature and do the removal? I'll test and get back to you in a sec on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, passive is only a possible value for the options object in addEventListener, not removeEventListener

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, nevermind looks like that's just for useCapture / {capture: true}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advice for this on MDN reads as:

It's worth noting that some browser releases have been inconsistent on this, and unless you have specific reasons otherwise, it's probably wise to use the same values used for the call to addEventListener() when calling removeEventListener().

https://wiki.developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave this one be. I'm not sure how relevant that advice on MDN is anymore - probably "not very".

};
}, [currentNode, requestAnimationFrame]);

useEffect(() => {
if (currentNode !== null) {
const resizeObserver = new ResizeObserver((entries) => {
Expand Down