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

Initial implementation of <details> block #23940

Closed
wants to merge 18 commits into from
Closed

Conversation

georgeh
Copy link
Contributor

@georgeh georgeh commented Jul 14, 2020

Description

Adds a <details> block

How has this been tested?

Screenshots

Editor:

image

Rendered:

Closed:
Screen Shot 2020-07-14 at 4 19 06 PM

Opened:
Screen Shot 2020-07-14 at 4 19 11 PM

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Jul 14, 2020

Size Change: +551 B (0%)

Total Size: 1.16 MB

Filename Size Change
build/api-fetch/index.js 3.44 kB -1 B
build/block-directory/index.js 7.96 kB -1 B
build/block-library/editor-rtl.css 8.38 kB +19 B (0%)
build/block-library/editor.css 8.38 kB +19 B (0%)
build/block-library/index.js 133 kB +478 B (0%)
build/blocks/index.js 48.4 kB -1 B
build/components/index.js 200 kB -3 B (0%)
build/core-data/index.js 11.8 kB -1 B
build/data/index.js 8.45 kB -1 B
build/edit-navigation/index.js 10.9 kB -2 B (0%)
build/edit-post/index.js 304 kB +5 B (0%)
build/edit-site/index.js 17 kB -1 B
build/edit-widgets/index.js 9.38 kB -1 B
build/editor/index.js 45.3 kB -2 B (0%)
build/element/index.js 4.65 kB +1 B
build/format-library/index.js 7.72 kB +1 B
build/i18n/index.js 3.56 kB -2 B (0%)
build/keyboard-shortcuts/index.js 2.52 kB +2 B (0%)
build/keycodes/index.js 1.94 kB +1 B
build/media-utils/index.js 5.33 kB +1 B
build/plugins/index.js 2.56 kB -1 B
build/rich-text/index.js 13.9 kB +41 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 125 kB 0 B
build/block-editor/style-rtl.css 10.6 kB 0 B
build/block-editor/style.css 10.6 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.49 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/style-rtl.css 5.63 kB 0 B
build/edit-post/style.css 5.63 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@georgeh
Copy link
Contributor Author

georgeh commented Jul 14, 2020

@enriquesanchez I have some questions about a11y with this. Ideally I'd like to use the native <summary> element in the editor to make the editor appear the same as the rendered. The challenge is that the element listens for the spacebar keypress and toggles open/closed. The effect is that as you are writing the hidden part flashes open/closed.

I think I can disable the spacebar toggle, but will keyboard users still be able to toggle the visible portion? If not, would adding a block toolbar button for toggling be a usable workaround?

@ZebulanStanphill ZebulanStanphill added Needs Accessibility Feedback Need input from accessibility New Block Suggestion for a new block labels Jul 15, 2020
@ZebulanStanphill
Copy link
Member

The term "accordion" should probably be added as a keyword so this block shows up when searching that term in the inserter.

packages/block-library/src/details/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/details/edit.js Outdated Show resolved Hide resolved
@enriquesanchez
Copy link
Contributor

Hi @georgeh, thanks for the ping! 👋

I think I can disable the spacebar toggle, but will keyboard users still be able to toggle the visible portion? If not, would adding a block toolbar button for toggling be a usable workaround?

Oh, this is a tricky one. Knowing that we want as much fidelity as possible between the editor and the front-end, I think users should be able to toggle expanded/closed in the editor as well.

An idea I can think of is to use the toggle arrow as the only means to expand/collapse the section in the editor? And ignore any spacebar/enter key presses while the summary text is focused?

Screen Shot 2020-07-15 at 16 22 37

Another thought is to mimic the functionality already found in the Navigation block, where submenus are hidden by default but are shown if the parent block receives focus. Could that maybe also work here? The user would not need to toggle anything, just focus on the block to see the expanded section.

Screen Shot 2020-07-15 at 16 27 04

I'll bring this up during the next accessibility team meeting this Friday so we can get more folks chiming in and providing feedback.


