Triaging to @aghassemi , feel free to re-assign 😄

related #22381

@caroqliu , do you think there will be time to get to this sometime soon? I imagine it wouldn't be hard to implement... hopefully... perhaps?

Or perhaps it would make a good first issue for aspiring contributors?

Just pinging this up...

Hey @morsssss, thanks for keeping track of this! Yes, we are expecting to get to this in the coming weeks.

@SimonHogstromRonbol Would allowing just <p heading>* in a section's first child be sufficient for your purposes? It may not be the best idea to allow all tags to be headers, for example, do we want <video> tags as clickable headers? Is there a subset of tags we can reasonably allow to be headers?

Implementation options

If we allow only a subset of non-heading tags to be headers (with the heading attribute of course), I can simply modify this component's AMP validation rules to accommodate those tags.

If we allow any non-heading tag to be a header, as long as it has the heading attribute, there are 2 options

1. Find some way to add a wildcard validation rule that allows all non-heading tags to be the first child as long as they have a heading attribute.
2. Check this rule at runtime.

* or maybe <p data-header> because of this

I personally would bias toward giving developers the benefit of the doubt. Certainly most people would discourage a developer from making a <video> tag the header (and I think this is a wonderful example), but a determined AMP developer could do the same thing in other ways - say, by using events & actions.

In other words, you can already build your own accordion in AMP using events, actions, and animations, using any elements you'd like, so I wouldn't restrict what the header tag should be.

That's just my opinion, though. If you want to keep this safer, I personally prefer the wildcard validation rule approach over a check that occurs at runtime.

@codyjrivera allowing <p heading> or <p data-header> to be the header for the accordion would solve our use case, but as @morsssss pointed out, if we look beyond this specific use case, we might want to question why we restrict what elements can be the header in the first place.

If the reason for disallowing certain elements is not for the betterment of performance, and not for the betterment of user friendliness, but instead based on what we feel would suit the element best, we start restricting the creativity of developers, and I know this sounds a little lofty, but being a developer whose main focus is on building sites using AMP, and hearing from the people on my team that are also mainly focused on AMP, the frustration starts, when they feel like a rule doesn't serve a purpose other than making an AMP element fit a design or an opinion put forth by the developer behind it.

So, while I agree that using a <video> element seems like it wouldn't fit well as heading for the accordion, maybe someone out there is making something funky with video descriptions and looking to do exactly that, we don't know.

So if I were to choose, I'd suggest that adding a heading or data-header attribute to any element that is a direct child element in each section should be allowed.

Taking a deeper look at this, this seems to be the only AMP component that uses the validator to enforce its child tags to be <h1>-<h6> and other "text" tags. If I am mistaken, please let me know. It appears that both the current implementation of <amp-accordion> and the suggestion to allow a few new header tags such as <p heading> are at odds with how other AMP components are implemented. Thus, we have a compelling case to do what is argued above and allow all tags to be headers.

@morsssss Is there even a reason for us to check non-<h1>-<h6> and <header> tags for a heading or data-header attribute? If we just document that the first child of a <section> tag is always a header, does that suffice?

I don't think there's any reason to check those tags. I'd guess that the <h*> requirement comes a time when AMP was young, and before it was employed for such a diverse range of use cases.

It might have been more logical to have people tag the header in some way, not to simply assume that the first child is the header, but that horse is already out of the barn. Um, that's not the expression. What's the expression? Anyway, I mean to say, <amp-accordion> takes the first child to be the header, and it's documented, and it's widely used that way.

If someone wants to use something strange for the header, that's their choice, and I don't think that could be used to create a layout that shifts before user action or otherwise harm our guarantees for performance and layout.

Thanks for this discussion - and thanks for being flexible :)

@SimonHogstromRonbol Though it's not clear why we restricted the header on the first place, on second look I would argue there is an accessibility case for keeping new allowances at least well-defined. We currently preventDefault on all click events to the header to prioritize the accordion expand interaction. The only exceptions to these are if the click occurs on a nested anchor tag or if the target has a tap action. When we allow any element to act as a header, we are agreeing to exhaustively support user clicks on them one way or another, including elements like <video>, <detail>, yes, but also any element that may have a click handler assigned to them via amp-script.

@caroqliu Good point that we have to take accessibility into account when we change the default behavior of certain elements, would it make sense to go from an allowlist approach to a blocklist approach, so instead of just allowing <h*> tags, we could block, form elements, video, details, anchor tags, and so forth? This would solve the issue that me and my team are having.

Alternatively, I feel like in a perfect world we'd be able to define any non-<h*> tag with a heading or data-header attribute to make it the heading of the accordion, and then if the element is one of those elements that might cause an accessibility issue when preventDefault is put on, we could throw a warning to the console informing the developer, when you're in development mode (#development=1), that the chosen header might cause accessibility issues and that they might want to consider using something a different element.

I agree with that. To avoid potentially disallowing valid use cases that we can't always predict, I think it would be best to block a few undesirable elements with a denylist.

@ampproject/wg-caching Is there a way of enforcing a denylist on child tags using the validator syntax? There's first_child_tag_name_oneof and child_tag_name_oneof for allowlists, but are there similar methods for denylists?