[TreeItem] TreeItem hijacks focus when typing on an input box that updates it.

[AlexKvazos]

I am using a to change the contents of a TreeView and a matching TreeItem is being focused when I backspace.

CodeSandbox: https://codesandbox.io/s/bold-hill-rw9y5

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The focus is lost from the text field and hijacked by the TreeItem

Expected Behavior 🤔

The focus should remain on the TextField since the user never selected or focused on the TreeItem.

1. Go to the sandbox: https://codesandbox.io/s/bold-hill-rw9y5
2. Collapse and expand a category.
3. Click the TextField search box and type "bananas"
4. Start deleting and you'll notice you loose focus mid way before you clear the query.

Context 🔦

I want to be able to filter out items from a tree view.

[eps1lon]

Can't reproduce:

Please include your environment as was requested in the template.

## Your Environment 🌎

<!--
  Include as many relevant details about the environment with which you experienced the bug.
  If you encounter issues with typescript please include version and tsconfig.
-->

| Tech        | Version |
| ----------- | ------- |
| Material-UI | v4.?.?  |
| React       |         |
| Browser     |         |
| TypeScript  |         |
| etc.        |         |

[AlexKvazos]

    "@material-ui/core": "4.9.4",
    "@material-ui/icons": "4.9.1",
    "@material-ui/lab": "4.0.0-alpha.44",
    "react": "16.12.0",
    "react-dom": "16.12.0",

[eps1lon]

    "@material-ui/core": "4.9.4",
    "@material-ui/icons": "4.9.1",
    "@material-ui/lab": "4.0.0-alpha.44",
    "react": "16.12.0",
    "react-dom": "16.12.0",

That is not reproducing what you describe though:

Start deleting and you'll notice you loose focus mid way before you clear the query.

Could you describe the additional steps from your gif that are missing from the initial description?

Also please include your browser.

[AlexKvazos]

@eps1lon My apologies. Honestly it's not exactly the same each time. What I do to reproduce is to open and close the nodes multiple times, and then start typing items in the list and deleting the query again. Repeating the whole process until at some random point it freezes and I loose focus.

Running
Chrome Version 79.0.3945.130 (Official Build) (64-bit) on Mac OS 10.15.3 (19D76)

[eps1lon]

I think I got it including the crash:

The crash happens when I press a after the tree stole focus.

Confirmed on master

[AlexKvazos]

Thank you @eps1lon. Happy to work this out if I can be given any pointers.

Frustration made me just think of a "disableKeyEvents" setting on my project to get around it but we probably need something better.

Let me know and happy to help.

[eps1lon]

With a confirmed reproduction you can work on this. Would be greatly appreciated!

[AlexKvazos]

@eps1lon I think I need a pointer on why this could happen if I want to fix this with a better solution other than "disableKeyEvents".

If you're okay with that quick fix then happy to send a PR just now.

[eps1lon]

I think starting with a test in https://github.com/mui-org/material-ui/blob/master/packages/material-ui-lab/src/TreeView/TreeView.test.js that replays the reproduction would be a good start. Then it might be clearer how the fix should look like.

[joshwooding]

Quick update: Fixed this on the weekend but was having trouble getting a test to pass.

[eps1lon]

Quick update: Fixed this on the weekend but was having trouble getting a test to pass.

Do you have the branch pushed so that I can take a look?

[joshwooding]

@eps1lon I’ll push it when I get home

[joshwooding]

@eps1lon Sorry it took so long!

[tonyhallett]

@joshwooding @eps1lon
There are two issues here.
Firstly there is the focus stealing and secondly the crash.

The crash occurs because the TreeItem is not cleaned up properly by the TreeView.

When the TreeItem is removed it informs the TreeView.

React.useEffect(() => {
    if (removeNodeFromNodeMap) {
      return () => {
        removeNodeFromNodeMap(nodeId);
      };
    }
    return undefined;
  }, [nodeId, removeNodeFromNodeMap]);

The TreeView cleans up the nodeMap

