fix(Dropdown): prevent unnecessary duplicate VO readings

[emyarod]

Closes #7707

This PR prevents VO from reading listbox items twice except for items whose width exceeds the listbox menu width

Changelog

Changed

• attach title only when menu item option width exceeds listbox menu width
• forward refs in ListBoxMenuItem

Testing / Reviewing

Ensure that native HTML tooltips are only added to long listbox menu item options with ellipses

[netlify]

Deploy preview for carbon-elements ready!

Built with commit 77a9187

https://deploy-preview-7768--carbon-elements.netlify.app

[netlify]

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 77a9187

https://deploy-preview-7768--carbon-components-react.netlify.app

[joshblack]

Would it make sense to do this check in the menu item component itself? Something like

const ListBoxMenuItem = ({ children, isActive, isHighlighted, title, ...rest }) => {
  const ref = useRef(null);
  const truncated = useTruncated(ref);

  // ...

  return (
    <div {...rest} className={className} title={truncated ? title : undefined}>
      <div
        className={`${prefix}--list-box__menu-item__option`}
        ref={ref}>
        {children}
      </div>
    </div>
  );
};

function useTruncated(ref) {
  const [truncated, setTruncated] = useState(false);

  useLayoutEffect(() => {
    const { offsetWidth, scrollWidth } = ref.current;
    if (offsetWidth < scrollWidth) {
      setTruncated(true);
    } else {
      setTruncated(false);
    }
  });

  return truncated;
}

[emyarod]

I tried this to begin with but Downshift was not playing nice with hooks in the ListBoxMenuItem. Did this approach work in your testing?

[joshblack]

@emyarod I see what you mean, surprisingly it changes depending on if you are viewing the story in the storybook or in a standalone/full screen view 🤔 (meaning it works when full screen)

Would be good to figure out what's going on with it for the "ideal" setup, I wonder if it's just the way that we're rendering items versus some weird downshift issue. I think reading from a ref in render is one of those weird forbidden rules

Read a property from an object, unless that object was "newly created" or if the property will never change after it is read once.

Since reading the ref can change between renders. Quick example: https://codesandbox.io/s/dazzling-leftpad-ql8b1?file=/src/App.js

[emyarod]

it changes depending on if you are viewing the story in the storybook or in a standalone/full screen view 🤔 (meaning it works when full screen)

I wasn't seeing this from my testing and the error was appearing in all views. that's what initially led me to believe that it's Downshift related and also why I began exploring forwarding refs to the listbox menu item

[emyarod]

do we want to continue exploring our initial idea of using hooks in ListBoxMenuItem or should we proceed with something else?

[joshblack]

@emyarod would be great to figure out what's going on with using hooks in ListBoxMenuItem 🤔 I think that'd be ideal for this setup. I wonder if the issue with downshift is a small one or if it really is a broader issue for the library.

[emyarod]

the problem is we are using hooks in class based components. I have refactored ComboBox to a function component but in order to preserve compatibility with MultiSelect and Dropdown I still need to forward the ref with the object ref syntax. I didn't want to refactor those other ListBox components in this PR but I can do so in separate PRs so that we will no longer need to forward refs to ListBox.MenuItem

[joshblack]

@emyarod definitely agreed that hooks don't make sense in class-based components. I think from above what was surprising was that using hooks in one of the components rendered by the class-based component was violating the rules of hooks as we should be able to call/render components that use hooks inside of components that are class-based.

[emyarod]

gotcha do you have something else in mind to resolve the error? from my testing refactoring the component was the only way I could resolve the issue so I rolled it into this PR

[emyarod]

I assume this PR review was held up for #7777 due to the massive merge conflicts

[dakahn]

Getting back to this sorry -- reads reaally well. Also reads great in NVDA and JAWS 2020 on Windows latest with Edge and Firefox

[TheSharpieOne]

Without a dependency array, this will trigger the onInputChange with every render.

This should only be triggered when the inputValue changes

  useEffect(() => {
    if (onInputChange) {
      onInputChange(inputValue);
    }
  }, [inputValue]); // <-- it should have this!

[emyarod]

related #8306 #8308