search

openui / open-ui

Maintain an open standard for UI and promote its adherence and adoption.
https://open-ui.org
Other
3.59k stars 191 forks source link

[exclusive accordion] exclusively non-exclusive... #786

Closed bkardell closed 1 year ago

bkardell commented 1 year ago

The exclusive accordions proposal allows authors to create native exclusive accordions via

...add an attribute to the details element. All details elements in the same tree that have the same value for this attribute would form an exclusive accordion. The syntax of this attribute and the rules for matching its values would match the name attribute and its use in defining radio button groups.

(In this definition of exclusive, it is possible to have all closed, but only one open at a time).

Importantly: With <details>, if an author sets the open attribute, it is open. If an author sets an element's .open = false the attribute is removed. If a user toggles it open or closed, this is reflected in the DOM attribute. This proposal would manage a group of them, so that any of those affected the others.

There's a challenge created by the fact that this is very _un_like radio buttons (or most other elements). The implication of that difference is that certain norms about what happens with initial or dynamic markup won't hold up. For example, given initial markup like this (code below, but if you have chromium and experimental features enabled you can run these pens)

<details open name="test">
  <summary>A</summary>
  One
</details>
<details name="test">
  <summary>B</summary>
  Two
</details>
<details name="test">
  <summary>C</summary>
  Three
</details>
<details open name="test">
  <summary>D</summary>
  Four
</details>

The current spec and implementation will begin with both details open (so... not exclusive). When you act, it will run the algorithm and eventually sort it right but it's pretty weird in a way that other controls aren't (because the attribute is not the live value).

This also means it doesn't apply to DOM manipulations - so., inserting a new details into the same group does not run the algorithm.. For example:


<details open name="not-test">
  <summary>X</summary>
  Added to the group 'test' below late by changing the name -- shouldn't it cause A to close?
</details>

<details open name="test">
  <summary>A</summary>
  One
</details>
<details name="test">
  <summary>B</summary>
  Two
</details>
<details name="test">
  <summary>C</summary>
  Three
</details>
<details name="test">
  <summary>D</summary>
  Four
</details>

<script>
  document.querySelector('[name="not-test"]').setAttribute("name", "test")
</script>

The above yields an accordion in which both 'X' and 'A' are in the open state, and thus, not exclusive.

There's currently a pull for all of this against HTML which explains this in this note:

This exclusivity is enforced by user agents only when a details element is opened, and is not enforced by user agents when parsing or when inserting elements into the DOM tree. This is why there are also requirements on authors to ensure that the exclusivity is correct.

I'm concerned that this feels confusing. It does to me as a developer, and I believe this was also called out in https://github.com/mozilla/standards-positions/issues/831#issuecomment-1616678969 . It's surprising to me that more reviews haven't commented on it, so I'm keen on at least doing some proactive polling or something on this

domenic commented 1 year ago

I think there are several cases here.

  1. Changing an existing-in-the-tree <details> element's name=""

  2. Inserting a new named <details> into the tree.

  3. Initial parsing.

(1) and (2) are reasonable to handle, IMO. Although they might have small performance implications.

(3) is trickier. I can't find any other cases where we would just totally ignore an attribute in the parser's stream of text. That is,

<details name="a" open="bogus text 1">1</details>

<details name="a" open="bogus text 2">2</details>

getting parsed as

<details name="a" open="bogus text 1">1</details>

<details name="a">2</details>

