Terra-core: [terra-responsive-element] Does not honor proper React.useEffect behavior

Created on 9 Oct 2019 · 8Comments · Source: cerner/terra-core

Bug Report

Description

terra-responsive-element doesn't work well when used with React.useEffect, specifically when creating an effect that focuses on an element when mounting, e.g. focus on a button:

function Example() {
  const buttonRef = React.useRef(null);

  React.useEffect(() => {
    const { current: button } = buttonRef;
    button.focus();
  }, []); // only on mount (empty dep list means on mount)

  return (
    <button type="button" ref={buttonRef}>A button with a ref</button>
  );
}

This pattern is common when using modals (i.e. terra-modal-manager) to shift focus into the modal (from the body) when opened (mounted) for a11y and proper focus management.

Some consumers are using terra-responsive-element in the following way:

// pretend that this was mounted using `disclosureManager.disclose({ ... })`
function ModalContent() {
  const titleRef = React.useRef(null);

  React.useEffect(() => {
    const { current: title } = titleRef;
    if (title) {
      // this will never fire, using this example setup
      title.focus(); 
    } else {
      console.error("No title ref to focus");
    }
  }, []);

  const tinyContent = (
    <div>
      <h1 tabIndex="0" ref={titleRef}>Tiny Title Here</h1>
      <p>Tiny content here</p>
      <button type="button">Some other focusable thing</button>
    </div>
  );

  const largeContent = (
    <div>
      <h1 tabIndex="0" ref={titleRef}>Large Title Here</h1>
      <p>Large content here</p>
      <button type="button">Some other focusable thing</button>
    </div>
  );

  return (
    <ResponsiveElement
      responsiveTo="window"
      tiny={tinyContent}
      large={largeContent}
    />
  );
}

I have reservations about this pattern (should probably hoist the ResponsiveElement above or use the React.useContext(ActiveBreakpointContext) from terra-breakpoints), but it doesn't work as expected.

Steps to Reproduce

Check out the code/deployments below:

Here's a minimal (working?, not as expected tho) example using terra-responsive-element:

Another example that works as expected, using terra-breakpoints inside terra-modal-manager (as described in the example above):

Additional Context / Screenshots

Expected Behavior