const removeNodeFromNodeMap = React.useCallback(
    id => {
      const nodes = getNodesToRemove(id);
      const newMap = { ...nodeMap.current };

      nodes.forEach(node => {
        const map = newMap[node];
        if (map) {
          if (map.parent) {
            const parentMap = newMap[map.parent];
            if (parentMap && parentMap.children) {
              const parentChildren = parentMap.children.filter(c => c !== node);
              newMap[map.parent] = { ...parentMap, children: parentChildren };
            }
          }

          delete newMap[node];
        }
      });
      nodeMap.current = newMap;
    },
    [getNodesToRemove],
  );

But when we then focus by first character - as did @eps1lon - the nodeMap has been cleaned up but the firstCharMap has not and the map variable is undefined

const focusByFirstCharacter = (id, char) => {
    let start;
    let index;
    const lowercaseChar = char.toLowerCase();

    const firstCharIds = [];
    const firstChars = [];
    
    Object.keys(firstCharMap.current).forEach(nodeId => {
      const firstChar = firstCharMap.current[nodeId];
     
      const map = nodeMap.current[nodeId]; // *****************************************
      const visible = map.parent ? isExpanded(map.parent) : true;

[mlizchap]

Could we just add a null check before map.parent to prevent the crash?

const visible = map && map.parent ? isExpanded(map.parent) : true;

...but there's also the issue of focus stealing. I'm still trying to wrap my head around the details of the component, but I'd like to look at it more tonight when I have more time if no one's currently looking into it.

[joshwooding]

@tonyhallett Thanks for the help. I've already fixed the crash in #19973 but having trouble with the asynchronous nature of the test.

[tonyhallett]

@joshwooding Your test 'works after conditional rendering' deals with ArrowDown.

#19882 (comment)

Is related to focusByFirstCharacter.

@mlizchap

it('should not throw when an item has been removed', () => {
      function TestComponent() {
        const [hide, setState] = React.useState(false);

        return (
          <React.Fragment>
            <button type="button" onClick={() => setState(true)}>
              Hide
            </button>
            <TreeView data-testid='treeView'>
              {!hide && <TreeItem nodeId="test" label="test" />}
              <TreeItem nodeId='focusByFirstChar' data-testid='focusByFirstChar' label='focusByFirstChar'/>

            </TreeView>
          </React.Fragment>
        );
      }
      const { getByText, getByRole } = render(<TestComponent />);
      fireEvent.click(getByText('Hide'));
      expect(()=>{
        fireEvent.keyDown(getByRole('treeitem'), { key: 'a'});
      }).not.to.throw()
    })

and the resolution

const removeNodeFromNodeMap = React.useCallback(
    id => {
      const nodes = getNodesToRemove(id);
      const newMap = { ...nodeMap.current };
      const newFirstCharMap = { ...firstCharMap.current}; //*******
      nodes.forEach(node => {
        const map = newMap[node];
        if(newFirstCharMap[node]){ //******
          delete newFirstCharMap[node];//******
        }
        if (map) {
          if (map.parent) {
            const parentMap = newMap[map.parent];
            if (parentMap && parentMap.children) {
              const parentChildren = parentMap.children.filter(c => c !== node);
              newMap[map.parent] = { ...parentMap, children: parentChildren };
            }
          }

          delete newMap[node];
        }
      });
      nodeMap.current = newMap;
      firstCharMap.current = newFirstCharMap; // *********
    },
    [getNodesToRemove],
  );

[tonyhallett]

I was about to make a pull request for this. Proceed ? If desired could extract the firstCharMap clean up to a separate function.

[joshwooding]

@tonyhallet Sounds good. It's probably better to make a separate function for the firstCharMap clean up part.

[tonyhallett]

@joshwooding Will do. Be tomorrow though

[joshwooding]

@tonyhallett Cool. I would also add a null check for the map.parent in focusByFirstCharacter too.

[tonyhallett]

The sandbox provided https://codesandbox.io/s/bold-hill-rw9y5 illustrates two crashes. The first due to focusByFirstCharacter is still an issue. The second can be thrown with arrow down - this does not occur with the latest build.