seems quite strange to me! (I've added bogus text to the attributes, instead of leaving them empty as is traditional, to emphasize how strange it is to just delete stuff like that.)

I think it might still be OK to handle (3). It's nice to have invariants. But it will be unprecedented.

bkardell commented 1 year ago

@domenic specifically tho #3 is hard because details' open attribute is unique, right? Parsing can handle selects with n options claiming to be open or radio buttons claiming to be checked the way that they do without this problem (as 2 examples) because they don't reflect the current state. That's what makes it tricky, the kind of mismatch at the starting point. How would you imagine responding #3? Would parsing change the dom (removing other opens on the same name) or would it break the reflection (seems to me a non-starter)? You're obviously much more knowledgeable there, so maybe you have insights i can't see

domenic commented 1 year ago

Correct, that is why (3) is hard. And indeed, parsing going back and modifying other previously-parsed elements is what I was imagining as the fix.

At first I was envisioning parsing going back and modifying previously-parsed details elements with the same name by removing their open attribute. However this is not right, because we want the first open details of a given name to win. So instead we need to have the parser ignore the open attribute of the second-onward details element with a given name.

mfreed7 commented 1 year ago

I think this mostly stems from the unfortunateness of a "live" open attribute on <details>. There seem to be only two options, and both are bad in one respect or another:

  1. Do like <input type=radio>, which has checked as a content attribute which gets ignored after parsing, and doesn't track the myInput.checked IDL attribute. This solves the (3) problem above, but is also very developer-confusing. It's an attribute with the same name that isn't reflected.
  2. Do what is being proposed in the PR, which respects whatever the developer did, including multiple (invalid) open attributes on a group of <details> elements. Maintain the exclusive accordion status upon user interaction.

To me, both are confusing in some way, but (1) is much more likely to get hit daily. Developers always need to be dealing with the JS checked attribute, and typically see the elements in Devtools wearing their checked content attributes. That's confusing always. In the (2) case, the developer added two open details in their accordion. Presumably that was on purpose? Or most likely they were copy/pasting something and forgot to delete one open. Either way, they'll quickly discover they did this, correct the problem, and never look back. So yes it's confusing, but only for a moment, and only in a corner case.

==> Seems like (2) is the right path.

At first I was envisioning parsing going back and modifying previously-parsed details elements with the same name by removing their open attribute. However this is not right, because we want the first open details of a given name to win. So instead we need to have the parser ignore the open attribute of the second-onward details element with a given name.

Well, if you include multiple <input type=radio checked name=foo> in a page, the last one gets checked, not the first. Still, it feels quite fraught for the parser to try to maintain this state by removing/changing attributes as it parses. It'd be great to avoid this solution.

css-meeting-bot commented 1 year ago

The Open UI Community Group just discussed [exclusive accordion] exclusively non-exclusive....

The full IRC log of that discussion <masonf> q?
<masonf> q+
<brecht_dr> bkardell_: There is one way that summary and details are different. They have a live open attribute that always reflects the current state. This cuases a mismatch on how the spec is currently working
<brecht_dr> bkardell_: An auther could make a exclusive accordion that can have multiple open. Or if you use JS it could also happen that multiple detials are open at the same time.
<masonf> q?
<brecht_dr> bkardell_: It feels strange because of the attribute solution. One idea is not to use summary and details. Other idea that was mentioned was panelset.
<brecht_dr> masonf: Agreed that it is unfortunate that there is a live open attribute.
<brecht_dr> bkardell_: We can either deal with the live open attribute or we don't.
<brecht_dr> masonf: You can do funny things, but it's more of a one time confusion. For example by copy pasting multiple open attributes.
<brecht_dr> masonf: Having the parser actually delete the open attribute, seems even more confusing in my opinion
<masonf> ack masonf
<masonf> ack dbaron_
<brecht_dr> dbaron_: a few thoughts: The idea where the author writes multiple open attributes and only one is open seems confusing because it closes the others. Doesn't feel right. Wether or not the parser fixes it for the author, it still feels like an error
<flackr> q+
<brecht_dr> dbaron_: I also feel that some of these things are edge cases. A developer usually would know what he/she wants to have open. It might be even more common that no item is open at all, which is fine.
<brecht_dr> dbaron_: Still want to catch up a bit more
<masonf> ack flackr
<brecht_dr> flackr: About it being confusing: We do have the exact same problem with the initial checked state of radio buttons.
<brecht_dr> flackr: If we decide that a live attribute was a bad thing and that the details element was named in a way it doesn't suggest it is an accorion, maybe we should consider another element (question?)
<scotto> there is inconsistency with the 'choose one' between elements.
<brecht_dr> flackr: It is the same input resulting in a state when we talk about radio buttons. I did also hear the options that every details element is open until you intereact, which seems perfectly fine.
<scotto> e.g., someone puts multiple summary elements into a details, the first one gets chosen
<brecht_dr> masonf: That is actually the current proposal: All 3 stay open till the user clicks on them
<masonf> q?
<scotto> which i realize is different than the attributes... just pointing out HTML does both - choosing the first or the last depending on the feature
<brecht_dr> bkardell_: Ideally, from a user and author standpoint: You want it to update on first interaction.
<brecht_dr> masonf: I'm worried more about implementers at this stage
<masonf> q?
<brecht_dr> masonf: If I could go back in time, I would choose the open attribute not to be live
<brecht_dr> ntim: The dialog element open attribute isn't relly live, but, "live-ish"
<brecht_dr> bkardell_: My ask is: could we, should we poll this? What people expect in multiple details elements with open attribute inside of an accordion.
<brecht_dr> flackr: It could be decided that this has a different semantic meaning than details.
<masonf> q?
<scotto> are a11y issues being considered though?
<brecht_dr> masonf: Should we add a github vote?
<brecht_dr> bkardell_: I don't want to stop progress
<brecht_dr> masonf: A11Y issues are being concerned
<brecht_dr> q?
<brecht_dr> masonf: We are planning to do the control, styling and accessibility ( that's the idea )
<brecht_dr> masonf: If the general idea is that it's broken and it can't be fixed, then another element would be a better idea, but i hope that is not the case
<masonf> Proposed resolution: keep the existing behavior from the proposed <details name> attribute PR.
<brecht_dr> masonf: A new element would also slow down progress
<brecht_dr> q+
<brecht_dr> dbaron_: It's important to include a note on how important this scenario is
<brecht_dr> bkardell_: I'd rather hear more opinions of people than start a poll
<brecht_dr> masonf: can we ask people what they want based on multiple choice
<brecht_dr> bkardell_: MasonF can open an issue
<masonf> q?
<masonf> brecht_dr: seems ok to me to have all of them start open, and then become exclusive upon user interaction
<brecht_dr> bkardell_: I don't think that any library works like this, which makes it strange
<masonf> github, take up https://github.com/whatwg/html/issues/4180
dbaron commented 1 year ago

So here's a draft of a proposed poll. This is not yet ready to be sent out for voting, so please don't publicize it or vote yet. I plan to edit this comment as updates are needed.

There is a proposal to add a feature to the <details> element in HTML by adding a name attribute to the <details> element. This feature would allow a set of <details> elements to be related to each other because they have the same name. (This is similar to <input type=radio> elements being related based on their name.) The idea of this feature is that it allows multiple <details> elements to be linked in an "exclusive accordion", where opening one of the elements closes the currently-open one. For example, this creates a group using the name options:

<details name=options open>
  <summary>Size</summary>
  ...
</details>
<details name=options>
  <summary>Color</summary>
  ...
</details>
<details name=options>
  <summary>Material</summary>
  ...
</details>

The idea of this feature is that an exclusive accordion always has either zero or one open <details> elements.

In this proposal, the browser helps to enforce this exclusivity idea. If a user clicks on a <summary> to open a <details> element, any other already-open <details> element in the group is closed. The same happens if a web page uses setAttribute to change the open attribute or sets the open property on the element.

Another result of the idea of the feature is that it is considered an authoring error to write HTML like this:

<details name=options open>...</details>
<details name=options open>...</details>
<details name=options>...</details>

because two of the <details> elements are initially open, within a set that is intended to have at most one open. An HTML validator should report this as an error.

Because the <details> element maintains its open/closed state as an attribute, there are cases where it's not reasonable for the browser to immediately enforce the idea that only one <details> element in the group is open. The simplest example of such a case is the "authoring error" case above. This is also the case when <details> elements are added or moved in the DOM by using features like innerHTML or appendChild. (There are also other related cases like handling of changes to the name attribute.)

We'd like to know what developers think should happen in these cases (such as when the HTML parser encounters the example above that involves open attributes on two different elements). The two main options for handling this are:

  1. The elements remain open (as specified in the markup) until a different <details> element in the group is opened, which causes all the open ones to close. This means that the "exclusivity" concept is broken until either the user or a script opens a different member of the group.
  2. The browser closes all but one of the open details elements (leaving either the first or the last open) in a task that happens slightly after the insertion. This means that the state of having multiple <details> elements open will be visible to script for a period of time, but will get corrected before the document is visually displayed to the user. This also means that the specification will need to say that the browser picks either the first or last of the open <details> elements as the one that was intended to be open.

What do you think about these options?

Please (NOT YET!) vote with a single emoji reaction to this comment.

bkardell commented 1 year ago

Not sure how you really want suggestions or questions. I took a crack at mods to this that I think adds some words that felt missing, removed some that felt unnecessary, and just added some whitespace/heading. Not sure if it is helpful

https://gist.github.com/bkardell/65f86e6a8e76b3650c5495ff9c29af9a

I also had some questions that I left in the discord, but I'll repeat here that what is described here has a mismatch with one of the pens I illustrated with originally: https://codepen.io/briankardell/pen/jOQBzOw?editors=1111

This changes the name attribute of an open details to move it to a new group[1]. In that case currently exclusivity is broken as well, and if I try to close A, it closes and then reopens! Something is a bug there for sure - how much is, I guess part of the question. The text above is also a little confusing to me as it lists exclusivity as being enforced when a user acts to open or close one - but option 1 in "what do we do" seems to suggest it would only be righted on opening? Should it say opening or closing? Is that related to the behavior problem I described above with this pen?

[1] I know this seems bizarre, it's not that I am suggesting someone has a use case for that (tho, I bet they do - the web is so big) - it's more that you could accidentally create a similar situation by accidentally dynamically adding an 'open' before the name but after insertion or something.

mfreed7 commented 1 year ago

Not sure how you really want suggestions or questions. I took a crack at mods to this that I think adds some words that felt missing, removed some that felt unnecessary, and just added some whitespace/heading. Not sure if it is helpful

https://gist.github.com/bkardell/65f86e6a8e76b3650c5495ff9c29af9a

I couldn't see the differences here, so they must be small. Both versions (comment above and this gist) read really nicely to me. LGTM to post either of them.

I also had some questions that I left in the discord, but I'll repeat here that what is described here has a mismatch with one of the pens I illustrated with originally: https://codepen.io/briankardell/pen/jOQBzOw?editors=1111

This changes the name attribute of an open details to move it to a new group[1]. In that case currently exclusivity is broken as well, and if I try to close A, it closes and then reopens! Something is a bug there for sure - how much is, I guess part of the question.

Do you think this is a fundamental issue, or perhaps just a bug in the implementation? If the latter, I'd love not to confuse this issue with that bug. But of course, if this represents a more fundamental problem in the approach, then I think maybe it'd help to explain why it's broken that way?

The text above is also a little confusing to me as it lists exclusivity as being enforced when a user acts to open or close one - but option 1 in "what do we do" seems to suggest it would only be righted on opening? Should it say opening or closing? Is that related to the behavior problem I described above with this pen?

I'm not sure where it says closing a <details> triggers exclusivity. It should (and I think does) just say that opening triggers behaviors. Closing a details shouldn't close other details, because when they're exclusive, the others are already closed.

dbaron commented 1 year ago

I've updated the comment to reflect many of the changes in Brian's gist. The main set of changes that I didn't take was the restructuring around explaining the error cases; I still think it's useful to say first that the case is an authoring error (which I think it should be under either proposal), and then to raise the question of how that authoring error should be handled.

This changes the name attribute of an open details to move it to a new group[1]. In that case currently exclusivity is broken as well, and if I try to close A, it closes and then reopens! Something is a bug there for sure - how much is, I guess part of the question. The text above is also a little confusing to me as it lists exclusivity as being enforced when a user acts to open or close one - but option 1 in "what do we do" seems to suggest it would only be righted on opening? Should it say opening or closing? Is that related to the behavior problem I described above with this pen?

Yeah, so I think name attribute changes are an intermediate case -- it's something that we could handle technically, but I'm also not sure if we want to, since it does force us into making an arbitrary choice about which of the relevant details elements are supposed to be open.

I think that if A closes and then reopens when you try to close it, that would be a bug. I don't see that bug when testing locally, though! (Maybe I saw something weird happen once, but I couldn't get it to happen again.)

I do think only righting things on opening is a reasonable choice because it's the choice that doesn't force the browser into picking which one was the right one to stay open. (We would have to do that if we tried to right-on-close when closing one of three open elements in a group.)

bkardell commented 1 year ago

I think that if A closes and then reopens when you try to close it, that would be a bug. I don't see that bug when testing locally, though!

Yeah I'm pretty sure this is a bug, but since lots of what do to with conflicts here seems a little unintuitive to me, I am just trying to make sure what's accident vs intent... It kind of works two ways depending on what seems to be subtly what you do with your pointer after clicking... I have no explanation really but here's a video explaining/showing https://drive.google.com/file/d/1u0Et_21FdIET-enP0ytuCZdkYbgHLxHj/view?usp=drive_link

Yeah, so I think name attribute changes are an intermediate case -- it's something that we could handle technically, but I'm also not sure if we want to, since it does force us into making an arbitrary choice about which of the relevant details elements are supposed to be open.

For me this is a kind of critical question to answer before asking the question: How many "exceptions to the rule" are there where it is possible to create an actual UI state that is not "not actually exclusive"? If you add an element to a group in any way I can see which already states it is open, that creates this problem, I think - and there are several ways to do that (original source, fragments, subtrees, changing the name, at least). Are there others? Are we asking "What happens in all of those cases" or specifically about when someone sends markup in the original http response source?

mfreed7 commented 1 year ago

For me this is a kind of critical question to answer before asking the question: How many "exceptions to the rule" are there where it is possible to create an actual UI state that is not "not actually exclusive"?

I think it’s important to state that “the rule” is that the grouped details elements will maintain exclusivity across user interactions. That’s the tough part, which today requires JS, which we’re trying to automate. We’re not trying to build a set of details elements that cannot be made in any way to be open at the same time, no matter what DOM manipulations are performed or invalid HTML is provided.

I think you’re concerned that this behavior is going to cause actual problems for developers - perhaps you can elaborate on the concrete use case, framework, or whatever that you think will have issues with the proposed behavior? Then we can examine that use case and see if anything can be done about it?

dbaron commented 1 year ago

My sense is that we could complicate the poll by trying to ask about dynamic changes to name as well, but I think we can also try to extrapolate the answer that makes sense for dynamic changes to name based on the answer for the question that I've already described. (That is, I would probably lean towards enforcing exclusivity for dynamic changes to name if and only if we enforce exclusivity for DOM insertions.) I think there's a limit to how complex we can make a poll question like this and still get useful signal from it.

bkardell commented 1 year ago

We’re not trying to build a set of details elements that cannot be made in any way to be open at the same time, no matter what DOM manipulations are performed or invalid HTML is provided.

To be honest i thought that was literally the question at hand so I'll have to admit I'm a little confused now. Nevertheless, i spoke with David and i trust he'll do his best to convey the concern and I'm fine even pre-saying I'm ok with setting up a poll with whatever he writes there.. He has my feedback as best i can give it. (i think it would be better in a fresh issue so it doesn't feel already overwhelming though)