React.useEffect() (especially, React.useEffect(() => { ... }, []) expected behavior should be honored.

Possible Solution

I would probably suggest either deprecating ResponsiveElement in preference for using terra-breakpoints ActiveBreakpointContext (like in the example repo I linked above), since it can be heavy when used in actual apps (at least as I have seen consumers use it) or providing guidance on better patterns of use (e.g. hoisting above, like state, and using it like you would with ActiveBreakpointContext).

Environment

Component Name and Version: terra-responsive-element 5.10.0

terra-responsive-element bug

Source

dev-cprice

All 8 comments

This is likely a side effect of the same issue here: https://github.com/cerner/terra-core/issues/2673

The uncontrolled responsive element doesn't know which element to render until it's mounted and calculated the available size.

At first glance I don't think this is something that can be resolved with the uncontrolled responsive element. However I'd be interested in seeing if the issue is the same with the controlled responsive element.

Would you be willing to recreate these examples with the controlled responsive element and see if the same issues are encountered?

https://engineering.cerner.com/terra-ui/components/terra-responsive-element/responsive-element/responsive-element

Example from the doc site:

import React, { useState } from 'react';
import Placeholder from 'terra-doc-template/lib/Placeholder';
import ResponsiveElement from 'terra-responsive-element';

const BreakpointExample = () => {
  const [breakpoint, setBreakpoint] = useState('');

  return (
    <ResponsiveElement onChange={value => setBreakpoint(value)}>
      <Placeholder title={breakpoint} />
    </ResponsiveElement>
  );
};

export default BreakpointExample;

StephenEsser on 9 Oct 2019

👍1

Yeah, when looking at ways to fix it, nothing stood out to me as possible... Didn't even see there was a controlled responsive element now 😮

I'll update the first repo (minimal example) with an example using controlled real quick 👍

dev-cprice on 9 Oct 2019

Okay, updated the MVE: https://dev-cprice.github.io/responsive-element-use-effect-bug/

code: https://github.com/dev-cprice/responsive-element-use-effect-bug

has a checkbox to switch between controlled and uncontrolled responsive element...

No change in behavior that I can see. However, I am still using the tiny & large props and not children.

dev-cprice on 9 Oct 2019

The controlled responsive element is expecting children to be used. Adding any content into the preset breakpoint props (tiny/large) will flip the component back into an uncontrolled component. I think to test this we'll want to use the children prop.

The controlled component example should end up looking very similar to the terra-breakpoints example.

StephenEsser on 9 Oct 2019

Ah, I suspected that. I'll redeploy again 🙃

dev-cprice on 9 Oct 2019

Okay, round 3. Looks like the useEffect mount effect is working as expected. Of note though, the example doesn't end up quite 1-to-1 with the terra-breakpoints code.

If we do something like:

export default function Controlled() {
  const titleRef = React.useRef(null);
  const [titleFocusedProperly, setTitleFocusedProperly] = React.useState(false);
  const [activeBreakpoint, setActiveBreakpoint] = React.useState("");

  React.useEffect(() => {
    const { current: title } = titleRef;

    if (title) {
      title.focus();
      setTitleFocusedProperly(true);
    } else {
      console.warn("Controlled: Couldn't focus on the title...");
      setTitleFocusedProperly(false);
    }
  }, []);

  const large = (
    <div>
      <span tabIndex="0" ref={titleRef}>
        Large Title Here
      </span>
      <p>Large Content Here</p>
      <Button text="Focusable Button" />
    </div>
  );

  const tiny = (
    <div>
      <span tabIndex="0" ref={titleRef}>
        Tiny Title Here
      </span>
      <p>Tiny Content Here</p>
      <Button text="Focusable Button" />
    </div>
  );

  const content = React.useMemo(() => {
    switch (activeBreakpoint) {
      case "large":
      case "huge":
      case "enormous":
        return large;
      default:
        return tiny;
    }
  }, [activeBreakpoint, large, tiny]);

  const onChange = React.useCallback(value => setActiveBreakpoint(value), []);

  return (
    <div>
      <h1>Controlled</h1>
      <p>Title focused properly? {titleFocusedProperly.toString()}</p>
      <ResponsiveElement responsiveTo="window" onChange={onChange}>
        {content}
      </ResponsiveElement>
    </div>
  );
}

Then the React.useEffect(() => { ... }, []); will only run once: on App.js mount. To be fair, this is expected behavior.

Instead, I used composition to re-render the Large and Tiny components, instead of using computed values inside the App component:

function Large() {
  const titleRef = React.useRef(null);
  const [titleFocusedProperly, setTitleFocusedProperly] = React.useState(false);

  React.useEffect(() => {
    const { current: title } = titleRef;

    if (title) {
      title.focus();
      setTitleFocusedProperly(true);
    } else {
      console.warn("Controlled: Couldn't focus on the large title...");
      setTitleFocusedProperly(false);
    }
  }, []);

  return (
    <div>
      <p>Title focused properly? {titleFocusedProperly.toString()}</p>
      <span tabIndex="0" ref={titleRef}>
        Large Title Here
      </span>
      <p>Large Content Here</p>
      <Button text="Focusable Button" />
    </div>
  );
}

function Tiny() {
  const titleRef = React.useRef(null);
  const [titleFocusedProperly, setTitleFocusedProperly] = React.useState(false);

  React.useEffect(() => {
    const { current: title } = titleRef;

    if (title) {
      title.focus();
      setTitleFocusedProperly(true);
    } else {
      console.warn("Controlled: Couldn't focus on the tiny title...");
      setTitleFocusedProperly(false);
    }
  }, []);

  return (
    <div>
      <p>Title focused properly? {titleFocusedProperly.toString()}</p>
      <span tabIndex="0" ref={titleRef}>
        Tiny Title Here
      </span>
      <p>Tiny Content Here</p>
      <Button text="Focusable Button" />
    </div>
  );
}

export default function Controlled() {
  const [activeBreakpoint, setActiveBreakpoint] = React.useState("");

  const content = React.useMemo(() => {
    switch (activeBreakpoint) {
      case "large":
      case "huge":
      case "enormous":
        return <Large />;
      default:
        return <Tiny />;
    }
  }, [activeBreakpoint]);

  const onChange = React.useCallback(value => setActiveBreakpoint(value), []);

  return (
    <div>
      <h1>Controlled</h1>
      <ResponsiveElement responsiveTo="window" onChange={onChange}>
        {content}
      </ResponsiveElement>
    </div>
  );
}

This works as the terra-breakpoints example does. And it makes sense why, for sure.

Going forward, should we deprecate the uncontrolled terra-responsive-element API? I can see this biting people, especially when using hooks like React.useEffect.

dev-cprice on 9 Oct 2019

We may want to address the point you made in the summary of this issue first and determine if we want to deprecate the terra-responsive-element completely.

Components using the terra-responsive-element with responsiveTo set to window should be able to uplift onto the terra-breakpoints package and remove the dependency on the responsive element all together. But components and projects using the responsive element to be responsive to their parent containers may have a hard time finding an alternative solution to the responsive element.

I think there is value in keeping the controlled responsive element around. The onChange and onResize callbacks open a lot of functionality for responsive components. It acts as an abstraction and wrapper for a consistent resize observer. The uncontrolled responsive element also has some valid use-cases, but most situations should be using the controlled component or could be converted into a controlled component to achieve the same functionality.

I don't believe the issues mentioned above can be resolved with the uncontrolled version of the component. If we consider this issue a real bug we may want to deprecate it. At a minimum if we choose to keep the uncontrolled responsive element we'd need to document the caveats to using it.

StephenEsser on 10 Oct 2019

👍1

Issue logged to deprecate uncontrolled responsive element #2719