Fix resolver by oatkiller · Pull Request #61886 · elastic/kibana

oatkiller · 2020-03-30T20:01:12Z

Summary

This PR fixes several issues with the Resolver graph. The root cause is an SVG which was effecting the layout of the main resolver node. This effected the layout and Resolver's ability to respond to user interactions.

Note

If you have a really big monitor and your browser window is maxed out, you might not be able to reproduce these issues. Try at around 1600px width.

Issues on master

The graph isn't rendering things centered on the origin as expected:

using the panel to center on lsass.exe doesn't center on the lsass.exe node:

The right part of the graph has an SVG element rendered. Clicking and or dragging in that area is non-responsive

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

This reverts commit dd93a14.

This reverts commit 427bee4030137f7c0d99575dc847f00f08697266.

This reverts commit 4af3a439259f53816917d89689ec05010c0be0ee.

…raph

bkimmel · 2020-03-30T21:18:55Z

x-pack/plugins/endpoint/public/embeddables/resolver/store/actions.ts

  readonly payload: {
    /**
-     * Used to identify the process node that should be brought into view.
+     * Used to identify the process node that the user focused on (in the DOM)


@oatkiller this might actually not be true... e.g. when someone switches the active descendant (with the Panel), the tree node doesn't have/get focus until it's returned to its parent composite

I don't understand what you mean. But if you have a better suggestion for this, please let me know. Resolver already has a concept of 'focusing' on a process. It happens when you click the icon in the panel and the graph moves to center the process. This action should probably be combined with that one. Thoughts?

According to the role description for tree, focus and activedescendant are independent concepts. When the user clicks the icon in the panel, the icon should still have input focus, but the Resolver's aria-activedescendant changes to the node's ID and when focus returns to the tree/(Resolver) you assign focus back to the activedescendant node.

With this is mind, my suggestion might be to change this interface name to something like userSelectedResolverNode but we're going to be taking a swipe at this whole active/focus thing soon so feel free to let it ride for now.

Maybe I understand. Basically you're saying that the 'selected' (our definition) resolver node and the focused (DOM's definition) element aren't necessarily (and shouldn't be) the same?

oatkiller · 2020-03-30T21:13:18Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/defs.tsx

- * 3) <use> elements can be handled by compositor (faster)
+ * This `<defs>` element is used to define the reusable assets for the Resolver
+ * It confers several advantages, including but not limited to:
+ *  1. Freedom of form for creative assets (beyond box-model constraints)


using markdown for IDEs that support it

oatkiller · 2020-03-30T21:13:50Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/defs.tsx

+    </svg>
+  ))
+)`
+  position: absolute;


This is the most important change in this PR. Without this style change, the SymbolDefinitions DOM nodes change the size of the resolver graph and effect all the calculations in a negative way.

oatkiller · 2020-03-30T21:14:37Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/index.tsx

-            ))}
+            {[...processNodePositions].map(([processEvent, position], index) => {
+              const adjacentNodeMap = processToAdjacencyMap.get(processEvent);
+              if (!adjacentNodeMap) {


Asserting that this isn't falsey (it never should be) so that ProcessEventDot doesn't need conditional logic for that case.

oatkiller · 2020-03-30T21:15:00Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

       * map of what nodes are "adjacent" to this one in "up, down, previous, next" directions
       */
-      adjacentNodeMap?: AdjacentProcessMap;
+      adjacentNodeMap: AdjacentProcessMap;


this is no longer optional

oatkiller · 2020-03-30T21:15:46Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

      const [magFactorX] = projectionMatrix;

-      const selfId = adjacentNodeMap?.self;
+      const selfId = adjacentNodeMap.self;


No longer necessary to check if adjacentNodeMap is optional

oatkiller · 2020-03-30T21:17:32Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

+        selfId,
+      ]);
+      const labelId = useMemo(() => resolverNodeIdGenerator(), [resolverNodeIdGenerator]);
+      const descriptionId = useMemo(() => resolverNodeIdGenerator(), [resolverNodeIdGenerator]);


This should only be generated once per instance of this component

oatkiller · 2020-03-30T21:17:53Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

Explaining the use of any here

oatkiller · 2020-03-30T21:18:21Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

            role="treeitem"
-            {...levelAttribute}
-            {...flowToAttribute}
+            aria-level={adjacentNodeMap.level}


assign these directly, as the adjacentNodeMap will always be available

@oatkiller that's OK for level, but aria-flowto can't be empty/null so I think that one still has to be spread

typescript caught this as well. ty

oatkiller · 2020-03-30T21:18:45Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

 };

-function getNodeType(processEvent: ResolverEvent): keyof typeof nodeAssets {
+function nodeType(processEvent: ResolverEvent): keyof typeof nodeAssets {


removing get from name for consistency with other Resolver function names.

oatkiller · 2020-03-30T21:26:00Z

x-pack/plugins/endpoint/public/embeddables/resolver/store/actions.ts

  readonly payload: {
    /**
-     * Used to identify the process node that should be brought into view.
+     * Used to identify the process node that the user focused on (in the DOM)


I don't understand what you mean. But if you have a better suggestion for this, please let me know. Resolver already has a concept of 'focusing' on a process. It happens when you click the icon in the panel and the graph moves to center the process. This action should probably be combined with that one. Thoughts?

bkimmel · 2020-03-30T21:30:45Z

x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx

@oatkiller it's documented in SVG 1.1 ( Rec status since 16 Aug 2011) https://www.w3.org/TR/SVG11/animate.html#__smil__ElementTimeControl__beginElement

I'll add that link to the comment

d658188 @bkimmel

oatkiller · 2020-03-31T13:05:09Z

@elasticmachine merge upstream

kibanamachine · 2020-03-31T16:13:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d658188

History

💚 Build #37511 succeeded 0c8c03e
💔 Build #37489 failed 0b505f5
💔 Build #37308 failed 2dc6242
💔 Build #37300 failed e686039b242c4ed2a5f419a23334b5c8b515e3b6
💔 Build #37285 failed 427bee4030137f7c0d99575dc847f00f08697266

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

Panning, zooming, centering did now always work correctly.

* upstream/master: (69 commits) Adding PagerDuty icon to connectors cards (elastic#60805) Fix drag and drop flakiness (elastic#61993) Grok debugger migration (elastic#60658) Endpoint: Fix resolver SVG position issue (elastic#61886) [SIEM] version 7.7 rule import (elastic#61903) Added styles to make combobox list items wider for alerting flyout (elastic#61894) [UA] Tight worker loop can cause high CPU usage (elastic#60950) [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709) [ML] Catching unknown index pattern errors (elastic#61935) [Discover] Deangularize and euificate sidebar (elastic#47559) Endpoint: Add ts-node dev dependency (elastic#61884) Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901) [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649) Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171) [Maps] Explicitly pass fetch function to ems-client (elastic#61846) [SIEM][CASE] Fix aria-labels and translations (elastic#61670) [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842) [EPM] update epm filepath route (elastic#61910) APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732) [Logs UI] Log stream row rendering (elastic#60773) ...

Panning, zooming, centering did now always work correctly.

* master: (64 commits) Adding PagerDuty icon to connectors cards (elastic#60805) Fix drag and drop flakiness (elastic#61993) Grok debugger migration (elastic#60658) Endpoint: Fix resolver SVG position issue (elastic#61886) [SIEM] version 7.7 rule import (elastic#61903) Added styles to make combobox list items wider for alerting flyout (elastic#61894) [UA] Tight worker loop can cause high CPU usage (elastic#60950) [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709) [ML] Catching unknown index pattern errors (elastic#61935) [Discover] Deangularize and euificate sidebar (elastic#47559) Endpoint: Add ts-node dev dependency (elastic#61884) Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901) [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649) Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171) [Maps] Explicitly pass fetch function to ems-client (elastic#61846) [SIEM][CASE] Fix aria-labels and translations (elastic#61670) [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842) [EPM] update epm filepath route (elastic#61910) APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732) [Logs UI] Log stream row rendering (elastic#60773) ...

oatkiller added 9 commits March 30, 2020 17:11

Revert "Resolver/nodedesign 25 (elastic#60630)"

e8f7035

This reverts commit dd93a14.

Revert "Revert "Resolver/nodedesign 25 (elastic#60630)""

68d6d6f

This reverts commit 427bee4030137f7c0d99575dc847f00f08697266.

style fixes

6e63b1b

DNM debug changes

569e56a

fix issue with adjacency node map being passed down

35c4b9d

Revert "DNM debug changes"

1cab6d2

This reverts commit 4af3a439259f53816917d89689ec05010c0be0ee.

Prevent SymbolDefinitions from effecting the layout of the resolver g…

192fcba

…raph

remove object allocations

797a29c

update comment on action

2dc6242

oatkiller force-pushed the fix-resolver branch from fbf9b5b to 2dc6242 Compare March 30, 2020 21:11

bkimmel reviewed Mar 30, 2020

View reviewed changes

oatkiller marked this pull request as ready for review March 30, 2020 21:23

oatkiller requested a review from a team as a code owner March 30, 2020 21:23

oatkiller commented Mar 30, 2020

View reviewed changes

oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 30, 2020

bkimmel reviewed Mar 30, 2020

View reviewed changes

bkimmel approved these changes Mar 30, 2020

View reviewed changes

kqualters-elastic approved these changes Mar 30, 2020

View reviewed changes

elasticmachine and others added 3 commits March 31, 2020 09:05

Merge branch 'master' into fix-resolver

0b505f5

fix type error

0c8c03e

remove any type and fix comment on beginElement

d658188

oatkiller merged commit 503fbfb into elastic:master Mar 31, 2020

oatkiller mentioned this pull request Mar 31, 2020

[7.x] Endpoint: Fix resolver SVG position issue (#61886) #62021

Merged

oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Mar 31, 2020

Endpoint: Fix resolver SVG position issue (elastic#61886)

62391d2

Panning, zooming, centering did now always work correctly.

oatkiller pushed a commit that referenced this pull request Mar 31, 2020

Endpoint: Fix resolver SVG position issue (#61886) (#62021)

40ef05e

Panning, zooming, centering did now always work correctly.

oatkiller deleted the fix-resolver branch March 31, 2022 11:04

Conversation

oatkiller commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Note

Issues on master

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller commented Mar 31, 2020

Uh oh!

kibanamachine commented Mar 31, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

oatkiller commented Mar 30, 2020 •

edited

Loading