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

Fix resolver #61886

Merged
merged 12 commits into from
Mar 31, 2020
Merged

Fix resolver #61886

merged 12 commits into from
Mar 31, 2020

Conversation

oatkiller
Copy link
Contributor

@oatkiller oatkiller commented Mar 30, 2020

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:
image

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

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

Checklist

Delete any items that are not applicable to this PR.

For maintainers

/**
* When the user switches the active descendent of the Resolver.
*/
interface UserFocusedOnResolverNode {
readonly type: 'userFocusedOnResolverNode';
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 oatkiller marked this pull request as ready for review March 30, 2020 21:23
@oatkiller oatkiller requested a review from a team as a code owner March 30, 2020 21:23
* 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using markdown for IDEs that support it

</svg>
))
)`
position: absolute;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

))}
{[...processNodePositions].map(([processEvent, position], index) => {
const adjacentNodeMap = processToAdjacencyMap.get(processEvent);
if (!adjacentNodeMap) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -82,7 +82,7 @@ export const ProcessEventDot = styled(
/**
* map of what nodes are "adjacent" to this one in "up, down, previous, next" directions
*/
adjacentNodeMap?: AdjacentProcessMap;
adjacentNodeMap: AdjacentProcessMap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is no longer optional

@@ -91,7 +91,7 @@ export const ProcessEventDot = styled(

const [magFactorX] = projectionMatrix;

const selfId = adjacentNodeMap?.self;
const selfId = adjacentNodeMap.self;
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 longer necessary to check if adjacentNodeMap is optional

selfId,
]);
const labelId = useMemo(() => resolverNodeIdGenerator(), [resolverNodeIdGenerator]);
const descriptionId = useMemo(() => resolverNodeIdGenerator(), [resolverNodeIdGenerator]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should only be generated once per instance of this component

);
const handleClick = useCallback(() => {
if (animationTarget.current !== null) {
// Use undocumented `beginElement` API on SVG element
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explaining the use of any here

@@ -171,8 +163,8 @@ export const ProcessEventDot = styled(
viewBox="-15 -15 90 30"
preserveAspectRatio="xMidYMid meet"
role="treeitem"
{...levelAttribute}
{...flowToAttribute}
aria-level={adjacentNodeMap.level}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assign these directly, as the adjacentNodeMap will always be available

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typescript caught this as well. ty

@@ -273,7 +265,7 @@ const processTypeToCube: Record<ResolverProcessType, keyof typeof nodeAssets> =
unknownEvent: 'runningProcessCube',
};

function getNodeType(processEvent: ResolverEvent): keyof typeof nodeAssets {
function nodeType(processEvent: ResolverEvent): keyof typeof nodeAssets {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/**
* When the user switches the active descendent of the Resolver.
*/
interface UserFocusedOnResolverNode {
readonly type: 'userFocusedOnResolverNode';
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@oatkiller oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 30, 2020
);
const handleClick = useCallback(() => {
if (animationTarget.current !== null) {
// Use undocumented `beginElement` API on SVG element
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add that link to the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oatkiller
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@oatkiller oatkiller merged commit 503fbfb into elastic:master Mar 31, 2020
oatkiller pushed a commit to oatkiller/kibana that referenced this pull request Mar 31, 2020
Panning, zooming, centering did now always work correctly.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 31, 2020
* 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)
  ...
oatkiller pushed a commit that referenced this pull request Mar 31, 2020
Panning, zooming, centering did now always work correctly.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 1, 2020
* 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 oatkiller deleted the fix-resolver branch March 31, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants