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.

Sign up for GitHub

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

Jump to bottom

Fix resolver #61886

Merged
merged 12 commits into from
Mar 31, 2020
Merged

Fix resolver #61886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e8f7035
Revert "Resolver/nodedesign 25 (#60630)"
Mar 30, 2020
68d6d6f
Revert "Revert "Resolver/nodedesign 25 (#60630)""
Mar 30, 2020
6e63b1b
style fixes
Mar 30, 2020
569e56a
DNM debug changes
Mar 30, 2020
35c4b9d
fix issue with adjacency node map being passed down
Mar 30, 2020
1cab6d2
Revert "DNM debug changes"
Mar 30, 2020
192fcba
Prevent SymbolDefinitions from effecting the layout of the resolver g…
Mar 30, 2020
797a29c
remove object allocations
Mar 30, 2020
2dc6242
update comment on action
Mar 30, 2020
0b505f5
Merge branch 'master' into fix-resolver
elasticmachine Mar 31, 2020
0c8c03e
fix type error
Mar 31, 2020
d658188
remove any type and fix comment on beginElement
Mar 31, 2020
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
3 changes: 2 additions & 1 deletion x-pack/plugins/endpoint/public/embeddables/resolver/store/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,15 @@ interface UserChangedSelectedEvent {
interface AppRequestedResolverData {
readonly type: 'appRequestedResolverData';
}

/**
* 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?

*/
readonly nodeId: string;
};
Expand Down
35 changes: 22 additions & 13 deletions x-pack/plugins/endpoint/public/embeddables/resolver/view/defs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
euiPaletteForStatus,
colorPalette,
} from '@elastic/eui';
import styled from 'styled-components';

/**
* Generating from `colorPalette` function: This could potentially
Expand Down Expand Up @@ -396,17 +397,25 @@ const SymbolsAndShapes = memo(() => (
));

/**
* This <defs> element is used to define the reusable assets for the Resolver
* It confers sevral advantages, including but not limited to:
* 1) Freedom of form for creative assets (beyond box-model constraints)
* 2) Separation of concerns between creative assets and more functional areas of the app
* 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

* 2. Separation of concerns between creative assets and more functional areas of the app
* 3. `<use>` elements can be handled by compositor (faster)
*/
export const SymbolDefinitions = memo(() => (
<svg>
<defs>
<PaintServers />
<SymbolsAndShapes />
</defs>
</svg>
));
export const SymbolDefinitions = styled(
memo(({ className }: { className?: string }) => (
<svg className={className}>
<defs>
<PaintServers />
<SymbolsAndShapes />
</defs>
</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.

left: 100%;
top: 100%;
width: 0;
height: 0;
`;
25 changes: 16 additions & 9 deletions x-pack/plugins/endpoint/public/embeddables/resolver/view/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,22 @@ export const Resolver = styled(
projectionMatrix={projectionMatrix}
/>
))}
{Array.from(processNodePositions).map(([processEvent, position], index) => (
<ProcessEventDot
key={index}
position={position}
projectionMatrix={projectionMatrix}
event={processEvent}
adjacentNodeMap={processToAdjacencyMap.get(processEvent)}
/>
))}
{[...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.

// This should never happen
throw new Error('Issue calculating adjacency node map.');
}
return (
<ProcessEventDot
key={index}
position={position}
projectionMatrix={projectionMatrix}
event={processEvent}
adjacentNodeMap={adjacentNodeMap}
/>
);
})}
</StyledResolverContainer>
)}
<StyledPanel />
Expand Down
71 changes: 37 additions & 34 deletions x-pack/plugins/endpoint/public/embeddables/resolver/view/process_event_dot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

}) => {
/**
* Convert the position, which is in 'world' coordinates, to screen coordinates.
Expand All @@ -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


const nodeViewportStyle = useMemo(
() => ({
Expand All @@ -117,27 +117,31 @@ export const ProcessEventDot = styled(

const labelYHeight = markerSize / 1.7647;

const levelAttribute = adjacentNodeMap?.level
? {
'aria-level': adjacentNodeMap.level,
}
: {};

const flowToAttribute = adjacentNodeMap?.nextSibling
? {
'aria-flowto': adjacentNodeMap.nextSibling,
}
: {};
/**
* An element that should be animated when the node is clicked.
*/
const animationTarget: {
current:
| (SVGAnimationElement & {
/**
* `beginElement` is by [w3](https://www.w3.org/TR/SVG11/animate.html#__smil__ElementTimeControl__beginElement)
* but missing in [TSJS-lib-generator](https://github.com/microsoft/TSJS-lib-generator/blob/15a4678e0ef6de308e79451503e444e9949ee849/inputfiles/addedTypes.json#L1819)
*/
beginElement: () => void;
})
| null;
} = React.createRef();
const { cubeSymbol, labelFill, descriptionFill, descriptionText } = nodeAssets[
nodeType(event)
];
const resolverNodeIdGenerator = useMemo(() => htmlIdGenerator('resolverNode'), []);

const nodeType = getNodeType(event);
const clickTargetRef: { current: SVGAnimationElement | null } = React.createRef();
const { cubeSymbol, labelFill, descriptionFill, descriptionText } = nodeAssets[nodeType];
const resolverNodeIdGenerator = htmlIdGenerator('resolverNode');
const [nodeId, labelId, descriptionId] = [
!!selfId ? resolverNodeIdGenerator(String(selfId)) : resolverNodeIdGenerator(),
resolverNodeIdGenerator(),
resolverNodeIdGenerator(),
] as string[];
const nodeId = useMemo(() => resolverNodeIdGenerator(selfId), [
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

resolverNodeIdGenerator,
selfId,
]);
const labelId = 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 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 dispatch = useResolverDispatch();

Expand All @@ -154,14 +158,11 @@ export const ProcessEventDot = styled(
[dispatch, nodeId]
);

const handleClick = useCallback(
(clickEvent: React.MouseEvent<SVGSVGElement, MouseEvent>) => {
if (clickTargetRef.current !== null) {
(clickTargetRef.current as any).beginElement();
}
},
[clickTargetRef]
);
const handleClick = useCallback(() => {
if (animationTarget.current !== null) {
animationTarget.current.beginElement();
}
}, [animationTarget]);

return (
<EuiKeyboardAccessible>
Expand All @@ -171,8 +172,10 @@ 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

aria-flowto={
adjacentNodeMap.nextSibling === null ? undefined : adjacentNodeMap.nextSibling
}
aria-labelledby={labelId}
aria-describedby={descriptionId}
aria-haspopup={'true'}
Expand Down Expand Up @@ -202,7 +205,7 @@ export const ProcessEventDot = styled(
begin="click"
repeatCount="1"
className="squish"
ref={clickTargetRef}
ref={animationTarget}
/>
</use>
<use
Expand Down Expand Up @@ -273,7 +276,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.

const processType = processModel.eventType(processEvent);

if (processType in processTypeToCube) {
Expand Down