One thing I also noticed while testing this PR, is that the summary text is missing the toggle arrow that indicates this is an expandable/collapsable item.

Screen Shot 2020-07-15 at 16 28 41

I think having it there is important to let users know how they can interact with it.

@ZebulanStanphill
Copy link
Member

@enriquesanchez I think the reason the summary arrow-thing is missing in the editor is because the editor is using a div right now, rather than an actual summary element (which gets its arrow from default browser styles).

@georgeh
Copy link
Contributor Author

georgeh commented Jul 16, 2020

@ZebulanStanphill - you're right, I initially was using summary for the editor side but had to abandon it because of a11y concerns.

@enriquesanchez I'm going to try it with the summary and using the focus to show/hide the hidden part, thanks for the pointer!

@enriquesanchez
Copy link
Contributor

Hi @georgeh 👋

As promised, we discussed this PR in the accessibility team chat, and left our feedback on the issue.

@georgeh
Copy link
Contributor Author

georgeh commented Jul 23, 2020

Thanks for the pointers @enriquesanchez, I was hoping that this would be a quick way to get a basic accordion knocked out but now see that <details> isn't a good fit for that.

I still think there's value in having a <details> block so I'm going to keep working on working on improving the keyboard accessibility for this.

@enriquesanchez
Copy link
Contributor

Glad to hear that, @georgeh!

Please let us know when/if we can help test your PR.

packages/block-library/src/details/block.json Outdated Show resolved Hide resolved
packages/block-library/src/details/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/details/edit.js Outdated Show resolved Hide resolved
@georgeh
Copy link
Contributor Author

georgeh commented Jul 30, 2020

@ZebulanStanphill I pushed up a version that opens/closes entirely based on the block selection. What I like about this approach is that it's simple. What I dislike is that there's no way to see how a page looks with multiple details open (e.g. "what does this look like when everything is pushed down the page").

Also, when you use the keyboard to move into the details from the bottom, I would want the last hidden block to be selected instead of the summary, so I'm going to try to figure out how to tell when that happens. If you want to try what I'm talking about, add a details, then add some InnerBlocks to it, then add another block after the details block. That last block, when focused, will close the details block. Now press "up" on the keyboard and it takes you to the summary part of details, not the last InnerBlocks. That feels weird to me.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

@ZebulanStanphill I pushed up a version that opens/closes entirely based on the block selection. What I like about this approach is that it's simple. What I dislike is that there's no way to see how a page looks with multiple details open (e.g. "what does this look like when everything is pushed down the page").

Yeah, I'm not sure if this is the right approach or not. The big issue here is that we need to be able to edit the text of what would normally be a clickable element, while also wanting to be able to use the clickable element in the same UI. I'm not entirely sure what the best approach is in this situation.

Also, when you use the keyboard to move into the details from the bottom, I would want the last hidden block to be selected instead of the summary, so I'm going to try to figure out how to tell when that happens. If you want to try what I'm talking about, add a details, then add some InnerBlocks to it, then add another block after the details block. That last block, when focused, will close the details block. Now press "up" on the keyboard and it takes you to the summary part of details, not the last InnerBlocks. That feels weird to me.

I think from an accessibility perspective, the current behavior makes the most sense, but I'm not certain. It would be good to get some accessibility team feedback here.

packages/block-library/src/details/edit.js Show resolved Hide resolved
packages/block-library/src/details/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/details/edit.js Show resolved Hide resolved
packages/block-library/src/details/editor.scss Outdated Show resolved Hide resolved
packages/block-library/src/details/index.js Outdated Show resolved Hide resolved
packages/block-library/src/details/save.js Outdated Show resolved Hide resolved
packages/block-library/src/details/editor.scss Outdated Show resolved Hide resolved
category,
edit,
save,
// details and summary are not translated because they are the HTML tags
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// details and summary are not translated because they are the HTML tags
// "details" and "summary" are not translated because they are the HTML tags.