mfreed7 commented 1 year ago

We’re not trying to build a set of details elements that cannot be made in any way to be open at the same time, no matter what DOM manipulations are performed or invalid HTML is provided.

To be honest i thought that was literally the question at hand so I'll have to admit I'm a little confused now.

I'd say the concrete use case is: "I'm a developer and I want to implement a simple accordion control, but I don't want to have to worry about all the Javascript it takes to get that right." The current proposal does that, I think.

Nevertheless, i spoke with David and i trust he'll do his best to convey the concern and I'm fine even pre-saying I'm ok with setting up a poll with whatever he writes there.. He has my feedback as best i can give it. (i think it would be better in a fresh issue so it doesn't feel already overwhelming though)

Great! Thanks.

LeaVerou commented 1 year ago

I’m quite concerned that authors would not want to use an exclusive accordion that does not guarantee exclusivity, since that could really break layouts. It is also quite confusing that while it uses the same grouping mechanism as radios, it behaves very differently. OTOH @mfreed7 hit the nail on the head here wrt the issues involved.

Not sure what the right solution is here, it's a really tough problem. It is possible that the best solution hasn't been proposed yet.

domenic commented 1 year ago

FWIW I think that although removing extraneous open attributes during parsing is unprecedented, and is a bit weird, it's the least-bad option. I like how it preserves the invariant.

There is indeed some weak precedent, in how parsing does plenty of other "mutations" already, when you have invalid markup. E.g. <b><i>foo</b></i> -> <b><i>foo</i></b>, inserting implicit <body> and <html>, <i class="x" class="y"> -> <i class="x">, <body a="1"><body b="2"> -> <body a="1" b="2">, and so on. This proposal definitely goes further, in that it's enforcing a more "semantic" constraint than any other things the parser currently does. But maybe this is some evidence it would be acceptable.

mfreed7 commented 1 year ago

There is indeed some weak precedent, in how parsing does plenty of other "mutations" already, when you have invalid markup. E.g. <b><i>foo</b></i> -> <b><i>foo</i></b>, inserting implicit <body> and <html>, <i class="x" class="y"> -> <i class="x">, <body a="1"><body b="2"> -> <body a="1" b="2">, and so on. This proposal definitely goes further, in that it's enforcing a more "semantic" constraint than any other things the parser currently does. But maybe this is some evidence it would be acceptable.

You're right that those are precedents for parser shenanigans. However, correct me if I'm wrong, but there is a treasure trove of XSS vectors listed in the sentence above. I'd hate to add another set of those.

I’m quite concerned that authors would not want to use an exclusive accordion that does not guarantee exclusivity

Can you expand on this? The existing proposal will guarantee exclusivity, so long as you don't provide it broken markup in the first place. I.e. if you hold it right, which is relatively straightforward, it'll do what you want. Is there a real world use case that you can see being broken? I'm honestly asking - I haven't heard any but perhaps there are some.

LeaVerou commented 1 year ago

I think I'm tending to agree with @domenic that out of the options currently being discussed, stripping out extraneous open attributes does seem like the least bad.

@mfreed7:

The existing proposal will guarantee exclusivity, so long as you don't provide it broken markup in the first place. I.e. if you hold it right, which is relatively straightforward, it'll do what you want. Is there a real world use case that you can see being broken? I'm honestly asking - I haven't heard any but perhaps there are some.

If there are cases where it's not exclusive, then it does not guarantee exclusivity. At best, it merely encourages it. Broken markup can happen in all kinds of well-intentioned scenarios, since markup is often generated dynamically. E.g. in Vue you can do things like:

<details id="foo" :open="defaultPanel === 'foo'">
...
</details>
<details id="bar" :open="defaultPanel === 'bar'">
...
</details>

I can easily imagine cases where authors got buggy expressions or made typos and ended up with multiple panels open at once. There's the argument that breaking badly helps authors debug by making errors obvious, but the counterargument is that if the breakage is often experienced by end-users first, so breaking gracefully tends to be a good quality in an API (HTML vs XHTML being an extreme example of this).

dbaron commented 1 year ago

I've posted the poll itself at #812. Please share it (preferably without advocacy to choose a particular option, so that people can make their own choices after reading it).

Regarding modification of attributes during parsing -- I think we don't know what problems and constraints are going to exist if we go down that path, and we also don't even know when we'll find out about problems if we try it. (For comparison, I think it took a decent amount of time before we understood the problems and constraints caused by mutation events.) Essentially, I think there are enough unknown unknowns, and thus enough risk, that it's not worth trying it for this magnitude (small) of feature.

stephband commented 1 year ago

I want something more generic than an extension to <details>. I had thought that the recently proposed popover attribute was a step in the right direction, being an attribute we can apply to any element to give it some two-state show/hide behaviour. Why can't we have another attribute that gives us one-of-many show/hide behaviour?

This attribute would be so flexible, it would allow us to design all sorts of things that require this behaviour without lumbering us with details/summary semantics, style and transition difficulties, and the fact that details requires its toggle button (the <summary> element) to be contained inside it. Tabs, for example:

<button target="tab-1">Title 1</button>
<button target="tab-2">Title 2</button>

<ul>
    <li switchable open id="tab-1">
        ...
    </li>

    <li switchable id="tab-2">
        ...
    </li>
</ul>

I have been using my own system of behaviour attributes for many years. I like the separation they afford between semantics and behaviour, and how composable they are with existing elements.

I fear that by giving this behaviour only to the <details> element we will end up with another API that is unique to one element only, like <dialog>. And we will still have to carry on using the old solutions for tabs, slide-shows and the like anyway, when they could benefit from this built-in one-of-many switchable behaviour.

dbaron commented 1 year ago

One note on the complexity of the poll: the poll currently has 22 votes on it. My social media post announcing the poll currently has 19 boosts and 10 favorites. I think that says the poll was likely already too complex.

LJWatson commented 1 year ago

The explainer suggests it may be necessary to change the roles for <details> and/or <summary>. I'd like to suggest this is not necessary, at least from a screen reader user's point of view.

For a single details/summary component it's the state of the <summary> element that informs the user what they're dealing with. Essentially that there is a button and that something is collapsed or expanded.

That remains true when the component is one in a set of multiple such components. The essential interaction doesn't change, but the consequences of interacting with one component influence the behaviour of the other components in the set.

Instead of complicating things with different roles, I suggest leaning into existing/learned patterns - notably radio buttons.

The name attribute causes the number of things in the set to be exposed in the acc tree. A screen reader user might hear something like "Radio button, 1 of 3, checked" for example.

If the same UX is applied to this exclusive accordion pattern, the screen reader user might hear "Button, 1 of 3, collapsed", and the fact of it being one of a set is the bit that conveys the difference in behaviour.

Note, I've limited the example announcements to role/state info only. Ordinarily the accessible name for the element would be announced too of course.

annevk commented 1 year ago

We already have attribute mutation upon element insertion, right? nonce is such an attribute. Doing that again seems okay, although it's definitely weird.

dbaron commented 1 year ago

I think @annevk's example of nonce in https://github.com/openui/open-ui/issues/786#issuecomment-1692041464 does point the way towards a non-delayed variant of option 2. It does seem less problematic, or at least less unknown, to change an attribute on the element being inserted in the middle of insertion, rather than changing attributes on entirely different elements.

Furthermore, https://github.com/openui/open-ui/issues/812#issuecomment-1689980112 points out that it's better for progressive rendering of a slowly-loading document to change the openness of the newly-inserted details rather than the one that's already in the document. This points in the same direction as well (i.e., also points towards removing the open attribute from the newly-inserted element rather than from a different element).

I've started to prototype this to see how difficult it would be (and I plan to continue doing this, so I may well have more thoughts on it as I fill in more code and write tests!). But what I'm currently thinking is that this approach would look something like:

That said, I'd note that the developer poll is currently closer to 50-50 (although leaning towards option 2) than it is towards a clear consensus. My impression is that developers do disagree on which approach has better developer ergonomics. But I think it's worth exploring the implementability of this approach (given the new suggestion) so we can consider both aspects together.

dbaron commented 1 year ago

My attempt to prototype this in Chrome led to hitting a DCHECK() failure that says my code could potentially fire mutation events when mutation events are not allowed. It's not clear to me why the implementation of nonce doesn't hit this; I don't see anything based on code inspection, and I'm going to have to debug it.

css-meeting-bot commented 1 year ago

The Open UI Community Group just discussed [exclusive accordion] exclusively non-exclusive....

