[Select] Simplify blur logic

packages/material-ui/src/Select/Select.test.js

@@ -88,33 +88,43 @@ describe('<Select />', () => {
     expect(container.querySelector('input')).to.have.property('type', 'hidden');
   });
 
-  // TODO: leaky abstraction
-  it('should ignore onBlur the first time the menu is open', () => {
+  it('should ignore onBlur when the menu opens', () => {
     // mousedown calls focus while click opens moving the focus to an item
     // this means the trigger is blurred immediately
     const handleBlur = spy();
-    const { getByRole, getAllByRole } = render(
-      <Select onBlur={handleBlur} value="">
+    const { getByRole, getAllByRole, queryByRole } = render(
+      <Select
+        onBlur={handleBlur}
+        onMouseDown={event => {
+          // simulating certain platforms that focus on mousedown
+          if (event.defaultPrevented === false) {
+            event.currentTarget.focus();
+          }
+        }}
+        value=""
+      >
         <MenuItem value="">none</MenuItem>
         <MenuItem value={10}>Ten</MenuItem>
       </Select>,
     );
 
     const trigger = getByRole('button');
     // simulating user click
-    fireEvent.mouseDown(trigger);
-    trigger.focus();
-    trigger.click();
+    act(() => {
+      fireEvent.mouseDown(trigger);
+      trigger.click();
+    });
 
     expect(handleBlur.callCount).to.equal(0);
     expect(getByRole('listbox')).to.be.ok;
 
     act(() => {
+      fireEvent.mouseDown(getAllByRole('option')[0]);
       getAllByRole('option')[0].click();
     });
-    trigger.blur();
 
-    expect(handleBlur.callCount).to.equal(1);
+    expect(handleBlur.callCount).to.equal(0);
+    expect(queryByRole('listbox')).to.be.null;
   });
 
   it('options should have a data-value attribute', () => {

packages/material-ui/src/Select/SelectInput.js

@@ -55,58 +55,49 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   } = props;
 
   const inputRef = React.useRef(null);
-  const displayRef = React.useRef(null);
-  const ignoreNextBlur = React.useRef(false);
+  const [displayNode, setDisplaNode] = React.useState(null);
   const { current: isOpenControlled } = React.useRef(openProp != null);
   const [menuMinWidthState, setMenuMinWidthState] = React.useState();
   const [openState, setOpenState] = React.useState(false);
-  const [, forceUpdate] = React.useState(0);
   const handleRef = useForkRef(ref, inputRefProp);
 
   React.useImperativeHandle(
     handleRef,
     () => ({
       focus: () => {
-        displayRef.current.focus();
+        displayNode.focus();
       },
       node: inputRef.current,
       value,
     }),
-    [value],
+    [displayNode, value],
   );
 
   React.useEffect(() => {
-    if (isOpenControlled && openProp) {
-      // Focus the display node so the focus is restored on this element once
-      // the menu is closed.
-      displayRef.current.focus();
-      // Rerender with the resolve `displayRef` reference.
-      forceUpdate(n => !n);
+    if (autoFocus && displayNode) {
+      displayNode.focus();
     }
-
-    if (autoFocus) {
-      displayRef.current.focus();
-    }
-  }, [autoFocus, isOpenControlled, openProp]);
+  }, [autoFocus, displayNode]);
 
   const update = (open, event) => {
     if (open) {
       if (onOpen) {
         onOpen(event);
       }
-    } else if (onClose) {
-      onClose(event);
+    } else {
+      displayNode.focus();
+      if (onClose) {
+        onClose(event);
+      }
     }
 
     if (!isOpenControlled) {
-      setMenuMinWidthState(autoWidth ? null : displayRef.current.clientWidth);
+      setMenuMinWidthState(autoWidth ? null : displayNode.clientWidth);
       setOpenState(open);
     }
   };
 
   const handleClick = event => {
-    // Opening the menu is going to blur the. It will be focused back when closed.
-    ignoreNextBlur.current = true;
     update(true, event);
   };
 
@@ -140,21 +131,6 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     }
   };
 
-  const handleBlur = event => {
-    if (ignoreNextBlur.current === true) {
-      // The parent components are relying on the bubbling of the event.
-      event.stopPropagation();
-      ignoreNextBlur.current = false;
-      return;
-    }
-
-    if (onBlur) {
-      event.persist();
-      event.target = { value, name };
-      onBlur(event);
-    }
-  };
-
   const handleKeyDown = event => {
     if (!readOnly) {
       const validKeys = [
@@ -168,14 +144,21 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
 
       if (validKeys.indexOf(event.key) !== -1) {
         event.preventDefault();
-        // Opening the menu is going to blur the. It will be focused back when closed.
-        ignoreNextBlur.current = true;
         update(true, event);
       }
     }
   };
 
-  const open = isOpenControlled && displayRef.current ? openProp : openState;
+  const open = displayNode !== null && (isOpenControlled ? openProp : openState);
+
+  const handleBlur = event => {
+    // if open event.stopImmediatePropagation
+    if (!open && onBlur) {
+      event.persist();
+      event.target = { value, name };
+      onBlur(event);
+    }
+  };
 
   delete other['aria-invalid'];
 
@@ -247,8 +230,8 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   // Avoid performing a layout computation in the render method.
   let menuMinWidth = menuMinWidthState;
 
-  if (!autoWidth && isOpenControlled && displayRef.current) {
-    menuMinWidth = displayRef.current.clientWidth;
+  if (!autoWidth && isOpenControlled && displayNode) {
+    menuMinWidth = displayNode.clientWidth;
   }
 
   let tabIndex;
@@ -271,16 +254,16 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
           },
           className,
         )}
