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.

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

<details> open attribute not synchronised #15486

Open
afenton90 opened this issue Apr 24, 2019 · 28 comments
Open

<details> open attribute not synchronised #15486

afenton90 opened this issue Apr 24, 2019 · 28 comments

Comments

@afenton90
Copy link

What is the current behavior?
open attribute does not synchronise with the virtual dom on the details element, when using the details element as a controlled component.

I have a codesandbox here that demonstrates the current behaviour.

What is the expected behavior?
The open attribute stays synchronised with the virtual dom and is added/removed.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React: 16.8.6
Browser: Chrome
Did this work in previous versions of React? No

@baruchvlz
Copy link

It looks like it does synchronizes with the virtual dom.

image

Also, you don't need to do open={open} for the details element. HTML understands that clicking on the <summary> toggles the details. You can keep the onClick={onToggle} too. Check it here

@afenton90
Copy link
Author

afenton90 commented Apr 24, 2019

Thanks @baruchvlz. So is the official advice that details cannot be used as a controlled component?

@alejandronanez
Copy link

I just ran into the same issue today, and I fixed it by adding an event.preventDefault to the onClick handler. It is not crystal clear to me why this works tbh.

const onToggle = event => {
  event.preventDefault();
  setOpen(!open);
};

// Later on...
return (
  <div className="App">
    <details open={open} onClick={onToggle}>...</details>
  </div>
)

Codesandbox

@afenton90
Copy link
Author

Awesome @alejandronanez I'll go with that for now.
I'll let the maintainers decide whether this issue is a bug or just a quirk of the details element.

@gzqby
Copy link

gzqby commented May 5, 2019

@alejandronanez is right!details element have Default behavior! So,it is not controlled by onclickEvent

@julienw
Copy link

julienw commented Aug 27, 2019

Also the <details> element supports a toggle event (onToggle with React) that could make it possible to keep the native implementation while still updating the local state.

@stale
Copy link

stale bot commented Jan 27, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 27, 2020
@stale
Copy link

stale bot commented Feb 3, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Feb 3, 2020
@julienw
Copy link

julienw commented Feb 5, 2020

This "stale" bot thing is painful. It shouldn't close bugs without someone checks it doesn't happen anymore...

@gaearon gaearon reopened this Apr 1, 2020
@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 1, 2020
@wesleyboar-fka-iosulfur
Copy link

I do not have a solution for onClick, but I believe I have noticed a common discrepancy, in this thread, among attempts to mimic <details> behavior with React, and actual <details> behavior.

Summary
The live code samples provided thus far set onClick on the <details> element. This does not accurately represent the standard behavior of a <details> tag.

Explanation
In a raw <details>/<summary>, if one clicks on the content within an open <details>, underneath the <summary>, the <details> element does not close—only a click (or other activating user action) on the <summary> element will close (and open) the <details>. But, in the live code samples provided thus far, clicking on the described region closes the <details> element.

Example
Without React:

Summary

Content (clicking here has no effect)

With React:
https://codesandbox.io/s/react-details-bug-ov9sl

Reference

  • The <details> element content is shown on "click" of the <summary> tag.
  • The toggle event (not click event) is the <details> element event for changing the open state.

Browser Support
Chrome and Edge only recently have support

@bitworking
Copy link

To mimic the default behavior I did the following:

<details open={open} onClick={onToggle}>
  <summary>{title}</summary>
  <div role="region" onClick={(e) => e.stopPropagation()}>
    {children}
  </div>
</details>

@butchler
Copy link

butchler commented Jul 4, 2021

