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

Add name attribute for grouping details elements into an exclusive accordion #9400

Merged
merged 14 commits into from
Oct 2, 2023

Conversation

dbaron
Copy link
Member

@dbaron dbaron commented Jun 7, 2023

This PR adds a name attribute to the details element. This has been discussed a bit in the Open UI Community Group and I've written an explainer for it.

This work began as part of the followup work after pausing work on CSS Toggles. For some recent discussions in the Open UI Comunity Group, see May 25 minutes and June 1 minutes for recent discussions.

(See WHATWG Working Mode: Changes for more details.)


/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/indices.html ( diff )
/interactive-elements.html ( diff )

source Outdated
<li>
<p>Let <var>group members</var> be a list of elements, containing all elements in this
<code>details</code> element's <i>details name group</i> except for this <code>details</code>
element, in <span>tree order</span>.</p>
Copy link
Member Author

@dbaron dbaron Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to call out this "in tree order" as a substantive open issue.

For a start, it doesn't match the behavior that I've described in the explainer, implemented in Chromium, and tested for in the WPT test. That behavior is insertion order rather than tree order. However, as discussed in #9390, it seems that specifying insertion order is a substantial amount of work (and would require patching both DOM and HTML so that the removal steps can be invoked with the necessary information to connect all removed elements to their old root prior to the removal).

However, there was a reason for choosing insertion order. In particular, my motivation was based on the following points:

  • I initially thought that the only way this ordering is web exposed is through mutation events, which are deprecated, and possibly on a path to removal (see the removal plan). However, I realize that it's also web-exposed through the ordering of toggle events, although I haven't yet written a test for that (but probably should [Edit: tests added in Test order of toggle events in addition to order of DOMSubtreeModified events. web-platform-tests/wpt#40429]). Given my initial (incorrect) understanding, I thought it was relatively unimportant for the ordering behavior to be good, and probably only important that it be defined and interoperable. (But it might even be ok for it to be undefined.) However, even with the exposure through the ordering of toggle events, I'm not sure the behavior is particularly important.
  • Doing notification in tree order is more expensive. It requires one of the following, either of which has a cost:
    • Maintaining the elements in the set in tree order (when adding/removing from the set). The performance of doing this is particularly problematic because it adds a cost to HTML parsing or DOM manipulation of any HTML that uses this feature. Chromium does have code (TreeOrderedList) that would be convenient for this, but I think it's the worst possible choice.
    • Ensuring that they're sorted at the time of notification. The performance of this option is much less problematic because only a single sort would need to be done during a UI interaction, and the cost of that single sort is likely acceptable for all reasonable uses of this feature (despite being nonzero). Chromium sort of has code (TreeOrderedMap) that I think could be extended to handle this, but that extension process would be quite complex because the existing code is specialized for handling two cases (image maps and slots) and extending it is rather complicated.
  • Maintaining the set in insertion order into either the Document or the ShadowRoot (as described in the explainer, as implemented, and as tested) seemed like the easiest option since it is well defined, can be implemented efficiently, and the web exposure of the behavior is minimal.

However, there doesn't appear to be an existing pattern of specifications defining use of insertion order for sets of elements. It's common for sets of observers/listeners or similar.

So I'm curious what folks think of the tradeoff here. I'm aware of four options:

  • tree order: less efficient (worse for implementers and authors), but a common existing pattern
  • insertion order, specified formally: probably a better (more efficient) but still well-defined behavior; not an existing pattern, and requires substantially more specification work.
  • insertion order, specified in a handwavy way: similarly efficient, perhaps good enough, and doesn't require a bunch of specification work
  • undefined behavior: perhaps acceptable in this case given how minimal the web exposure is, but also probably an unusual choice, but easy to specify.

The current PR specifies tree order, whereas the explainer, implementation, and tests do insertion order. So something definitely needs to change here, but I didn't want to do all the spec work for the second option above without getting some feedback first.

I'm curious what folks think of the choices here... or whether you see flaws in the above analysis.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative for the insertion order edits would be using the hook proposed in whatwg/dom#1185.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case when details is connect to document needs to be defined. Dealing only with open attribute setting isn't enough to handle document parsing. The first open details element might not be the first details in the group.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're asking about defining enforcement of exclusivity during document parsing -- then it was an intentional design decision not to enforce the exclusivity during document parsing, on the grounds that it would introduce too much complexity and probably breaking of invariants. See this section of the explainer.

If that's not what you're asking about -- could you explain further?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that any different to radio groups? Parsing use case should be supported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's different from radio groups in that the open/closed state is stored in an attribute, and I believe changing attributes during parsing or during dom insertion is problematic (at least partly because of mutation events, although I think there may be other reasons).

(Also, I really still am interested in feedback on the "in tree order" issue that started this thread.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that we should aim for either tree order or formally-specified insertion order. I'm confident #9390 is solvable, and indeed, it seems like it would be good to solve the bugs you uncovered there regardless.

From a theoretical purity perspective, I generally prefer tree order. As you say, it's what the rest of the spec ecosystem does.

I'm curious to get a sense how bad, exactly, the inefficiency of using tree order is. How many nodes would you need to sort at the time of interaction? Do you have a rough idea of how slow such sorting would be, e.g. on a low-end mobile device?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the actual performance cost of doing tree order at time of user interaction probably isn't that bad -- it's a function of the number of details elements involved and their depth in the dom tree, and the former seems unlikely to be particluarly large. I suspect it's O(count * log(count) * depth) but I haven't checked this carefully.

I'll have to figure out of there's a reasonable existing way to reuse existing Chromium code to do this...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In https://chromium-review.googlesource.com/c/chromium/src/+/4617028 I've changed the Chromium implementation and the tests to do this in tree order. This does match other aspects of the platform.

The way Chromium tends to do things that require "in tree order" is simply to traverse the entire tree, or in some cases traverse some known-relevant subtree. In some cases there's caching of the result of that traversal, with varying cleverness. In this case I didn't bother with that; it's now just a full traversal of the tree of the document or shadow root. I think that's likely to have acceptable speed -- and it did substantially reduce the amount of code involved. (I removed most of the code that I wrote for this feature in the first place!)

So I think this is probably settled now, but I wanted to leave this thread open for a bit in case others had comments on that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw I also think tree order was the way to go here

@dbaron dbaron marked this pull request as ready for review June 7, 2023 14:36
@dbaron
Copy link
Member Author

dbaron commented Jun 7, 2023

I'm sure this has some major gaps because I'm new to doing substantive edits like this to HTML, but I've marked this as ready for review because I'm interested in feedback at this point.

Also cc:ing @emilio and @bkardell as folks who may be interested generally or in the particular open issue that I've raised above.

source Outdated
<ul>

<li>Both <var>a</var> and <var>b</var> are in the same <span>tree</span>, and the
<span>root</span> of that tree is a <code>Document</code> or a <code>ShadowRoot</code>.</li>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it matter whether ShadowRoot's host is connected or not? In other words, can there be details name groups which aren't connected to a document?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have an opinion on this. I'd be happy to add such a requirement, although I don't currently have such a requirement in the spec and I don't think I have such a requirement in the implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this requirement? Compared to e.g. radio buttons which just require being in the same tree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think disconnected radio groups work so great in practice, they generally complicate the implementation quite a bit (because any node can suddenly be the "owner" of the radio group).

Unless there's a strong use case for this to work, maybe keeping it to DocumentOrShadowRoot is nicer?

See bug 1685926 for a bug related to them not working properly in Gecko that I was aware of. Pretty sure other similar bugs exist :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we were consistent, but mostly from a theoretical purity perspective. You could imagine web developers wanting to get the exclusivity behavior while they prepare a tree pre-insertion, but I'm not sure if that's realistic.

If there are implementer concerns, I'm happy to keep it to DocumentOrShadowRoot.

This is an area that would benefit from extensive WPTs though. To list the cases that I can think of:

  • Connected
  • Connected, but the root is an {XHR responseDocument, document.implementation.createDocument(), template contents owner document}
  • Disconnected, root is {a div, a ShadowRoot, a template element}

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2023
…d events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2023
…d events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 8, 2023
…d events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1154716}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2023
…d events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1154716}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2023
…d events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1154716}
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<ul>

<li>Both <var>a</var> and <var>b</var> are in the same <span>tree</span>, and the
<span>root</span> of that tree is a <code>Document</code> or a <code>ShadowRoot</code>.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this requirement? Compared to e.g. radio buttons which just require being in the same tree.

Copy link
Member Author

@dbaron dbaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've resolved a lot of the editorial comments as well as some of the more interesting ones.

source Outdated Show resolved Hide resolved
source Outdated
<li>
<p>Let <var>group members</var> be a list of elements, containing all elements in this
<code>details</code> element's <i>details name group</i> except for this <code>details</code>
element, in <span>tree order</span>.</p>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the actual performance cost of doing tree order at time of user interaction probably isn't that bad -- it's a function of the number of details elements involved and their depth in the dom tree, and the former seems unlikely to be particluarly large. I suspect it's O(count * log(count) * depth) but I haven't checked this carefully.