-        ref={displayRef}
+        ref={setDisplaNode}
         data-mui-test="SelectDisplay"
         tabIndex={tabIndex}
         role="button"
         aria-expanded={open ? 'true' : undefined}
         aria-haspopup="listbox"
         aria-owns={open ? `menu-${name || ''}` : undefined}
         onKeyDown={handleKeyDown}
-        onBlur={handleBlur}
         onClick={disabled || readOnly ? null : handleClick}
+        onBlur={handleBlur}
         onFocus={onFocus}
         // The id can help with end-to-end testing automation.
         id={name ? `select-${name}` : undefined}
@@ -305,7 +288,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       <IconComponent className={clsx(classes.icon, classes[`icon${capitalize(variant)}`])} />
       <Menu
         id={`menu-${name || ''}`}
-        anchorEl={displayRef.current}
+        anchorEl={displayNode}
         open={open}
         onClose={handleClose}
         {...MenuProps}

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

@@ -1,18 +1,16 @@
 import React from 'react';
 import { expect } from 'chai';
-import { cleanup, createClientRender, wait } from 'test/utils/createClientRender';
+import { createClientRender, fireEvent, wait } from 'test/utils/createClientRender';
 import MenuItem from '@material-ui/core/MenuItem';
 import Select from '@material-ui/core/Select';
 import Dialog from '@material-ui/core/Dialog';
+import FormControl from '@material-ui/core/FormControl';
+import InputLabel from '@material-ui/core/InputLabel';
 
 describe('<Select> integration', () => {
   // StrictModeViolation: uses Fade
   const render = createClientRender({ strict: false });
 
-  afterEach(() => {
-    cleanup();
-  });
-
   describe('with Dialog', () => {
     function SelectAndDialog() {
       const [value, setValue] = React.useState(10);
@@ -77,4 +75,59 @@ describe('<Select> integration', () => {
       expect(getByRole('button')).to.have.text('Twenty');
     });
   });
+
+  describe('with label', () => {
+    // we're somewhat abusing "focus" here. What we're actually interested in is
+    // displaying it as "active". WAI-ARIA authoring practices do not consider the
+    // the trigger part of the widget while a native <select /> will outline the trigger
+    // as well
+    it('is displayed as focused while open', () => {
+      const { container, getByRole } = render(
+        <FormControl>
+          <InputLabel classes={{ focused: 'focused-label' }} htmlFor="age-simple">
+            Age
+          </InputLabel>
+          <Select inputProps={{ id: 'age' }} value="">
+            <MenuItem value="">none</MenuItem>
+            <MenuItem value={10}>Ten</MenuItem>
+          </Select>
+        </FormControl>,
+      );
+
+      const trigger = getByRole('button');
+      trigger.focus();
+      fireEvent.keyDown(document.activeElement, { key: 'Enter' });
+
+      expect(container.querySelector('[for="age-simple"]')).to.have.class('focused-label');
+    });
+
+    it('does not stays in an active state if an open action did not actually open', () => {
+      // test for https://github.com/mui-org/material-ui/issues/17294
+      // we used to set a flag to stop blur propagation when we wanted to open the
+      // select but never considered what happened if the select never opened
+      const { container, getByRole } = render(
+        <FormControl>
+          <InputLabel classes={{ focused: 'focused-label' }} htmlFor="age-simple">
+            Age
+          </InputLabel>
+          <Select inputProps={{ id: 'age' }} open={false} value="">
+            <MenuItem value="">none</MenuItem>
+            <MenuItem value={10}>Ten</MenuItem>
+          </Select>
+        </FormControl>,
+      );
+
+      getByRole('button').focus();
+
+      expect(container.querySelector('[for="age-simple"]')).to.have.class('focused-label');
+
+      fireEvent.keyDown(document.activeElement, { key: 'Enter' });
+
+      expect(container.querySelector('[for="age-simple"]')).to.have.class('focused-label');
+
+      getByRole('button').blur();
+
+      expect(container.querySelector('[for="age-simple"]')).not.to.have.class('focused-label');
+    });
+  });
 });