The title of this issue is inaccurate: React does keep the open attribute in sync with the DOM (as mentioned above #15486 (comment)). However, this is still a confusing case even though React isn't technically doing anything wrong.

It's confusing because, unlike most other native DOM elements, the <details> element has state which it can update by itself without React knowing about it. This is what happens in the codesandbox from the issue description (https://codesandbox.io/s/xl756mk82w) when you click on the element for the first time:

  1. User clicks on <details> element
  2. The onClick handler is triggered, and the React state is toggled from false to true
  3. React (when not in concurrent mode) immediately re-renders, setting the open attribute of the <details> element to true
  4. The default behavior of the <details> element kicks in, toggling the open state, reversing the open attribute back to false (but React doesn't know about this)

This is why the element does not open the first time you click. Because this happens instantly, it's not obvious that the state is actually being toggled twice. When you click on the element again, this happens:

  1. User clicks on <details> element
  2. The onClick handler is triggered, and React still thinks the element is open, so the React state is toggled from true to false
  3. React re-renders, but the open attribute is already false (because the native behavior reversed what React did in the first click), so React does not update the DOM
  4. The default behavior kicks in, and the element is toggled

This is why you can toggle the element after the first click, even though React and the DOM disagree about the current state. Every time it re-renders, React sees that the DOM is already in the state that it wants to be in, so it does not update the DOM and behaves as if the <details> element was an uncontrolled component (even though it is technically being controlled).

As mentioned in #15486 (comment), the solution is to call event.preventDefault() in the onClick handler to disable the browser's default behavior so that React and only React controls the state. Note that as #15486 (comment) mentions, to mimic native behavior the onClick handler should be on the <summary> element, not the <details> element.

#15486 (comment) mentioned that there is a toggle event which can be used to notify React about the current state. This might be useful when using the <details> element as a non-controlled component, but is a fragile way to use it as a controlled component because you still have the problem of the native behavior toggling the state separately from React. This codesandbox demonstrates how this could be an issue: https://codesandbox.io/s/react-details-bug-forked-iucdn. Clicking on the <details> element itself works correctly, but it causes an infinite re-rendering loop when you click on the button that toggles the React state alone. This is because onToggle is triggered not only when the user interacts with the element, but also triggered when React itself modifies the open attribute. This means that every time React toggles open, onToggle is triggered, making React toggle open, making onToggle get triggered again, etc.

tl;dr This is a confusing case, but is probably not an issue with React. The solution is to call event.preventDefault() in the onClick handler of the <summary> element.

@campbellgoe
Copy link

Finally, as the above comments found a solution: to use event.preventDefault() inside an onClick on the summary element, here is the code.
You must also specify the open attribute on the details element, and have no open attribute not even open={false} and therefore use {...(open ? { open: true } : {})}.

import { useState } from "react";

export default function Details({ summary, children, startOpen = false }){
  const [open, setOpen] = useState(startOpen);
  return (
    <details {...(open ? { open: true } : {})}>
      <summary
        onClick={(e) => {
          e.preventDefault();
          setOpen((open) => !open);
        }}
      >
        {typeof summary === "function" ? summary(open) : summary}
      </summary>
      {open && children}
    </details>
  );
};

Usage:

function App() {
  return (
    <div className="App">
      <Details summary={(open) => `${open ? "View" : "Hide"} contents`}>
        <p>The summary prop can be a function which gets the open value</p>
      </Details>
      <hr/>
      <Details summary="Details" startOpen={true}>
        <p>
          These contents are added/removed from the DOM depending on whether
          details is open or not.
        </p>
      </Details>
      <hr/>
    </div>
  );
}

See working / edit the code: https://codesandbox.io/s/beautiful-borg-nbh93?file=/src/App.js

@mrmiguu
Copy link

mrmiguu commented Oct 11, 2021

@campbellgoe

You must also specify the open attribute on the details element, and have no open attribute not even open={false} and therefore use {...(open ? { open: true } : {})}.

Hmm... this wasn't an issue for me in Chrome. Which browser did you notice this behavior?

@iampava
Copy link

iampava commented Jun 21, 2022

I just stumbled a somewhat similar bug, that I think is related to this.

I have a <details> element that is open by default, and the entire page flickers. Here's a CodeSandbox: https://codesandbox.io/s/throbbing-grass-1tphyl?file=/src/App.js

@butchler
Copy link

I have a <details> element that is open by default, and the entire page flickers. Here's a CodeSandbox: https://codesandbox.io/s/throbbing-grass-1tphyl?file=/src/App.js

The explanation was hidden in a large comment, but I mentioned this case above: #15486 (comment)

#15486 (comment) mentioned that there is a toggle event which can be used to notify React about the current state. This might be useful when using the <details> element as a non-controlled component, but is a fragile way to use it as a controlled component because you still have the problem of the native behavior toggling the state separately from React. This codesandbox demonstrates how this could be an issue: https://codesandbox.io/s/react-details-bug-forked-iucdn. Clicking on the <details> element itself works correctly, but it causes an infinite re-rendering loop when you click on the button that toggles the React state alone. This is because onToggle is triggered not only when the user interacts with the element, but also triggered when React itself modifies the open attribute. This means that every time React toggles open, onToggle is triggered, making React toggle open, making onToggle get triggered again, etc.

It seems like my sandbox did not include the button that I mentioned in the comment, but I updated it now to add the button.

In your sandbox, there is no button for toggling the React state alone, but because isOpen defaults to true, on mount React will set the details element's open property to true. This will trigger the onToggle event, which will call the event handler, which will flip the value of isOpen, which will again cause React to update the open property, and that repeats in an infinite loop.

@iampava
Copy link

iampava commented Jun 21, 2022

Oh, I see. So this is how it works 🤔

Thanks for the explanation. I'm gonna go with the solution proposed above, using a click event listener and .preventDefault().

SheepTester added a commit to SheepTester-forks/ExploratoryCurricularAnalytics that referenced this issue Aug 30, 2022
Was going to make <details> controlled but apparently you can't, facebook/react#15486
@boldium-eric-mikkelsen
Copy link

The idea that breaking an html element isn't a bug, is a problem. If a native element with w3c settled behavior is broken by React, React is broken, and this is a legit bug.

@butchler
Copy link

As far as I can tell nobody in this thread has suggested that this is not a problem. Maybe you were confused by my earlier comment where I said this?:

The title of this issue is inaccurate: React does keep the open attribute in sync with the DOM (as mentioned above #15486 (comment)).

I simply meant that the specific claim that the title made is technically inaccurate. I do think this is a very confusing thing that will waste a lot of people's time if it is not somehow fixed (or at least better documented).

In case you missed it, it is still possible to use the details element as long as you call event.preventDefault() in the event handler. Hopefully the React team can find a way to make it possible to use the details element without needing to do this, but for now you can use this as a workaround.

@jsejcksn
Copy link

jsejcksn commented May 6, 2023

It's possible to react to the native toggle event that's dispatched on the <details> element, using the nested open property to update state. This feels more idiomatic and avoids the previously mentioned workarounds related to the inner <summary> element. Here's a minimal, typed example:

TS Playground

import { type ReactElement, useState } from "react";

function Component(): ReactElement {
  const [open, setOpen] = useState(false);

  const handleToggle = (
    event: { currentTarget: { open: boolean } },
  ) => setOpen(event.currentTarget.open);

  return (
    <details onToggle={handleToggle} open={open}>
      <summary>{open ? "Hide" : "Show"}</summary>
      <p>More content</p>
    </details>
  );
}

@butchler
Copy link

butchler commented May 6, 2023

Using onToggle works for simple cases but may lead to an infinite re-rendering loop if the state is updated by anything other than the event handler. From my previous comment:

#15486 (comment) mentioned that there is a toggle event which can be used to notify React about the current state. This might be useful when using the <details> element as a non-controlled component, but is a fragile way to use it as a controlled component because you still have the problem of the native behavior toggling the state separately from React. This codesandbox demonstrates how this could be an issue: https://codesandbox.io/s/react-details-bug-forked-iucdn. Clicking on the <details> element itself works correctly, but it causes an infinite re-rendering loop when you click on the button that toggles the React state alone. This is because onToggle is triggered not only when the user interacts with the element, but also triggered when React itself modifies the open attribute. This means that every time React toggles open, onToggle is triggered, making React toggle open, making onToggle get triggered again, etc.

@jsejcksn
Copy link

jsejcksn commented May 6, 2023

Using onToggle works for simple cases but may lead to an infinite re-rendering loop if the state is updated by anything other than the event handler

@butchler That's only an issue when using a function that inverts the state value (like in your CodeSandbox example) — that's what causes the race condition and infinite re-render scenario. When setting it according to the value in the dispatched event (like in the code shown in my previous comment), you can be confident that won't happen.

If the new boolean value is opposite of the current value, it will re-render once. If it's the same, no new render will occur. From the React docs:

If the new value you provide is identical to the current state, as determined by an Object.is comparison, React will skip re-rendering the component and its children. This is an optimization. Although in some cases React may still need to call your component before skipping the children, it shouldn’t affect your code.

@butchler
Copy link

butchler commented May 6, 2023

Correct, and that's what I meant by "if the state is updated by anything other than the event handler". Sorry if my phrasing wasn't clear.

This means that for the case where you only need to know if the detail element is open or closed in your React component, using onToggle works fine. However, in some cases you might want to toggle the detail element via React state. For example, you might want to have a "expand all"/"collapse all" button that toggles all of the detail elements on a particular page. In that case, using onToggle will lead to an infinite render loop, and for most people it won't be obvious what's wrong or how to fix it.

For that reason, personally I wouldn't recommend onToggle as the preferred way to use detail elements. Ultimately it comes down to personal preference. In my case, I don't mind having to add the event.preventDefault() call if it means I never have to think about this potential edge case with onToggle.

@butchler
Copy link

butchler commented May 6, 2023

If you do use onToggle, I would recommend that you don't pass the open prop to the detail element. It gives the false impression that you can toggle the element with React state updates alone (i.e. not with the event handler) when in reality that will lead to an infinite render loop.

@jsejcksn
Copy link

jsejcksn commented May 6, 2023

If you do use onToggle, I would recommend that you don't pass the open prop to the detail element. It gives the false impression that you can toggle the element with React state updates alone (i.e. not with the event handler) when in reality that will lead to an infinite render loop.

@butchler You can have confidence that won't be an issue either — it'll still work!

Below I've included a reproducible example that you can use to verify. No build tools are needed — just save the contents as a local file and open in a browser. In case you don't have a text editor available, here's a link to a hosted version: https://jsfiddle.net/4w7tuLfk/

example.html
<!doctype html>
<html lang="en">

<head>
  <meta charset="utf-8" />
  <meta name="viewport" content="width=device-width, initial-scale=1" />
  <title>Details demo</title>
  <style>body { font-family: sans-serif; } button { margin: 1rem 0; } details { border: 1px solid blue; } summary { border: 1px solid red; }</style>
  <script src="https://cdn.jsdelivr.net/npm/react@18.2.0/umd/react.production.min.js"></script>
  <script src="https://cdn.jsdelivr.net/npm/react-dom@18.2.0/umd/react-dom.production.min.js"></script>
  <script src="https://cdn.jsdelivr.net/npm/@babel/standalone@7.21.8/babel.min.js"></script>
  <script type="text/babel" data-type="module" data-presets="env,react">
    // This example uses UMD modules
    const { useRef, useState } = React;

    function RenderCount() {
      const renderCountRef = useRef(0);
      renderCountRef.current += 1;
      return <div>Render count: {renderCountRef.current}</div>;
    }

    function App() {
      const [open, setOpen] = useState(false);

      return (
        <>
          <RenderCount />
          <button onClick={() => setOpen((open) => !open)}>
            Toggle using state
          </button>
          <details
            onToggle={(event) => setOpen(event.currentTarget.open)}
            open={open}
          >
            <summary>{open ? "Hide" : "Show"}</summary>
            <p>More content</p>
          </details>
        </>
      );
    }

    ReactDOM.createRoot(document.getElementById("root")).render(<App />);
  </script>
</head>

<body>
  <div id="root"></div>
</body>

</html>

@butchler
Copy link

butchler commented May 6, 2023

I apologize, I didn't notice the difference between your suggestion and the previous suggestions in this thread about using the toggle event. The key difference is to not directly toggle the React state, but to always set the React state to match the detail element's current .open state. That does indeed avoid the problem I mentioned. 👍

In hindsight it seems so obvious that I don't know why I never thought of it myself. 😅

@butchler
Copy link

butchler commented May 6, 2023

I noticed that the new React docs have pages documenting how to use common types of controlled elements such as input: https://react.dev/reference/react-dom/components/input.

If someone added a similar page describing usage of the detail element with React then maybe that would be enough to close this ticket?

@SheepTester
Copy link

Note that browsers can open <details> without firing click (but it does fire toggle, but you can't preventDefault it). If you search for content in the page, browsers can open <details> if its contents contain it . If you try to force it closed using e => (e.currentTarget.open = false), it still shows a flash of the contents being open.

Here's a demo: https://jsfiddle.net/SheepTester/57chzoen/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests