Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Details/summary block #45055
Add Details/summary block #45055
Changes from 12 commits
be21b7f
9686338
ed02fa7
340fe0e
7468272
04b8f85
80375c0
84ede00
843ab63
3c2397f
8810fb0
82ce6b0
b56a2a7
26b07c4
14d6974
125b246
82643c4
10ffe31
c75a4aa
bc421cc
bd3474a
19cf689
a7c05f2
c31508c
4aef03e
0297f7d
a1698ee
98ee1ee
b740036
d72a333
95482f0
3e2cd78
d4b3923
767c4a2
8146396
89a07cf
cb2a3d8
7dc6665
460041a
c923448
ff578c2
52a404c
c9382af
64be240
492ad92
90f5249
731a8e8
eb47efc
597f6b6
772a334
2a3b19b
995102f
9edd68e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a small disparity between the editor and frontend when using border radius:
Editor
Frontend
It's something that can be fixed in a follow-up and shouldn't block merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to reproduce this.
I am using Twenty Twenty-Three.
I do not see the clipping on macOS, in Firefox, Safari, or Chrome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work now too:
Very strange. I tried switching between a few different themes and couldn't reproduce.
Maybe it was an issue with my local environment. Both this and the previous issue could have been due to styles not loading properly on the frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a relief, thank you for double checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where headings wouldn't make semantic sense? That is, should we also allow body text (
<span />
) or something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joedolson what do you think? Could the content in
<summary>
use other tags than headings?It is valid HTML but do we want to allow it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much any inline tag can be used in
summary
, though some of the allowed tags would be a bit strange, likeaudio
.Restricting the content to a heading is what makes the most sense to me. Even if other tags are allowed, they could be attached within the heading (e.g., a heading containing
span
,strong
,mark
, etc.)I can imagine cases where the heading is not super helpful (e.g., if somebody uses the block to reveal single images with no alt text, then there's effectively no contained content, and the heading structure has no meaning); but I think that's more a misuse of the UI than a problem with the structure of the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that nesting the rich text inside a heading inside the summary would be unnecessary complex, especially if we want to be able to select the HTML tag for both the heading and the inner element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a big problem here: a
summary
element breaks the semantics of headings.https://daverupert.com/2019/12/why-details-is-not-an-accordion/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only tested with VoiceOver which did include the heading inside the summary when navigating via headings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I would rather have the block with a plain
<summary>
without inner element, than not have it available at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZebulanStanphill Note that article is 3 years old; given the rate of change in the accessibility API stack, it shouldn't be considered a strong reference. There are mixed opinions about whether headings are good or bad inside a
summary
element, so it's a decision that we would need to make. In my opinion, we should include a heading inside the summary.Here is some testing data: https://a11ysupport.io/tests/tech__html__details-summary#assertion-html-h1-6_elements-convey_role_and_name-
Note that 'fail' means "heading semantics were ignored by the screen reader", which is not the same as "this is not accessible". The open question is whether or not a
summary
should have both control and heading semantics, and not all API trees/AT agrees on this point. However, it does mean that the relationship between what a simple testing tool (like HeadingsMap) will tell you about the heading hierarchy on your page and the real experience that an AT user will have is broken.One argument in favor of allowing element choice inside the summary is that it allows a user to make their own decision about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joedolson Good point.
@carolinan I agree that a
<details>
block is desirable regardless of whether or not headings are ultimately allowed in its<summary>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about
allowedBlocks
here (context)... I've been testing with the latest posts blocks, image, group and a bunch of others, but I think it might be useful to restrict such blocks as cover and media text etc.Maybe even restricting container blocks such as group and column to avoid any layout weirdness.
What do you reckon about allowing all text blocks, image block, audio, and video?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is easier to start with a limitation, than trying to add one later.
Text and image blocks, yes.
I am not as sure about the audio and video. If the block is used as a transcript for video it probably would be a bad idea for the block to include another video.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if it was used as a disclosure for an FAQ or a series of instructions, enclosing videos could make sense. I think that's reasonable.
It would be good to ensure that any headings inside the content are below the level of the details headings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a way to have a disallow list rather than allowed list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same thing when looking at this! I'll make a note. It might be a useful feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some related discussion about allowed/disallowed on the original issue #5540 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
<BlockIcon />
not work in this context? Just asking out of curiosity.