I'll have to figure out of there's a reasonable existing way to reuse existing Chromium code to do this...

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 13, 2023
…to order of DOMSubtreeModified events., a=testonly

Automatic update from web-platform-tests
Test order of toggle events in addition to order of DOMSubtreeModified events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1154716}

--

wpt-commits: a61c7604859be25ba8fb24ac863ea15d51385e4f
wpt-pr: 40429
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Jun 14, 2023
…to order of DOMSubtreeModified events., a=testonly

Automatic update from web-platform-tests
Test order of toggle events in addition to order of DOMSubtreeModified events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1154716}

--

wpt-commits: a61c7604859be25ba8fb24ac863ea15d51385e4f
wpt-pr: 40429
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jun 16, 2023
…to order of DOMSubtreeModified events., a=testonly

Automatic update from web-platform-tests
Test order of toggle events in addition to order of DOMSubtreeModified events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1154716}

--

wpt-commits: a61c7604859be25ba8fb24ac863ea15d51385e4f
wpt-pr: 40429

UltraBlame original commit: c64a32b114f7e14f7ce9cfb33e513b7fc870c5e5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jun 16, 2023
…to order of DOMSubtreeModified events., a=testonly

Automatic update from web-platform-tests
Test order of toggle events in addition to order of DOMSubtreeModified events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1154716}

--

wpt-commits: a61c7604859be25ba8fb24ac863ea15d51385e4f
wpt-pr: 40429

UltraBlame original commit: c64a32b114f7e14f7ce9cfb33e513b7fc870c5e5
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jun 16, 2023
…to order of DOMSubtreeModified events., a=testonly

Automatic update from web-platform-tests
Test order of toggle events in addition to order of DOMSubtreeModified events.

I realized while writing
whatwg/html#9400 (comment) that the
ordering of the `open` attribute manipulation is also exposed through
`toggle` events, so this tests those events in addition to
`DOMSubtreeModified` events.

Bug: 1444057
Change-Id: I6d3c65f5402053d77e4f6c488aa07209181a8cdd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4599204
Reviewed-by: Mason Freed <masonfchromium.org>
Commit-Queue: David Baron <dbaronchromium.org>
Cr-Commit-Position: refs/heads/main{#1154716}

--

wpt-commits: a61c7604859be25ba8fb24ac863ea15d51385e4f
wpt-pr: 40429

UltraBlame original commit: c64a32b114f7e14f7ce9cfb33e513b7fc870c5e5
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good!

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<ul>

<li>Both <var>a</var> and <var>b</var> are in the same <span>tree</span>, and the
<span>root</span> of that tree is a <code>Document</code> or a <code>ShadowRoot</code>.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we were consistent, but mostly from a theoretical purity perspective. You could imagine web developers wanting to get the exclusivity behavior while they prepare a tree pre-insertion, but I'm not sure if that's realistic.

If there are implementer concerns, I'm happy to keep it to DocumentOrShadowRoot.

This is an area that would benefit from extensive WPTs though. To list the cases that I can think of:

  • Connected
  • Connected, but the root is an {XHR responseDocument, document.implementation.createDocument(), template contents owner document}
  • Disconnected, root is {a div, a ShadowRoot, a template element}

Copy link
Member Author

@dbaron dbaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments. Still a few comment threads left open, but most of the simple ones are resolved. One of the older and more interesting ones is also probably resolved, although I won't "Resolve conversation" on it quite yet.

source Outdated Show resolved Hide resolved
source Outdated
<li>
<p>Let <var>group members</var> be a list of elements, containing all elements in this
<code>details</code> element's <i>details name group</i> except for this <code>details</code>
element, in <span>tree order</span>.</p>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In https://chromium-review.googlesource.com/c/chromium/src/+/4617028 I've changed the Chromium implementation and the tests to do this in tree order. This does match other aspects of the platform.

The way Chromium tends to do things that require "in tree order" is simply to traverse the entire tree, or in some cases traverse some known-relevant subtree. In some cases there's caching of the result of that traversal, with varying cleverness. In this case I didn't bother with that; it's now just a full traversal of the tree of the document or shadow root. I think that's likely to have acceptable speed -- and it did substantially reduce the amount of code involved. (I removed most of the code that I wrote for this feature in the first place!)

So I think this is probably settled now, but I wanted to leave this thread open for a bit in case others had comments on that.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as an editor.

I'd still love a bit more discussion about what roots are allowed, in https://github.com/whatwg/html/pull/9400/files#r1222094129 . (Ideally from web developers, who could tell us if they expect to be able to manipulate details in an exclusive manner while disconnected?) But the current state looks reasonable, as long as it's tested.

aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 29, 2023
I realized this test was missing due to
whatwg/html#9400 (comment)

Bug: 1444057
Change-Id: I18d5b553d2ab20543b37e190c1768703b43ac843
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903590
Auto-Submit: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1203408}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 29, 2023
I realized this test was missing due to
whatwg/html#9400 (comment)

