[DataGrid] Prevents bubbling in menu header

[alexfauquette]

Fix #1721

This prevents bubbling events to the menu header. The bubbling was problematic when people click on a modal, the event reach the menu header, and so modified the sorting.

[m4theushw]

Looks good. Could you add at least two test cases: 1. ensuring that when [role="menu"] receives a click, it won't change the sort and 2. ensuring that when "Sort by ASC" it does change the sort? You can use as example: /~https://github.com/mui-org/material-ui-x/blob/0512a1399cf42c2257d72462a9161f23efac0b20/packages/grid/x-grid/src/tests/columnHeaders.DataGridPro.test.tsx#L44 and /~https://github.com/mui-org/material-ui-x/blob/0512a1399cf42c2257d72462a9161f23efac0b20/packages/grid/data-grid/src/tests/sorting.DataGrid.test.tsx#L78-L80

The PR title should follow the pattern [Component] Imperative commit message. Could you fix it?

[alexfauquette]

Test added, And I confirm the first one "should not modify column order when menu is clicked" catch the bug (It does not pass on master) :)

[oliviertassinari]

This is the main upside of running the tests in the same JavaScript runtime as the component we don't need to wait in the large majority of the cases. Here, it runs synchronously. (fireEvent.click wraps the action with a React. act that tell him to be synchronous).

Suggested change

	await waitFor(() => expect(getColumnValues(0)).to.deep.equal(['Nike', 'Adidas', 'Puma']));
	expect(getColumnValues(0)).to.deep.equal(['Nike', 'Adidas', 'Puma']);

[alexfauquette]

OK, I observed that test break if I remove waitFor to test that "menu" has been removed.

Is that because removing the menu first trigger an animation, and then delete the html element?

[oliviertassinari]

Regarding the menu, we don't mock the time for it, so we have to wait for the transition to start. It's a different case.

[oliviertassinari]

It's probably this one /~https://github.com/reactjs/react-transition-group/blob/5aa3fd2d7e3354a7e42505d55af605ff44f74e2e/src/Transition.js#L326 that prevents us to be writing the test synchronously.

In the main repo, we mock the time for it.

[m4theushw]

There're several other cases where we wait for the translation instead of mocking the time: /~https://github.com/mui-org/material-ui-x/blob/adc0468e18044f34cdffe7a084cfdab2f45bab62/packages/grid/data-grid/src/tests/rows.DataGrid.test.tsx#L208

I think we can leave as it is and fix this the other tests in a another PR.

[alexfauquette]

Sounds good to me. I open an issue for smoking time pointing to @oliviertassinari comment for mocking time, and merge this PR?

[oliviertassinari]

My first comment was really about using async/await to its strict minimum. We removed the await on the get column value, which ​is synchronous, great.

I would recommend we revert the changes on the tests that are not related to the bug we go after. I personally think that it's OK-ish it use waitFor with the menu transition.

[oliviertassinari]

Solves #1721

This is not recognized by GitHub: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

[m4theushw]

I pushed two commits to improve the tests. Always give preference to getByRole / queryByRole because doing that we are also checking that the UI is accessible: https://testing-library.com/docs/queries/about#priority