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

ryancogswell · 2019-03-06T03:49:18Z

I have followed (at least) the PR section of the contributing guide.

Closes #14756 and increases MenuList branch coverage from 94.64% to 100%.

Breaking change

Remove the MenuList instance focus() method. You would focus the item you are interested in directly.

…ods (mui#14756) * Remove test dependencies on focus, setTabIndex, and resetTabIndex * Remove focus method from MenuList * Increase MenuList branch coverage from 94.64% to 100%

mui-pr-bot · 2019-03-06T03:53:28Z

Details of bundle changes.

Comparing: 7a49f80...8db8c28

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.04%	-0.04%	371,832	371,670	91,939	91,903
@material-ui/core/Paper	0.00%	0.00%	76,648	76,648	19,308	19,308
@material-ui/core/Paper.esm	0.00%	0.00%	71,599	71,599	18,783	18,783
@material-ui/core/Popper	0.00%	0.00%	30,462	30,462	10,582	10,582
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,286	17,286	5,717	5,717
@material-ui/core/useMediaQuery	0.00%	0.00%	2,486	2,486	1,049	1,049
@material-ui/lab	0.00%	0.00%	184,281	184,281	50,584	50,584
@material-ui/styles	0.00%	0.00%	55,687	55,687	15,825	15,825
@material-ui/system	0.00%	0.00%	17,062	17,062	4,482	4,482
Button	0.00%	0.00%	99,409	99,409	26,607	26,607
Modal	0.00%	0.00%	98,984	98,984	26,263	26,263
colorManipulator	0.00%	0.00%	3,232	3,232	1,296	1,296
docs.landing	0.00%	0.00%	51,891	51,891	11,291	11,291
docs.main	-0.02%	-0.02%	678,326	678,164	206,184	206,145
packages/material-ui/build/umd/material-ui.production.min.js	-0.05%	-0.04%	322,466	322,304	85,044	85,007

Generated by 🚫 dangerJS against 8db8c28

eps1lon

As part of the work for #8191 I'm going to be converting MenuList to a function component instead of a class.
-- #14756

But this PR is indicating that the issue should be closed. What changed?

ryancogswell · 2019-03-06T12:18:08Z

As part of the work for #8191 I'm going to be converting MenuList to a function component instead of a class.
-- #14756

But this PR is indicating that the issue should be closed. What changed?

I changed MenuList.test.js so that it no longer uses any MenuList instance methods. Specifically, all the calls to MenuList.focus() were replaced by calls to initializeFocus which is defined in the test (and MenuList.focus() was completely removed):

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

The test that used to be calling setTabIndex and resetTabIndex was being used to test the following block within resetTabIndex:

    if (currentFocusIndex !== -1) {
      return this.setTabIndex(currentFocusIndex);
    }

I replaced that test with a different one that tests this condition without calling any instance methods explicitly:

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

  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);
    });
  });

MenuList is now ready to convert to a function component, but I'll do that in a separate pull request from these test changes.

eps1lon · 2019-03-06T12:35:33Z

I understood all of that 😄 I was irritated that you wanted to close the issue if not every point is addressed. It doesn't matter to me whether you need open or closed issues for bookkeeping. Just wanted to make sure that you're aware that the issue would be closed when this PR is merged.

ryancogswell · 2019-03-06T12:40:30Z

I wasn’t considering “converting to a function component” part of that issue — just part of the context for why the instance method dependencies need to be removed.

oliviertassinari · 2019-03-06T14:30:38Z

packages/material-ui/src/MenuList/MenuList.js

@@ -93,20 +93,6 @@ class MenuList extends React.Component {
    }
  };

-  focus() {


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.

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.

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.

Sounds like a better approach to me to be honest:

convert to function component

add imperative focus handle via actions

Switch tests to use that imperative handle instead of instance()

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/.

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.

@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

@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.

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.

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.

packages/material-ui/test/integration/MenuList.test.js

ryancogswell added 3 commits March 5, 2019 21:40

[MenuList] Remove focus method and test dependencies on instance meth…

f4137ce

…ods (mui#14756) * Remove test dependencies on focus, setTabIndex, and resetTabIndex * Remove focus method from MenuList * Increase MenuList branch coverage from 94.64% to 100%

Merge branch 'next' into menulist-tests

01b640b

[test] Formatting fixes (yarn prettier)

233d21a

eps1lon added test breaking change MenuList labels Mar 6, 2019

eps1lon reviewed Mar 6, 2019

View reviewed changes

oliviertassinari reviewed Mar 6, 2019

View reviewed changes

packages/material-ui/test/integration/MenuList.test.js Outdated Show resolved Hide resolved

oliviertassinari reviewed Mar 6, 2019

View reviewed changes

packages/material-ui/test/integration/MenuList.test.js Show resolved Hide resolved

[test] Code consistency fixes

8db8c28

oliviertassinari added the PR: good for merge label Mar 7, 2019

oliviertassinari approved these changes Mar 7, 2019

View reviewed changes

oliviertassinari merged commit 0c5342e into mui:next Mar 7, 2019

oliviertassinari mentioned this pull request Mar 8, 2019

[test] Increase the Codecov threshold #14796

Merged

ryancogswell deleted the menulist-tests branch March 10, 2019 18:54

ryancogswell mentioned this pull request Mar 13, 2019

[MenuList] Convert to a function component #14865

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ryancogswell commented Mar 6, 2019 •

edited by oliviertassinari

mui-pr-bot commented Mar 6, 2019 •

edited

eps1lon left a comment

ryancogswell commented Mar 6, 2019

eps1lon commented Mar 6, 2019

ryancogswell commented Mar 6, 2019

oliviertassinari Mar 6, 2019 •

edited

eps1lon Mar 6, 2019

ryancogswell Mar 6, 2019

eps1lon Mar 6, 2019

oliviertassinari Mar 7, 2019 •

edited

ryancogswell Mar 7, 2019

oliviertassinari Mar 7, 2019 •

edited

ryancogswell Mar 7, 2019

oliviertassinari Mar 7, 2019

eps1lon Mar 8, 2019

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

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

Conversation

ryancogswell commented Mar 6, 2019 • edited by oliviertassinari

Breaking change

mui-pr-bot commented Mar 6, 2019 • edited

eps1lon left a comment

Choose a reason for hiding this comment

ryancogswell commented Mar 6, 2019

eps1lon commented Mar 6, 2019

ryancogswell commented Mar 6, 2019

oliviertassinari Mar 6, 2019 • edited

Choose a reason for hiding this comment

eps1lon Mar 6, 2019

Choose a reason for hiding this comment

ryancogswell Mar 6, 2019

Choose a reason for hiding this comment

eps1lon Mar 6, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 7, 2019 • edited

Choose a reason for hiding this comment

ryancogswell Mar 7, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 7, 2019 • edited

Choose a reason for hiding this comment

ryancogswell Mar 7, 2019

Choose a reason for hiding this comment

oliviertassinari Mar 7, 2019

Choose a reason for hiding this comment

eps1lon Mar 8, 2019

Choose a reason for hiding this comment

ryancogswell commented Mar 6, 2019 •

edited by oliviertassinari

mui-pr-bot commented Mar 6, 2019 •

edited

oliviertassinari Mar 6, 2019 •

edited

oliviertassinari Mar 7, 2019 •

edited

oliviertassinari Mar 7, 2019 •

edited