The full IRC log of that discussion <jarhar> dbaron: we talked about two weeks ago about doing a developer poll about this question about whether exlusive accordions should always enforce exlclusivity when open attribute is present or when calling from script
<jarhar> dbaron: i originally specced it as saying it doesn't strictly enforce it, so if you write details open on two details elements that are in the same group, then they are both open until user opens a third one, then it would close those two. brian was not happy about that, thought it was bad DX
<jarhar> dbaron: i thought there was pluses and minuses
<jarhar> dbaron: various technical reasons i did this
<jarhar> dbaron: some of which people have pointed out solutions to, some of which are still up in the air
<jarhar> dbaron: we did a developer poll which i largely wrote the wording of but brian provided feedback
<jarhar> dbaron: the results were not overwhelming. we got a lot of responses, the results are i would say probably 60% to 40% in favor of brians view
<jarhar> dbaron: the voting results were slightly in favor of brians view, but there were a decent number of folks who thought the dev ergonomics were better one way or the other
<masonf> Option 1: 55, Option 2: 86
<gregwhitworth> q?
<jarhar> dbaron: i was hoping to say, "anne pointed out this thing in this other attribute that already works that way" and we could just do what brian prefers, but im not ready to say that
<jarhar> dbaron: i prototyped it, and it doesnt work
<jarhar> dbaron: i still have to figure out why the other thing i based it on works fine and this one doesnt
<jarhar> dbaron: hopefully soon ill be able to say im comfortable going either way
<bkardell_> q+
<jarhar> dbaron: im curious if other folks think that 60/40 dev poll if we are comfortable doing that if its more complex, should we do it that way
<Luke> q?
<gregwhitworth> ack bkardell_
<jarhar> bkardell: i would ask and make sure that you think that nothing i said in the thread sort of poisoned the poll
<jarhar> dbaron: i dont think so
<jarhar> bkardell: a few people did change their mind after they heard other details. some people read it specifically as i give you markup from the server and this is the only time that happens, but in reality there are multiple circumstances where that happens
<masonf> 61% for option 2
<jarhar> bkardell: 55 votes for option 1, and 86 for option 2, i think if anything its probably more slanted than it appears
<jarhar> bkardell: im surprised by that, i should be surprised since every poll we do is like this, this is the most lobsided weve ever seen
<Luke> Isn't it 68 votes option 1?
<jarhar> bkardell: it makes me feel better that there are other people who think that option 1 is viable
<masonf> q?
<masonf> q+
<jarhar> bkardell: im not going to specifically object to option 1, i just dont think that its a very good solution
<jarhar> bkardell: we are already inching out such a specific definition of it, with that 30 lines of js you can do more than this
<jarhar> bkardell: i was worried that devs were gonna complain about this and say that you can do better with a little js
<jarhar> bkardell: it seems like people wouldnt do that
<jarhar> bkardell: im glad we had the conversation and i dont think we have definitive results
<jarhar> bkardell: it does lean towards the way i see it
<jarhar> bkardell: its up to the group really
<Luke> q?
<ntim> q+
<gregwhitworth> ack masonf
<jarhar> masonf: my sense is that most of the objection to option 2 was that there are strange technical things you have to do to get that to work
<jarhar> masonf: there are cases where you remove attrs that are in the html. there are proposals that make that work. id say wait until david is done to verify that it is implemetnable
<jarhar> masonf: option 1 would be great, but if it isn't possible to do then we cant do it
<jarhar> masonf: whatever somebody did to publicize this, it was the most voted poll ever in openui
<masonf> q?
<gregwhitworth> ack dbaron
<jarhar> dbaron: when i wrote the poll i left option 2 vague in terms of behavior to enforce exlclusivity
<bkardell_> q+ to clarify something quickly
<jarhar> dbaron: i got feedback about progressive rendering. interaction with progressive rendering and technical issue both point in the direction that is both better and more likely to be implemnetnable is that if youre inserting something into the docuemtn and theres already an open one, then you remove the attribute from the new one because you dont want to close one thats already open
<jarhar> dbaron: also better from the tech perspective because youre messing with attributes on the thing being inserted which is a little less weird
<jarhar> dbaron: i posted it on mastodon and it got a lot of boosts
<jarhar> dbaron: i was worried because the number of boosts and votes were about the same
<jarhar> dbaron: it was less than 2:1 ratio
<jarhar> dbaron: which meant that some of the people boosting werent voting
<gregwhitworth> ack bkardell_
<Zakim> bkardell_, you wanted to clarify something quickly
<jarhar> bkardell: i noted this carefully in the original issue, i just want to reiterate that all of the complexity of why it is this way is specifically unique to details
<jarhar> bkardell: we are starting from the most problematic place, an alternative is to just not fire events
<jarhar> dbaron: there are a few other elements, dialog does it state the same way
<masonf> ack ntim
<jarhar> ntim: doesnt option 1 provide more flexibility for developers? if they want to do not exclusive initially, then enforce it on user action?
<jarhar> masonf: that was one of the points made for option 1
<gregwhitworth> q?
<bkardell_> q+
<dbaron> (got my network disconnected, and then easily able to rejoin)
<jarhar> masonf: the counterpoint is that its funny, its funny capability. you get to do whatever you like in html, but then when you click it goes away. im paraphrasing though
<gregwhitworth> ack bkardell_
<jarhar> bkardell: my pushback is that there are a lot of accordions, point me to one that did that
<bkardell_> we have ressearch for a reason, right?
<jarhar> dbaron: with what i was proposing before, my approach was to say this is an authoring error and one that should be shown by the validator if its in the markup just to make it clear that its not the intent of the feature to do that sort of thing
<jarhar> masonf: so its technically outlawed before, just possible
<masonf> q?
<dbaron> (maybe "just" was the wrong word there, multiple reasons...)
<bkardell_> good clarification dbaron :)
<jarhar> masonf: no resolution here, just make sure option 2 is implementable and come back to discuss again?
dbaron commented 1 year ago

So the reason the manipulation of the nonce attribute is OK but the manipulation of the open attribute appears not to be (based on the DCHECK() failure) is that the manipulation of the nonce attribute sets the attribute to the empty string (but does not remove it).

In Chromium and WebKit, modifying attributes does not fire mutation events, whereas adding or removing them does. This was changed in bug 81141; see the relevant Chromium code comment. Based on a skim of the code, modifying attributes looks like it fires both a DOMSubtreeModified mutation event and a DOMAttrModified mutation event in Gecko. A test%3B%0At.addEventListener(%22DOMAttrModified%22%2C%20event%20%3D%3E%20%7B%0A%20%20%20%20w(%60%24%7Bevent.type%7D%20%24%7Bevent.attrChange%7D%20%22%24%7Bevent.attrName%7D%22%20%22%24%7Bevent.prevValue%7D%22%20%22%24%7Bevent.newValue%7D%22%60)%3B%0A%7D)%3B%0A%0Aw(%22adding%20attribute%3A%22)%3B%0At.setAttribute(%22data-test%22%2C%20%221%22)%3B%0Aw(%22changing%20attribute%3A%22)%3B%0At.setAttribute(%22data-test%22%2C%20%222%22)%3B%0Aw(%22removing%20attribute%3A%22)%3B%0At.removeAttribute(%22data-test%22)%3B%0A%0A%3C%2Fscript%3E%0A) shows the same results: Gecko fires two mutation events for atttribute changes, additions, and removals, whereas Chromium and WebKit fire one mutation event for additions and removals and none for changes.

I'd also note that the Gecko code for implementing the nonce attribute does it asynchronously, which I think contradicts the relevant part of the spec for nonce.

mfreed7 commented 1 year ago

Based on a skim of the code, modifying attributes looks like it fires both a DOMSubtreeModified mutation event and a DOMAttrModified mutation event in Gecko. A test%3B%0At.addEventListener(%22DOMAttrModified%22%2C%20event%20%3D%3E%20%7B%0A%20%20%20%20w(%60%24%7Bevent.type%7D%20%24%7Bevent.attrChange%7D%20%22%24%7Bevent.attrName%7D%22%20%22%24%7Bevent.prevValue%7D%22%20%22%24%7Bevent.newValue%7D%22%60)%3B%0A%7D)%3B%0A%0Aw(%22adding%20attribute%3A%22)%3B%0At.setAttribute(%22data-test%22%2C%20%221%22)%3B%0Aw(%22changing%20attribute%3A%22)%3B%0At.setAttribute(%22data-test%22%2C%20%222%22)%3B%0Aw(%22removing%20attribute%3A%22)%3B%0At.removeAttribute(%22data-test%22)%3B%0A%0A%3C%2Fscript%3E%0A) shows the same results: Gecko fires two mutation events for atttribute changes, additions, and removals, whereas Chromium and WebKit fire one mutation event for additions and removals and none for changes.

This aligns with my local testing of Mutation Events behavior across browsers. It's not interoperable.

I'd also note that the Gecko code for implementing the nonce attribute does it asynchronously, which I think contradicts the relevant part of the spec for nonce.

I definitely think we should do this (remove the extra open attributes) synchronously if possible.

annevk commented 1 year ago

Could potentially poke further holes in mutation events by not firing them for open attribute mutations.

(Gecko script runners run pretty quickly so it might be tricky to construct a test case that proves they don't follow the standard.)

dbaron commented 1 year ago

What I tried, which is somewhat similar, is not firing mutation events for the open attribute mutations (removals) that result from the insertion of a <details> element into the DOM. I still fire the toggle events, though, since they're asynchronous. This appears to work (see work in progress), though I should write some additional tests.

dbaron commented 1 year ago

More specifically, what I'm proposing is the following changes to the behavior (using some terminology from https://github.com/whatwg/html/pull/9400 ), which I believe together lead to enforcing the invariant that at most one of the <details> elements in an exclusive accordion is open at a time:

The end result of this is that:

Note that this means that dynamic changes of the open attribute and dynamic changes of the name attribute are handled in opposite ways. I think this is fine, and I think it makes more sense to handle a dynamic change to the name attribute like an insertion (since both are operations that add a member to the group).

css-meeting-bot commented 1 year ago

The Open UI Community Group just discussed [exclusive accordion] exclusively non-exclusive..., and agreed to the following:

The full IRC log of that discussion <jarhar> dbaron: we discussed this issue a few weeks ago, at the time i was - the results from the developer poll was 60/40 in favor of enforcing exclusive accordions
<jarhar> dbaron: i was worried that it would be too difficult
<bkardell_> q+
<jarhar> dbaron: i adjusted how it would work, but i found a way that was not too difficult and enforced it
<jarhar> dbaron: i have an impelemntation of that, and i updated the spec pr
<jarhar> dbaron: the conclusion last time was if this is going to work then we should go that way
<jarhar> dbaron: after trying it i think it will work, so im going that way.
<jarhar> dbaron: i still need to write a reply on the discussion on the issue
<jarhar> dbaron: i figured its worth it to give that update
<masonf> Proposed resolution: exclusive accordions should be "always exclusive", i.e. Option 2.
<jarhar> dbaron: seeing if people are still ok with that approach
<gregwhitworth> ack bkardell_
<jarhar> bkardell: thank you for doing that david. i think its a big improvement
<jarhar> bkardell: the direction of it was not what i expected, i thought that the last one would be not the first one, but now i understand
<jarhar> bkardell: now theres no inconsistency, i think it turned out really well
<keithamus> +1
<masonf> RESOLVED: exclusive accordions should be "always exclusive", i.e. Option 2.
<bkardell_> shipit
<zcorpan> Zakim: make minutes
bkardell commented 1 year ago

I feel like this is resolved and I am voluntarily closing it. Feel free to reopen if we think that's wrong, but closing issues seems like a Good Thing to me

yisibl commented 6 months ago

Can we add some way to turn all details open or close?