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

[MenuList] Remove focus method and test dependencies on instance methods #14757

Merged
merged 4 commits into from
Mar 7, 2019
Merged

[MenuList] Remove focus method and test dependencies on instance methods #14757

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f4137ce
[MenuList] Remove focus method and test dependencies on instance meth…
ryancogswell Mar 6, 2019
01b640b
Merge branch 'next' into menulist-tests
ryancogswell Mar 6, 2019
233d21a
[test] Formatting fixes (yarn prettier)
ryancogswell Mar 6, 2019
8db8c28
[test] Code consistency fixes
ryancogswell Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 0 additions & 14 deletions packages/material-ui/src/MenuList/MenuList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,20 +93,6 @@ class MenuList extends React.Component {
}
};

focus() {
Copy link
Member

Choose a reason for hiding this comment

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

You can find the same method in the RadioGroup component:
https://github.com/mui-org/material-ui/blob/7a49f80111e1b30a28fb442ff69db3fab1b721fe/packages/material-ui/src/RadioGroup/RadioGroup.js#L23

@nathanmarks Wanted to allow people to easily focus the component. However, removing it is fine to me. I doubt a lot of people are using it, it's not very practical and should be even worse in the future: #14761.

Copy link
Member

Choose a reason for hiding this comment

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

We can always think of allowing an imperative handle when converting to a function component e.g.

const menu = React.createRef();
<MenuList actions={menu} />

menu.current.focus();

function MenuList({ actions }) {
  useImperativeHandle(actions, () => {
    return { focus: () => oldFocusLogic(state) };
  })
}

As long as we didn't document it it's not necessarily part of the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to retain the focus functionality via an actions property, I could go ahead and add that (and the conversion to a function component) to this pull request and then officially document it via the actions property. Focus is one of those few things that are legitimate use cases for imperative code. Menu has a similar focus() method and I just realized that it is leveraging MenuList's selectedItemRef instance property in its functionality. So that would also need to change in order to convert MenuList to a function component.

I would recommend that we use the same approach (receiving a ref via an actions property) in both Menu and MenuList (and eventually RadioGroup, but I would rather not bring that into the immediate scope of this MenuList change) and then I would change the Menu focus logic to delegate to MenuList focus logic via the new actions property rather than using MenuList's selectedItemRef directly. This would provide a pattern we could propagate to any other components where similar focus logic would be desirable while avoiding barriers to switching to function components.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a better approach to me to be honest:

  1. convert to function component
  2. add imperative focus handle via actions
  3. Switch tests to use that imperative handle instead of instance()

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to keep this focus method anyway? What's the use case, does it worth the effort? The Menu focuses the first item when opened. Is this the use case you have in mind? Should we keep this behavior? For reference, the dropdowns I can benchmark don't: https://getbootstrap.com/docs/4.3/components/dropdowns/, https://garden.zendesk.com/react-components/menus/, https://evergreen.segment.com/components/menu/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If focusing the first item were the main use case, I'd say we could drop it. In the other libraries you can use Tab or DownArrow after it opens to get focus to the first item which seems fine.

The more problematic scenario is supporting a selected item. It doesn't look like Bootstrap's Dropdown supports that concept. You can style a dropdown item as "active", but I don't think that will bring focus to it when you open it.

Bootstrap doesn't seem to advertise using Dropdown as a replacement for a native select. In the form controls section of their documentation, they only use native selects which then automatically take care of bringing the focus to the selected option when you open them (and the first option is automatically considered selected if nothing is explicitly marked as selected). So to mimic native behavior decently in SelectInput, I think we probably need it.

Copy link
Member

Choose a reason for hiding this comment

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

@ryancogswell It's a great analysis of the situation. We need to focus the active item when the select component opens. The logic should be implemented somewhere we need to keep it.
But, should we keep it in the MenuList component? I'm wondering if we shouldn't move it to the SelectInput.js component.

The menu component is a hybrid between a dropdown and a select component. I'm not happy with the current state of affairs. The material design specification was updated since we first designed it. https://material.io/design/components/menus.html.
Some comments based on what I can see:

  • "Exposed": I have seen some people struggling to make the exposed select variant work. We could add a demo in the select page.
  • "Dropdown menu": We partially support this mode. Yes, we can change the Menu position. But we block the scroll. While locking the scroll on mobile can be useful, we need a property to change the scroll behavior at the very least.
  • Menu focus behavior on open: most of the alternative implementations keep the focus on the element that triggers the menu/dropdown to open. Shouldn't we change the behavior?
  • "Cascading menu": we don't support this use case. [Menu] Support cascading / nested menu #11723

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliviertassinari In the short-term, I would like to add the focus method back in to MenuList via an actions prop so that Menu's focus method can delegate to it since the current Menu.focus implementation won't work once I convert MenuList to a function component. This will allow me to continue this work more incrementally.

I want to convert MenuList to a function component without any changes in functionality so that the code review can assume no intentional changes in behavior and just focus on syntax conventions and how the hooks are being used. This will be a pretty quick increment -- I'm very comfortable with hooks.

After the conversion to a function component, I'll step back and try to absorb the different use cases you've laid out more fully to try to avoid moving forward in the wrong direction. The "Cascading menu" case in particular is one that I would like to have at least a vague picture of how I would go about it (even if I don't try to tackle this in the near term) to see if that influences how the code should evolve.

Menu focus behavior on open: most of the alternative implementations keep the focus on the element that triggers the menu/dropdown to open. Shouldn't we change the behavior?

I think changing the behavior would definitely be preferable for the "exposed" case. It's less clear to me what the behavior should be when the menu opens in a way that covers the triggering element (e.g. our Simple Menu demo). In the end, I think we may want a variant property to differentiate some of these use cases to provide a way to group certain behaviors together (that would otherwise be controlled by multiple lower-level properties) in a way that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with your strategy, one step at the time. I was trying to lay out the global picture. Cascading menu is a big one, I would ignore it for now.

Copy link
Member

Choose a reason for hiding this comment

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

In the short-term, I would like to add the focus method back in to MenuList via an actions

We definitely need an imperative handle for Menu. Ref forwarding is currently blocked for MenuList because

https://github.com/mui-org/material-ui/blob/8a13bc294e04c1af8fd5cf69a287132974b81577/packages/material-ui/src/Menu/Menu.js#L51

In other words: Menu currently needs access to the instance.

const { currentTabIndex } = this.state;
const list = this.listRef;
if (!list || !list.children || !list.firstChild) {
return;
}

if (currentTabIndex && currentTabIndex >= 0) {
list.children[currentTabIndex].focus();
} else {
list.firstChild.focus();
}
}

resetTabIndex() {
const list = this.listRef;
const currentFocus = ownerDocument(list).activeElement;
Expand Down
110 changes: 83 additions & 27 deletions packages/material-ui/test/integration/MenuList.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
import React from 'react';
import React, { useRef, useLayoutEffect } from 'react';
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
import { assert } from 'chai';
import { spy } from 'sinon';
import MenuList from 'packages/material-ui/src/MenuList';
import MenuItem from 'packages/material-ui/src/MenuItem';
import RootRef from 'packages/material-ui/src/RootRef';
import { createMount } from 'packages/material-ui/src/test-utils';

function FocusOnMountMenuItem(props) {
const listItemRef = useRef();
useLayoutEffect(() => {
listItemRef.current.focus();
}, []);
return (
<RootRef rootRef={listItemRef}>
<MenuItem {...props} tabIndex={0} />
</RootRef>
);
}

function assertMenuItemTabIndexed(wrapper, tabIndexed) {
const items = wrapper.find('li[role="menuitem"]');
assert.strictEqual(items.length, 4);
Expand Down Expand Up @@ -33,6 +46,14 @@ function assertMenuItemFocused(wrapper, tabIndexed) {
});
}

function initializeFocus(wrapper) {
wrapper
.find('[tabIndex=0]')
.first()
.getDOMNode()
.focus();
}

describe('<MenuList> integration', () => {
let mount;

Expand All @@ -52,7 +73,7 @@ describe('<MenuList> integration', () => {
<MenuList>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<MenuItem>Menu Item 3</MenuItem>
<MenuItem>Menu Item 4</MenuItem>
</MenuList>,
);
Expand All @@ -65,7 +86,7 @@ describe('<MenuList> integration', () => {
});

it('should select/focus the first item 1', () => {
wrapper.instance().focus();
initializeFocus(wrapper);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 0);
});
Expand All @@ -87,7 +108,7 @@ describe('<MenuList> integration', () => {
});

it('should focus the second item 1', () => {
wrapper.instance().focus();
initializeFocus(wrapper);
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 1);
assertMenuItemFocused(wrapper, 1);
Expand All @@ -110,20 +131,8 @@ describe('<MenuList> integration', () => {
}, 60);
});

it('should reset the tabIndex to the focused element when calling resetTabIndex', () => {
wrapper.instance().focus();
wrapper.simulate('keyDown', { key: 'ArrowDown' });
wrapper.instance().setTabIndex(2);
wrapper.instance().resetTabIndex();

assertMenuItemTabIndexed(wrapper, 1);
assertMenuItemFocused(wrapper, 1);

resetWrapper();
});

it('should select/focus the first item 2', () => {
wrapper.instance().focus();
initializeFocus(wrapper);
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 0);
});
Expand Down Expand Up @@ -161,7 +170,7 @@ describe('<MenuList> integration', () => {
<MenuList>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem selected>Menu Item 2</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<MenuItem>Menu Item 3</MenuItem>
<MenuItem>Menu Item 4</MenuItem>
</MenuList>,
);
Expand All @@ -174,13 +183,13 @@ describe('<MenuList> integration', () => {
});

it('should select/focus the second item', () => {
wrapper.instance().focus();
initializeFocus(wrapper);
assertMenuItemTabIndexed(wrapper, 1);
assertMenuItemFocused(wrapper, 1);
});

it('should focus the third item', () => {
wrapper.instance().focus();
initializeFocus(wrapper);
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 2);
assertMenuItemFocused(wrapper, 2);
Expand All @@ -199,14 +208,61 @@ describe('<MenuList> integration', () => {
});
});

it('should not crash and burn when calling focus() on an empty MenuList', () => {
const wrapper = mount(<MenuList />);
wrapper.instance().focus();
describe('MenuItem with focus on mount', () => {
let wrapper;

const resetWrapper = () => {
wrapper = mount(
<MenuList>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<FocusOnMountMenuItem>Menu Item 3</FocusOnMountMenuItem>
<MenuItem>Menu Item 4</MenuItem>
</MenuList>,
);
};

before(resetWrapper);

it('should have the 3nd item tabIndexed and focused', () => {
assertMenuItemTabIndexed(wrapper, 2);
assertMenuItemFocused(wrapper, 2);
});
});

it('should not crash and burn when calling focus() on an unmounted MenuList', () => {
const wrapper = mount(<MenuList />);
delete wrapper.instance().list;
wrapper.instance().focus();
describe('MenuList with disableListWrap', () => {
let wrapper;

const resetWrapper = () => {
wrapper = mount(
<MenuList disableListWrap>
<MenuItem>Menu Item 1</MenuItem>
<MenuItem>Menu Item 2</MenuItem>
<MenuItem>Menu Item 3</MenuItem>
<MenuItem>Menu Item 4</MenuItem>
</MenuList>,
);
};

before(resetWrapper);

it('should not wrap focus with ArrowUp from first', () => {
initializeFocus(wrapper);
wrapper.simulate('keyDown', { key: 'ArrowUp' });
assertMenuItemTabIndexed(wrapper, 0);
assertMenuItemFocused(wrapper, 0);
});
it('should not wrap focus with ArrowDown from last', () => {
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
initializeFocus(wrapper);
wrapper.simulate('keyDown', { key: 'ArrowDown' });
wrapper.simulate('keyDown', { key: 'ArrowDown' });
wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 3);
assertMenuItemFocused(wrapper, 3);

wrapper.simulate('keyDown', { key: 'ArrowDown' });
assertMenuItemTabIndexed(wrapper, 3);
assertMenuItemFocused(wrapper, 3);
});
});
});