Bug: 1444057
Change-Id: I18d5b553d2ab20543b37e190c1768703b43ac843
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903590
Auto-Submit: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1203408}
@GrahamTheDevRel
Copy link

GrahamTheDevRel commented Sep 30, 2023

Hey, I am very late to this discussion (and my first time entering a discussion like this so if there are better ways to ask, please let me know.).

Also apologies if this has been answered and I missed it, but how will the grouping be exposed to assistive tech?

At the moment the pattern most prefer for an exclusive accordion is:

<section role="group" aria-labelledby="faqs-heading">
  <h2 id="faqs-heading">FAQs (for example)</h2>
  <details>
    <summary>Accordion 1</summary>
  </details>
  <details>
    <summary>Accordion 1</summary>
  </details>
</section>

However if there is a name attribute added to make an exclusive accordion, will this be exposed as a group of items in some way in the accessibility tree / to assistive tech (AT)? As if that is the case that could lead to a lot of duplication of "group" announcements (compounded by <summary> itself being announced as a group like "Summary, group" in some screen reader / browser combos.).

If it is announced as a group, how would you label such a group?

If it isn't announced as a group, is there some way a screen reader user (for example) will know other elements have been collapsed when they open another? (minor point as this is an existing problem, which is why most prefer the role="group" attribute to be added to at least show the controls are related), or is this down to the developer to follow the pattern I described earlier / how we currently do this?

I did check the updated HTML spec, but I am not sure if I missed this point.

Thanks in advance and love this addition by the way!

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect! I'll leave this unmerged just in case you want to go the more dramatic route of disabling mutation events entirely, but my instinct is to start with this more conservative approach. Also, please file a Gecko bug!

@dbaron
Copy link
Member Author

dbaron commented Oct 2, 2023

@GrahamTheDevRel We've had some previous discussion of this. In this issue, see most recently #9400 (comment) (and the next comment), #9400 (comment) (and the next comment), and #9400 (comment). Definitely let me know what you think about that discussion -- it's not fully resolved yet and will probably need to continue in some other places (likely mostly outside this repository).

@dbaron
Copy link
Member Author

dbaron commented Oct 2, 2023

Looks perfect! I'll leave this unmerged just in case you want to go the more dramatic route of disabling mutation events entirely, but my instinct is to start with this more conservative approach.

I think I prefer the current approach as described in #9400 (comment) .

Also, please file a Gecko bug!

Filed bug 1856460.

@domenic domenic merged commit e0cf318 into whatwg:main Oct 2, 2023
1 check passed
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 4, 2023
…rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 4, 2023
…ame> elements into a group., a=testonly

Automatic update from web-platform-tests
Add test for insertion of new <details name> elements into a group.

I realized this test was missing due to
whatwg/html#9400 (comment)

Bug: 1444057
Change-Id: I18d5b553d2ab20543b37e190c1768703b43ac843
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903590
Auto-Submit: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1203408}

--

wpt-commits: 59cd10c8c3296f14addd02e7ad2c35da7b6afb1a
wpt-pr: 42249
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 4, 2023
…rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1205367}
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 4, 2023
…rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1205367}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 4, 2023
…rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1205367}
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 5, 2023
…ame> elements into a group., a=testonly

Automatic update from web-platform-tests
Add test for insertion of new <details name> elements into a group.

I realized this test was missing due to
whatwg/html#9400 (comment)

Bug: 1444057
Change-Id: I18d5b553d2ab20543b37e190c1768703b43ac843
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903590
Auto-Submit: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1203408}

--

wpt-commits: 59cd10c8c3296f14addd02e7ad2c35da7b6afb1a
wpt-pr: 42249
@aardrian
Copy link

@dbaron I have been on the road for the last couple weeks and will be for the next few weeks, so my responses will be slow.

@aardrian In 65a26ed I think I've addressed a part of your concern, although probably not all of it.

It did not (but I appreciate it is in there on this first pass) since it provides no rules nor guidance on accName for the group, does not give insight for how that information should be passed off to AAPIs, and does not address cases where authors stuff non-details/summary content into the same container.

I think some of what you were suggesting may belong in html-aam rather than html: in particular, if we're going to give advice about inferring an accessible name for a section from its header for this particular case. (At least, unless that's already an existing rule for a more general case?)

HTML-AAM maps things coming from HTML, so the formatting rules still need to be sorted here. I think.

I think it's possible HTML could say a little bit more, but I do want to (a) avoid giving advice that I wouldn't stand by 100% or very nearly 100% of the time and (b) avoid giving advice that's prematurely assuming what html-aam is going to say.

That's fair.

Does that seem reasonable? Is there other particular advice you think HTML could give now? And do you think it makes sense to open an html-aam issue on the topic?

I would have continued this here, but since I have been traveling (and contributing here is solely from my free time) I understand that this moved ahead and this issue is closed. Since you opened the HTML-AAM issue, though, Scott asked some questions that (IMO) need to be sorted here but we might as well discuss there (I took a first stab at it) versus cluttering this closed issue.

That is not me being passive aggressive or snarky. No idea how it comes off, though. The jet lag is not helping.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 26, 2023
…etails name> to revised spec rules., a=testonly

Automatic update from web-platform-tests
Update mutation event suppression for <details name> to revised spec rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1205367}

--

wpt-commits: a4288b9d453a8c48078b3e93a58c86cf97ac3551
wpt-pr: 42335
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this pull request Oct 27, 2023
…etails name> to revised spec rules., a=testonly

Automatic update from web-platform-tests
Update mutation event suppression for <details name> to revised spec rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1205367}

--

wpt-commits: a4288b9d453a8c48078b3e93a58c86cf97ac3551
wpt-pr: 42335
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 30, 2023
…etails name> to revised spec rules., a=testonly

Automatic update from web-platform-tests
Update mutation event suppression for <details name> to revised spec rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <dbaronchromium.org>
Reviewed-by: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1205367}

--

wpt-commits: a4288b9d453a8c48078b3e93a58c86cf97ac3551
wpt-pr: 42335

UltraBlame original commit: 79aff7d6f2407a4822f16b41afa8c0eab82537b2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 30, 2023
…etails name> to revised spec rules., a=testonly

Automatic update from web-platform-tests
Update mutation event suppression for <details name> to revised spec rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <dbaronchromium.org>
Reviewed-by: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1205367}

--

wpt-commits: a4288b9d453a8c48078b3e93a58c86cf97ac3551
wpt-pr: 42335

UltraBlame original commit: 79aff7d6f2407a4822f16b41afa8c0eab82537b2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 30, 2023
…etails name> to revised spec rules., a=testonly

Automatic update from web-platform-tests
Update mutation event suppression for <details name> to revised spec rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <dbaronchromium.org>
Reviewed-by: Joey Arhar <jarharchromium.org>
Cr-Commit-Position: refs/heads/main{#1205367}

--

wpt-commits: a4288b9d453a8c48078b3e93a58c86cf97ac3551
wpt-pr: 42335

UltraBlame original commit: 79aff7d6f2407a4822f16b41afa8c0eab82537b2
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
See discussions in:
whatwg/html#9400 (comment)
openui/open-ui#778 (comment)

Bug: 1444057
Change-Id: I901e4e3958cdf55a07cb9e5126ed235a07819228
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4734191
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1177898}
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
I realized this test was missing due to
whatwg/html#9400 (comment)

Bug: 1444057
Change-Id: I18d5b553d2ab20543b37e190c1768703b43ac843
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903590
Auto-Submit: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1203408}
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this pull request Dec 11, 2023
…rules.

The rules for mutation event suppression for <details name> were revised
during the process of reviewing the spec PR, based on the discussion
starting at
whatwg/html#9400 (comment) .  The
updated spec says that mutation events are suppressed during the "ensure
details exclusivity by closing other elements if needed" and "ensure
details exclusivity by closing the given element if needed" algorithms.
This updates the implementation and tests to follow that rule.  (The
"handling of insertion of elements into group" test is testing the case
where the events were already suppressed.)

This also renames the test to remove "tentative" from the name, since
the spec PR is landed and the test is now (with this change) up-to-date
with the spec.

Bug: 1444057
Change-Id: I9078beeb3527f2515f6e10efbf93a94232221238
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4912273
Commit-Queue: David Baron <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1205367}
@vaiil
Copy link

vaiil commented Dec 21, 2023

What about animation for exclusive accordion? It's impossible to animate closing state, because open attribute is removed immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

Successfully merging this pull request may close these issues.