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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 14 commits into from
Oct 2, 2023
Merged

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

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c88bbf7
Add name attribute for grouping details elements into an exclusive ac…
dbaron Jun 6, 2023
8639043
Address style/markup/typo review comments (many of which were suggest…
dbaron Jun 12, 2023
cba81a3
Convert both old and new open attribute change handling to be attribu…
dbaron Jun 12, 2023
51471c9
Add exclusive accordion example.
dbaron Jun 13, 2023
40c8996
Address the straightforward comments from the new round of review.
dbaron Jun 16, 2023
5134538
Address review comments from @annevk.
dbaron Jun 30, 2023
8d04f03
Add author conformance requirements.
dbaron Jul 28, 2023
9b76787
Enforce exclusivity in disconnected subtrees.
dbaron Aug 1, 2023
b7042fa
Merge branch 'add-details-name'
dbaron Sep 25, 2023
b945c9c
Strictly enforce exclusivity for <details name>.
dbaron Sep 25, 2023
20490af
Address review comments from @domenic.
dbaron Sep 28, 2023
0c64396
Update note explaining authoring requirement to reflect exclusivity e…
dbaron Sep 28, 2023
65a26ed
Add a should (with explanatory note) about keeping related elements t…
dbaron Sep 28, 2023
e3ede80
Address review comments from @domenic, including more suppression of …
dbaron Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 58 additions & 3 deletions source
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59454,6 +59454,7 @@ dictionary <dfn dictionary>FormDataEventInit</dfn> : <span>EventInit</span> {
<dd>One <code>summary</code> element followed by <span>flow content</span>.</dd>
<dt><span data-x="concept-element-attributes">Content attributes</span>:</dt>
<dd><span>Global attributes</span></dd>
<dd><code data-x="attr-details-name">name</code></dd>
<dd><code data-x="attr-details-open">open</code></dd>
<dt><span
data-x="concept-element-accessibility-considerations">Accessibility considerations</span>:</dt>
Expand All @@ -59465,6 +59466,7 @@ dictionary <dfn dictionary>FormDataEventInit</dfn> : <span>EventInit</span> {
interface <dfn interface>HTMLDetailsElement</dfn> : <span>HTMLElement</span> {
[<span>HTMLConstructor</span>] constructor();

[<span>CEReactions</span>] attribute boolean <span data-x="dom-details-name">name</span>;
dbaron marked this conversation as resolved.
Show resolved Hide resolved
[<span>CEReactions</span>] attribute boolean <span data-x="dom-details-open">open</span>;
};</code></pre>
</dd>
Expand All @@ -59485,6 +59487,11 @@ interface <dfn interface>HTMLDetailsElement</dfn> : <span>HTMLElement</span> {
<p>The rest of the element's contents <span>represents</span> the additional information or
controls.</p>

<p>The <dfn element-attr for="details"><code data-x="attr-details-name">name</code></dfn> content
attribute gives the name of the group of related <code>details</code> elements that the element is
a member of. Opening one member of this group causes other members of the group to close. If the
dbaron marked this conversation as resolved.
Show resolved Hide resolved
attribute is specified, its value must not be the empty string.</p>

<p>The <dfn element-attr for="details"><code data-x="attr-details-open">open</code></dfn> content
attribute is a <span>boolean attribute</span>. If present, it indicates that both the summary and
the additional information is to be shown to the user. If the attribute is absent, only the
Expand Down Expand Up @@ -59532,9 +59539,51 @@ interface <dfn interface>HTMLDetailsElement</dfn> : <span>HTMLElement</span> {
data-x="event-toggle">toggle</code> at the <code>details</code> element.</p></li>
</ol>

<p>The <dfn attribute for="HTMLDetailsElement"><code data-x="dom-details-open">open</code></dfn>
IDL attribute must <span>reflect</span> the <code data-x="attr-details-open">open</code> content
attribute.</p>
<p>The <dfn><i>details name group</i></dfn> that contains a <code>details</code> element
dbaron marked this conversation as resolved.
Show resolved Hide resolved
<var>a</var> also contains all the other <code>details</code> elements <var>b</var> that fulfill
all of the following conditions:</p>

dbaron marked this conversation as resolved.
Show resolved Hide resolved
<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

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}


<li>They both have a <code data-x="attr-details-name">name</code> attribute, their <code
data-x="attr-details-name">name</code> attributes are not empty, and the value of <var>a</var>'s
<code data-x="attr-details-name">name</code> attribute equals the value of <var>b</var>'s <code
data-x="attr-details-name">name</code> attribute.</li>

</ul>

<p>Whenever the <code data-x="attr-details-open">open</code> attribute is added to a
<code>details</code> element that has a <code data-x="attr-details-name">name</code> attribute
that is not empty, the user agent must run the following steps, which are known as the
<dfn>details group closing steps</dfn>, for this <code>details</code> element:</p>
dbaron marked this conversation as resolved.
Show resolved Hide resolved

<ol>
<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 •
edited
Loading

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.

Copy link

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?

Copy link

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


<p class="note">This is done because mutation events could be run during this algorithm, and the
mutation event listeners could change the members of the <i>details name group</i> or the
dbaron marked this conversation as resolved.
Show resolved Hide resolved
<span>tree order</span>.
</li>

<li>
<p>For each element <var>otherElement</var> in <var>group members</var>, in order:</p>
dbaron marked this conversation as resolved.
Show resolved Hide resolved
dbaron marked this conversation as resolved.
Show resolved Hide resolved
<ol>
dbaron marked this conversation as resolved.
Show resolved Hide resolved
<li><p>If the <code data-x="attr-details-open">open</code> attribute is set on
<var>otherElement</var>, remove the <code data-x="attr-details-open">open</code> attribute on
<var>otherElement</var>.
</ol>
</li>
</ol>

<p>The <dfn attribute for="HTMLDetailsElement"><code data-x="dom-details-name">name</code></dfn>
and <dfn attribute for="HTMLDetailsElement"><code data-x="dom-details-open">open</code></dfn>
IDL attributes must <span>reflect</span> the respective content attributes of the same name.</p>

</div>
dbaron marked this conversation as resolved.
Show resolved Hide resolved

dbaron marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -129304,6 +129353,7 @@ interface <dfn interface>External</dfn> {
<td><code>summary</code>*;
<span data-x="Flow content">flow</span></td>
<td><span data-x="global attributes">globals</span>;
<code data-x="attr-details-name">name</code>
domenic marked this conversation as resolved.
Show resolved Hide resolved
<code data-x="attr-details-open">open</code></td>
<td><code>HTMLDetailsElement</code></td>
</tr>
Expand Down Expand Up @@ -131575,6 +131625,11 @@ interface <dfn interface>External</dfn> {
<span data-x="attr-fe-name">form-associated custom elements</span>
<td> Name of the element to use for <span>form submission</span> and in the <code data-x="dom-form-elements">form.elements</code> API <!--or: Name of the element to use in the <code data-x="dom-form-elements">form.elements</code> API. -->
<td> <a href="#attribute-text">Text</a>*
<tr>
<th> <code data-x="">name</code>
<td> <code data-x="attr-details-name">details</code>
<td> Name of group of mutually-exclusive details elements
domenic marked this conversation as resolved.
Show resolved Hide resolved
<td> <a href="#attribute-text">Text</a>*
<tr>
<th> <code data-x="">name</code>
<td> <code data-x="attr-form-name">form</code>
Expand Down