Comment on lines 41 to 52
/* You may be wondering why we don't just use a <details> element here.
* The problem we are trying to solve is that a <summary> is basically a
* button, and when it has focus, it eats the space key.
*
* That's a problem if you want to use a <RichText> component inside your
* <summary>. Each time you press space, it toggles the rest of the
* <details>, and it doesn't even add a space to your <RichText>.
*
* Then there's the fact that the space exists for a11y reasons. If you
* catch the event and preventDefault() then you've remove the way for
* keyboard users to toggle the <details>.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Come to think of it, maybe we can use a details + summary element in the editor after all. Why not just catch the event and preventDefault() in the editor? Combine that with the current expand-on-focus behavior and nothing would change except we could use the actual HTML elements and avoid the CSS simulations, right? We might also have to set a different value for the HTML role attribute if using the RichText component doesn't already do that.

We already can't click-to-toggle the simulated details in the editor, so why not just go ahead and use a real details?

Copy link
Member

Choose a reason for hiding this comment

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

It's worth noting that a "browse mode" as proposed in #23328 would allow the user to interact with the details the same as they would on the front-end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started with summary and I ran into issues with RichText getting the space key event when I preventDefault(). Even if I stopped the space key event on summary, the event wasn't getting to RichText for some reason so mytextwouldruntogether.

I'll spend some more time with it though, in case I missed something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some progress on <summary> with @vcanales's help and now it works in Chrome and Safari. I'll see what I can do about Firefox

Copy link
Contributor Author

@georgeh georgeh Aug 5, 2020

Choose a reason for hiding this comment

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

@vcanales and I got details and summary to work in the edit view in Chrome and Safari by putting RichText as a child of summary. However, Firefox seems to have an issue with contenteditable inside summary that prevents it from seeing the space key. I've created a simple test on JSFiddle showing the issue. This is probably due to summary's role as Button.

I can't update the value in the Edit component because I don't know the current caret position, so the only other way forward I see is to add some way to trigger a keyboard event in RichText.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... this is a tough one. Pinging @ellatrix for technical help.

packages/block-library/src/details/block.json Outdated Show resolved Hide resolved
@paaljoachim
Copy link
Contributor

Btw here is the issue for the accordion block which is still in the design phase. #21584
As there seems to be some crossover between the Details Block and the Accordion Block.

@georgeh
Copy link
Contributor Author

georgeh commented Aug 3, 2020

@paaljoachim I thought that <details> would be a good way to address the need for an Accordion Block when I started this block, but @enriquesanchez and the a11y team pointed out why <details> isn't great at being an accordion. I think there's a need for both blocks.

@jasmussen
Copy link
Contributor

Thanks for the PR. I wanted to share some high level thoughts on this PR, and the idea of a "Details" block.

The thing that I like about keeping it just a simple details block is that it really is quite simple, but could conceivably be used by users in a number of creative ways. I picture one that is as minimal as can be, inherits theme colors, and just works well. It would make it easy for themes to embrace the block, and for patterns to use it in useful ways. With various global styles introducing visual features, even moreso.

Accordions, on the other hand, is a bigger undertaking than it might seem. I imagine it can be useful just like a details block, but in order to be an accordion worth including, it also comes with a great deal more complexity. It likely also needs to be more opinionated, in that it may need to come with icons or graphics, and certainly borders and a more elaborate behavior. Outside of the increase in scope, and especially until global styles are further ahead, this will likely make it harder for it to inherit theme styles and ensure contrast in arbitrary background colors (currently the only safe bet here is to use currentColor). This also limits the usage inside patterns.

It also opens many questions that a Details block does not:

  • Should it animate? Should the animation be toggleable? Off by default?
  • Should the action to open an accordion be attached to a heading? What heading level should it be? Should the heading level be smart according to adjacent headings, or an option with the heading checker?
  • What if you put several next to each other, should opening one close the others? Does that mean they need to be innerblocks?
  • Should one be open by default? What if others close when you open one, and you set two to open by default?

All of that is fixable. I can definitely see someone tackling an accordion block some time in the future, and I can personally see it being a delightful block in core.

My larger point with the above, though, is that there is a simplicity and utility to the Details block that doesn't easily translate to an Accordion block. For that reason, I don't think the two should necessarily be perceived as analogous. And I could definitely see both coexisting in core. I'd love to use the Details block for hiding spoilers, for example, but I wouldn't use an accordion for that.

</InspectorControls>
<details className={ className } open={ showInnerBlocks }>
<summary ref={ summaryRef }>
<RichText
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, could the tag name of rich text be summary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that initially but the summary was swallowing the space key event and so everywordwouldruntogether. Making RichText a child of summary fixed that issue in Chrome and Safari, but not in Firefox.

The other approach I took was to use divs and CSS to emulate details in the editor view, and that worked, but I would much rather use the native elements if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix Do you know if there is a way to tell RichText to insert a [space] at the current caret position? I'm going to poke around the components in @wordpress/rich-text and @wordpresss/block-editor to see if that would allow us to continue using <summary> in Firefox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wound up making a change to the RichText component that fixed it in Firefox and allowed us to use summary as our tagName

@paaljoachim
Copy link
Contributor

What are some usage cases for the Details block vs the Accordion block?
Would a user clearly see the difference between the two?

At present time I do not know what the difference is between the Details block and the Accordion block.
It seems with both one can click an arrow to open or hide the inner content.

@georgeh
Copy link
Contributor Author

georgeh commented Aug 11, 2020

This block is getting closer to ready, but I want to figure out if it's in the project's best interest to merge this or pass on it in favor of the accordion block.

A details block gives us native HTML disclosure widget with no client-side JavaScript. The biggest use case for this block is in FAQ pages, which are common enough to deserve consideration. It's also used for spoilers, and Github uses them for disclosure modals. A block makes the details element usable without raw HTML and easily reused in templates.

However, if we are adding an accordion block to core, there's some overlap in use cases. Having two blocks with very similar interactions in core may just serve to overwhelm and confuse people. The distinction between an accordion and details is tough to understand and communicate. The accordion will be more accessible and themeable than details, at the expense of some client-side JS.

So the question is do we ship this now or wait for an accordion? The accordion block will probably not use the details element, so I don't see a path for shipping this then deprecating when accordion is ready. The accordion will also need a lot of work to get something we will want to add in core.

It's also a question of how much Gutenberg's core block library should focus on making native HTML available to people versus making pre-assembled components.

I'm proud of this block and the technical hurdles we've overcome, but I'm more concerned with making sure the editor we ship is the best it can be for people who use it. I'd love to hear any more arguments for or against merging this that I've missed.

@ZebulanStanphill
Copy link
Member

I'm currently not sure whether we should ship this block or not; but if not, then I'd definitely recommend releasing it as a plugin. I'd certainly like to use it.

@ZebulanStanphill
Copy link
Member

The "Open by default" option is somewhat oddly aligned in the inspector. There aren't any other cases of controls being displayed in the inspector outside of an accordion.

image

Also, is there any reason we can't just put this control in the toolbar?

@paaljoachim
Copy link
Contributor

Perhaps we should call it a FAQ block.
Creating a plugin sounds like a good initial way to go. As the Accordion Block should cover the needs in core for a way to create a content heading and then to click the heading/arrow to open inner content. It would be nice to take things that have been learned here for the Details Block and if applicable bring it to the Accordion Block.

@jasmussen
Copy link
Contributor

Pushed the icon:

Screenshot 2020-08-12 at 17 42 22

By the way I see a double shadow inside that panel above 👆 — I think the compoment may be used wrong, and that we need to include a full collapsible panel, in order for it to behave as intended:

Screenshot 2020-08-12 at 17 44 22

You can call it "Settings", as there's only one.

@mapk
Copy link
Contributor

mapk commented Aug 12, 2020

I've been working over on the Accordion block side of things and thought I'd drop in here as well. 👋

The Details block (I prefer "Collapsible block") is so good! I love that it doesn't use any JS and is very simplistic as noted above.

But from a user perspective, it basically functions the same as the Accordion block. It's a line of text with an arrow that toggles to reveal other content. Styling alone shouldn't be the only visual differentiator between the two blocks. I imagine we can style the Details block just like we do the Accordion block without a whole lot of effort.

I tend to agree with @georgeh's thoughts here:

Having two blocks with very similar interactions in core may just serve to overwhelm and confuse people. The distinction between an accordion and details is tough to understand and communicate.

Both blocks really perform the same function but using different semantics. It's like having a "Bold" option that uses <b> and one that uses <strong>. I don't think we need both. The Details block is much further along and I'd be happy to depend on it for this functionality.

@ZebulanStanphill
Copy link
Member

I think we should avoid adding any styles to a Details block ourselves, except perhaps in the opt-in theme.scss styles. The more styles we add, the harder it is for themes to style the block.

In the case of an Accordion block, we're forced to have some styles since there are no native browser styles, but even then I think we should limit them to the bare minimum to make the block functional/accessible.

Perhaps to the average user, there's no obvious difference between a Details and Accordion block. But the Details block simply cannot be used as a truly accessible accordion. If we're only going to have one of the two, it must be the Accordion, because it will ultimately be more flexible in the long run.

Even if this PR doesn't get merged, I think the included fix (or something similar) to allow RichText to use the summary element should be merged separately, so 3rd party plugins (like the one @dingo-d shared) can implement a Details block properly.

@mapk
Copy link
Contributor

mapk commented Aug 12, 2020

I think we should avoid adding any styles to a Details block ourselves, except perhaps in the opt-in theme.scss styles. The more styles we add, the harder it is for themes to style the block.

I totally agree here, @ZebulanStanphill. My concern is that the default styling for the Accordion block is nearly identical to the Details block. It is without styling for the most part to make it easy for themes to style on their own. I was planning to add style variations which give the Accordion block borders, more prominent headings, etc. and even provide some block settings that allow further customizations with color.

@jasmussen
Copy link
Contributor

An FAQ can be created with a good accordion block. The spoiler use case can be filled if the accordion is simple enough. It also seems most of the agreement on this thread is that simple is a key value for the Details block.

In that vein, if the accordion block can be built in a truly simple way (no JS), it would avoid the pitfalls pondered previously. Presumably the theme integration can be solved with proper global styles integration.

If we were to go that route, we want to make sure to addres @carolinan's feedback here, and @dingo-d (also to Zeb's point) should feel free to take inspiration from the GPL work that transpired in this PR.

@ZebulanStanphill
Copy link
Member

Yeah, <details> should definitely be usable as the root element of a block, regardless of whether this PR goes through or not.

if the accordion block can be built in a truly simple way (no JS)

I'm pretty sure it's impossible to build an accessible accordion without JS. You need a <button> to expand/collapse it, and that's going to have to, at minimum, add/remove a CSS class somewhere. That said, the required JS may be so simple that we could just inline it as an onclick HTML attribute. I'm not certain of the other a11y behavioral requirements, but you definitely need at least a little JS to make it even open/close in the first place.

To reiterate, a <details> cannot be used as an accordion if you wish to use a heading as the summary, because the <summary> (essentially acting as a button) eats semantics. This is why accessible accordions put their <button> inside their <h1>-<h6> element if they use a heading, rather than vice-versa.

@georgeh georgeh mentioned this pull request Aug 13, 2020
5 tasks
@georgeh
Copy link
Contributor Author

georgeh commented Aug 14, 2020

I created #24558 with the <summary> fix for RichText

@paaljoachim
Copy link
Contributor

How is the details block coming along?
I just added a comment for the Open Floor to tomorrows Core Editor Meeting.

@jasmussen
Copy link
Contributor

I would say that given the overall excitement around the accordion block, this one is indefinitely shelved.

@georgeh
Copy link
Contributor Author

georgeh commented Oct 6, 2020

@paaljoachim To second what @jasmussen said, we are not looking to get this into core since it's not a great replacement for an actual full accordion. However we did expose a bug with using the <summary> element for a RichText component, and I hope #24558 gets merged so that plugins provide <details> blocks with native markup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants