This might be a dupe, I remember seeing a bug about this but can't find it right now. Maybe we fixed it but not completely?

Test case

https://codepen.io/fvsch/pen/yLBMEwe

<section>
  <div></div>
  <div></div>
  <div></div>
</section>

<style>
section {
  display: flex;
  justify-content: center;
  margin: 2rem;
  border: 4px solid;
  padding: 1rem;
  gap: 1rem;
}

section > div {
  display: flex;
  width: 5rem;
  height: 5rem;
  background-color: blue;
}
</style>

Steps to reproduce

1. Open test case and inspect one of the blue squares
2. In Layout, 4 panes are showing. Close every pane except the last one, "Box Model".
3. Select the parent node, <section>.

Expected result:
Layout now shows 3 panes:

• "Flex Container", closed
• "Grid", closed
• "Box Model", open

Actual result:
Layout now shows 3 panes:

• "Flex Container", closed
• "Grid", closed
• "Box Model", closed

The box model pane should be opened, not closed.

What's going on?

We remember the open/closed status of Layout accordion panes as an array of booleans, instead of remembering the state of each pane by name. This matters because we don't always show the same panes at the same index:

• Index 0: "Flexbox" or "Flex Container"
• Index 1: "Flex Item of …" or "Grid"
• Index 2: "Grid" or "Box Model"
• Index 3: "Box Model" or undefined

Looking at preferences in about:config, I'm seeing individual booleans for:

devtools.layout.boxmodel.opened	
devtools.layout.flexbox.opened	
devtools.layout.grid.opened

(Are we missing one for flexbox, since we have 2 flexbox-related panes? Or does it not matter much that we remember each separately?)

This seems encouraging, but we probably mess it up in devtools/client/inspector/layout/components/Accordion.js when we store a local state of "are accordion panes opened or not" as an array of booleans here:

this.state = {
  opened: props.items.map(item => item.opened),
};

We probably need to keep track of opened accordion content by name, and not by index.

After exploring the code a bit, we have at least 5 Accordion implementations:

• devtools/client/shared/components/Accordion.js: used by the Accessibility panel, and recently for Websockets in Network
• devtools/client/debugger/src/components/shared/Accordion.js: the one used in Debugger, and perhaps not shared with anyone since it uses JSX and Flow and I don't think other panels have the tools to make use of that (not seeing any import outside of Debugger).
• devtools/client/inspector/layout/components/Accordion.js: this one, which has a comment saying it's copied from debugger.html and any change should be upstreamed, but there have been substantial changes in both components so that comment is fiction at this point.
• Plus I guess the other accordion in Inspector (at least one, maybe 2?)
• Plus the strange table-based accordions in Network

Coming back to this one:

• We should probably remove the comment about upstreaming changes to Debugger
• I would LOVE to use devtools/client/shared/components/Accordion.js instead, which seems like the right "upstream" to target.

I would LOVE to use devtools/client/shared/components/Accordion.js instead, which seems like the right "upstream" to target.

That's bug 1525939.

Looks like devtools/client/inspector/layout/components/Accordion.js is used all over Inspector: in Rules, Layout, Fonts, etc.

Ultimately I would love to replace this component with the shared accessible one :yzen made for the Accessibility panel. The main difference seems to be that the Inspector's accordion keeps track of whether an accordion item's content was ever rendered (aka "created") and keeps it rendered in the DOM, hiding it with CSS.

Schematically, a closed accordion item will look like this in Inspector:

<div>
  <h2>I'm closed</h2>
  <div class="closed">Lots of content here</div>
</div>

and it would look like this in Debugger or Accessibility:

<div>
  <h2>I'm closed</h2>
</div>

Julian, do you know if Inspector needs this as a specific optimization?
I chatted with Jason briefly and he kinda remembers having that in Debugger and simplifying it away because it didn't change much (and perhaps caused bugs like this one).

Julian, do you know if Inspector needs this as a specific optimization?

Not that I know of. It was initially added in https://bugzilla.mozilla.org/show_bug.cgi?id=1308227 as a copy of the debugger one. As you said I think we just didn't synchronize the two files, so when the Debugger team changed the implementation, the inspector one was not updated.

There's always a risk that tests might fail, in case they rely on this markup being available even when hidden. But I would say, just try and change it, see what breaks :)

I am also forwarding the needinfo to Gabriel in case he has more info (I haven't touched the inspector in a while).

(In reply to Julian Descottes [:jdescottes] from comment #4)

Julian, do you know if Inspector needs this as a specific optimization?

Not that I know of. It was initially added in https://bugzilla.mozilla.org/show_bug.cgi?id=1308227 as a copy of the debugger one. As you said I think we just didn't synchronize the two files, so when the Debugger team changed the implementation, the inspector one was not updated.

There's always a risk that tests might fail, in case they rely on this markup being available even when hidden. But I would say, just try and change it, see what breaks :)

I am also forwarding the needinfo to Gabriel in case he has more info (I haven't touched the inspector in a while).

I replied to this in phabricator, but I don't think there are any risks here.

Comment on attachment 9088480 [details]
Bug 1576604 - Use shared Accordion component in Inspector; r=gl,yzen

Revision D43640 was moved to bug 1525939. Setting attachment 9088480 [details] to obsolete.

Let's make a smaller patch to devtools/client/inspector/layout/components/Accordion.js that can be landed in (or uplifted to) Firefox 70.

I'm also seeing glitches where the accordion headers appear late (after their content) when switching between tabs. Turns out that:

1. An ancestor of Accordion has visibility: hidden|visible set in JS.
2. visibility is an inherited and animatable property.
3. Accordion headers have transition: all 0.25s ease, so they animated visibility (keep it hidden for 0.25s, and visible at the end). Perf pressure may mean that the header remains hidden more than 250ms, not sure.

Let's remove the transition (only used for the background-color anyway), which is inconsistent with accordions in Debugger, Rules, etc.

https://hg.mozilla.org/mozilla-central/rev/994237f8b729

https://hg.mozilla.org/releases/mozilla-beta/rev/994237f8b729

Confirmed issue with 70.0a1 (2019-08-24) on Windows 10.
Fix verified with 71.0a1 (2019-09-26) on Windows 10 and 70.0b9 on Windows 10 ,macOS 10.13.6, Ubuntu 16.04.