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

kqualters-elastic · 2020-07-20T15:26:24Z

Summary

Fixes an issue in the useCamera hook where the useAutoUpdatingClientRect hook returned a reference to the resolver dom element that was stale, namely the top/y & left/x properties that are used by the zoom action.
BEFORE:

AFTER:

For maintainers

This was checked for breaking API changes and was labeled appropriately

…he element has changed within the page

elasticmachine · 2020-07-20T15:27:26Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-07-20T15:27:34Z

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

bkimmel · 2020-07-20T15:38:37Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.ts

+  useEffect(() => {
+    window.addEventListener('scroll', handleScroll, { passive: true });
+    return () => {
+      window.removeEventListener('scroll', handleScroll);


(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.

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

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

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

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

bkimmel · 2020-07-20T15:40:16Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.ts

  const ref = useCallback((node: Element | null) => {
    setCurrentNode(node);
    if (node !== null) {
      setRect(node.getBoundingClientRect());
    }
  }, []);
  const { ResizeObserver } = useContext(SideEffectContext);
+  const handleScroll = useCallback(() => {
+    if (currentX !== window.scrollX || currentY !== window.scrollY) {


🔴 might want to wrap this in a requestAnimationFrame b/c I think when you query scrollX, scrollY, and getBoundingClientRect it forces layout sync

re: scrollX, scrollY, getBoundingClientRect :

https://gist.github.com/paulirish/5d52fb081b3570c81e3a

Is it possible useCallback already does this for you under the hood? Seems like something @oatkiller would know.

@bkimmel can you explain how running that in raf would help?

Reading scrollX, scrollY and getBoundingClientRect all force sync layout according to: https://gist.github.com/paulirish/5d52fb081b3570c81e3a Since while this is scrolling, it will get called rapidly, wrapping it in a rAF may reduce layout thrashing. Otherwise it seems like we'd be in a tight loop of this code forcing layout calculations and changing them in quick succession. The rAF doesn't solve everything, but it at least aligns the queries on scrollX etc. until after the current layout has been calculated... so I'd expect it to be at least a little better. Is there a reason you know of or something I'm missing that would make it OK in this case?

bkimmel · 2020-07-20T15:54:03Z

Finished review: 1 🔴 and 1 ❔ / discussion. I'm on track for a thumb here the 🔴 is addressed.

oatkiller · 2020-07-20T17:21:55Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.ts

@@ -284,14 +284,30 @@ export function useAutoUpdatingClientRect(): [DOMRect | null, (node: Element | n
  const [rect, setRect] = useState<DOMRect | null>(null);
  // Using state as ref.current update does not trigger effect hook when reset
  const [currentNode, setCurrentNode] = useState<Element | null>(null);
-
+  const [currentY, setCurrentY] = useState<number>(0);


should you initialize this to the value by reading it from the dom?

const [currentY, setCurrentY] = useState<number>(() => window.scrollY); const [currentX, setCurrentX] = useState<number>(() => window.scrollX);

oatkiller · 2020-07-20T18:00:04Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.ts

+    if (currentX !== window.scrollX || currentY !== window.scrollY) {
+      if (currentNode !== null) {
+        setRect(currentNode.getBoundingClientRect());
+        setCurrentY(window.scrollY);


do you need to use state for currentY and currentX? If they change but setRect doesn't, there shouldn't be any need to render right?
how about making it like:

// assuming currentY and currentX are now refs currentY.current = window.scrollY currentX.current = window.scrollX setRect(currentNode.getBoundingClientRect());

oatkiller · 2020-07-20T18:00:55Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.ts

  const ref = useCallback((node: Element | null) => {
    setCurrentNode(node);
    if (node !== null) {
      setRect(node.getBoundingClientRect());
    }
  }, []);
  const { ResizeObserver } = useContext(SideEffectContext);
+  const handleScroll = useCallback(() => {
+    if (currentX !== window.scrollX || currentY !== window.scrollY) {


@bkimmel can you explain how running that in raf would help?

oatkiller · 2020-07-20T20:05:33Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.ts

+        const currentY = window.scrollY;
+
+        if (currentNode !== null && (previousX !== currentX || previousY !== currentY)) {
+          setRect(currentNode.getBoundingClientRect());


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

kibanamachine · 2020-07-20T20:50:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8838dea

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.3MB	+728.0B	7.3MB

History

💚 Build #63007 succeeded a47d047

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* master: (28 commits) allow some env settings for ingest manager (elastic#72544) Add inspector for VEGA (elastic#70941) chore(NA): fix grunt task for test:coverage (elastic#72539) Archive e2e test results in ES (elastic#72575) preserve 401 errors from new es client (elastic#71248) [SIEM][Detections] Updates text for severity and risk_score overrides (elastic#72244) fixing error occurences tooltip (elastic#72425) use KibanaClient interface instead of Client for new client interface (elastic#72388) [APM] Handle ML errors (elastic#72316) [Discover] Improve histogram tests (elastic#72235) [ftr/webdriver] retry on all errors, use Rx so that timers are canceled (elastic#72540) [pre-req] Move .storybook to storybook; standardize files (elastic#72384) [Security_Solution][Resolver][Bug]: Restore breadcrumb background (elastic#72538) [ML] Fix annotation detector linking & delayed_data(0) (elastic#72468) [Security Solution][Exceptions] - Make esTypes and subType available to index patterns (elastic#72336) [SIEM] Uses faster wait from testing-library and removes duplicate older wait idiom (elastic#72509) Fix long combo box items breaking out of flex item width (elastic#72512) [pipeline/commitStatus] update commit status in baseline-capture job (elastic#72366) [Security Solution][Resolver] Update the resolver element ref on scroll events if the position of the element has changed within the page (elastic#72461) [Maps] auto-fit to data bounds (elastic#72129) ...

…n scroll events if the position of the element has changed within the page (#72461) (#72543)

…n scroll events if the position of the element has changed within the page (#72461) (#72790)

Update the resolver element ref on scroll events if the position of t…

7699d79

…he element has changed within the page

kqualters-elastic requested review from a team as code owners July 20, 2020 15:26

kqualters-elastic added v7.9.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Endpoint Elastic Endpoint feature labels Jul 20, 2020

kqualters-elastic added the Team:Endpoint Data Visibility Team managing the endpoint resolver label Jul 20, 2020

kqualters-elastic changed the title ~~Update the resolver element ref on scroll events if the position of the element has changed within the page~~ [Security Solution] Update the resolver element ref on scroll events if the position of the element has changed within the page Jul 20, 2020

Merge branch 'master' into resolver-scroll-zoom-fix

a47d047

bkimmel reviewed Jul 20, 2020

View reviewed changes

oatkiller approved these changes Jul 20, 2020

View reviewed changes

Refactor useEffect for handling scroll

8838dea

bkimmel approved these changes Jul 20, 2020

View reviewed changes

oatkiller reviewed Jul 20, 2020

View reviewed changes

kqualters-elastic merged commit c3263aa into elastic:master Jul 20, 2020

kqualters-elastic deleted the resolver-scroll-zoom-fix branch July 20, 2020 21:41

kqualters-elastic mentioned this pull request Jul 20, 2020

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

Merged

kqualters-elastic added a commit that referenced this pull request Jul 21, 2020

[7.9] [Security Solution][Resolver] Update the resolver element ref o…

8c55e48

…n scroll events if the position of the element has changed within the page (#72461) (#72543)

kqualters-elastic mentioned this pull request Jul 22, 2020

[7.x] [Security Solution][Resolver] Update the resolver element ref on scroll events if the position of the element has changed within the page (#72461) #72790

Merged

kqualters-elastic added a commit that referenced this pull request Jul 22, 2020

[7.x] [Security Solution][Resolver] Update the resolver element ref o…

f599f56

…n scroll events if the position of the element has changed within the page (#72461) (#72790)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kqualters-elastic commented Jul 20, 2020

elasticmachine commented Jul 20, 2020

elasticmachine commented Jul 20, 2020

bkimmel Jul 20, 2020 •

edited

Loading

kqualters-elastic Jul 20, 2020

bkimmel Jul 20, 2020

bkimmel Jul 20, 2020

bkimmel Jul 20, 2020

bkimmel Jul 20, 2020

bkimmel Jul 20, 2020

bkimmel Jul 20, 2020

oatkiller Jul 20, 2020

bkimmel Jul 20, 2020

bkimmel commented Jul 20, 2020

oatkiller Jul 20, 2020

oatkiller Jul 20, 2020

oatkiller Jul 20, 2020

oatkiller Jul 20, 2020

kibanamachine commented Jul 20, 2020

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

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

Conversation

kqualters-elastic commented Jul 20, 2020

Summary

For maintainers

elasticmachine commented Jul 20, 2020

elasticmachine commented Jul 20, 2020

bkimmel Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkimmel commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jul 20, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

bkimmel Jul 20, 2020 •

edited

Loading