Details and accordion update based on Seven Style Guide

I was about to say this could share some styling with #2113931: File Field design update: Upload field. but the subtle inset styling from the design does not seem to be implemented there (yet?)

This looks awesome! The status report uses detail so we'll need a separate issue to clean that up. For example:

I notice the rounded corners, and the title color.

+++ b/core/themes/seven/css/components/details.css
@@ -0,0 +1,35 @@
+  border-radius: 3px;
+  -moz-border-radius: 3px;
+  -webkit-border-radius: 3px;

I don't think prefixes are necessary. If we do use them, shouldn't border-radius be at bottom of list?

Trying to add related issue #2857662: Update Status Report to account for Details and Accordion styling bringing fixed

+++ b/core/themes/seven/css/base/elements.css
@@ -167,8 +167,5 @@ details summary {
\ No newline at end of file

Needs a newline at end

Other than that, this looks good to me. I've tested on Simplytest with #2857662: Update Status Report to account for Details and Accordion styling bringing fixed and everything looks great!

Updated patch which removes some coding standard issues of #12 & also added interdiff.

(Updating commit credit)

+++ b/core/themes/seven/css/components/details.css
@@ -0,0 +1,32 @@
+/* @todo Regression: The summary of uncollapsible details are no longer
+     vertically aligned with the .details-wrapper in browsers without native
+     details support. */

We should still take care of the open @todo's

@AaronChristian if the regression is introduced in this patch, we should fix it. If not, we should file another bug report for that, as long as it can be worked without committing this patch first. If we just add a todo comment, it is highly unlikely that it gets fixed.

1. +++ b/core/themes/seven/css/components/details.css
   @@ -0,0 +1,32 @@
   +details {
   ...
   +details summary:hover,
   

   Let's create classes for details and details__summary so it is possible to use these styles for other types of elements as well. This also allows us to write better CSS.

2. +++ b/core/themes/seven/css/components/details.css
   @@ -0,0 +1,32 @@
   +details > .details-wrapper {
   

   Why do we have to use > here? We should document it with an inline comment if it is needed.

3. +++ b/core/themes/seven/css/components/details.css
   @@ -0,0 +1,32 @@
   +/* @todo Regression: The summary of uncollapsible details are no longer
   +     vertically aligned with the .details-wrapper in browsers without native
   +     details support. */
   

   If we have created issue for this, let's remove the @todo since it is reduntant. Please reference the issue to this issue.

4. +++ b/core/themes/seven/css/components/entity-meta.css
   @@ -51,9 +56,9 @@
    .entity-meta details > summary {
   ...
   +  padding: 0.95em 1.45em;
   

   I think this padding should be applied more globally for details summary elements because currently summary elements that are not inside .entity-meta look a bit awkward..

@AaronChristian I added the classes and rewrote the CSS accordingly. This allows us to do a few clean ups which I think is nice. What do you think about this?

That's ok for me too. Both in the status page and in other places where details appear, like in node creation. Here are some screenshots (Chrome & iOS, but tested Firefox too).

One detail. There's not enough contrast now on checkboxes and radios with the darker background:

Can be solved with this:

input[type="checkbox"], 
input[type="radio"] {
    box-sizing: border-box;
    padding: 0;
    box-shadow: 0 1px 1px 2px rgba(0,0,0,0.3);
    border-radius: 4px;
}
input[type="radio"] {
  border-radius: 50%;
}

Which produces:

One other thing. There'a a bug where margin is being added to the top of the page:

It's because—for some reason—html has the details class.

You can fix it with:

html.details {
  margin: 0;
}

Or perhaps:

body .details {
  margin-top: 1em;
  margin-bottom: 1em;
  background-color: #fcfcfa;
  border: 1px solid #bfbfbf;
  border-radius: 3px;
}

Both of them work. I don't know why the details class is being added to <html>, but there must be a good reason so I would not remove it.

After these I think this can go back to RTBC.

+++ b/core/themes/seven/templates/details.html.twig
@@ -0,0 +1,47 @@
+<details{{ attributes.addClass('details') }}>

I would suggest that we don't use a class name details. The class "details" is added to html because of how we have it configured. Is there any reason to add a class and not jsut theme the details element itself? I.e. do we envisage this css not applying to other details elements and do we want to apply it to things other than a details element?

Setting back to needs review to get opinions.

It is a bad practice to theme HTML tags directly since it forces that HTML tag always to have the same styles without proper variations or future support. If we would choose to style details HTML tag directly and we would have to introduce a new details element in the future, we would have to add a class for the details element, reset the default styles set for the tag and then apply any new styles.

There are few exceptions where not styling HTML tags directly would be very inconvenient (headings, paragraph etc) but even in these cases using classes would be more robust. I've themed elements directly in projects and in almost every case I've regretted it later because it makes refactoring or changing CSS more complicated than it should be.

In this case, I don't see any downsides in using classes. In my opinion, the only problem we have is to name this in a way it doesn't clash with the HTML element class.

I postponed #2279049: Add a CSS animation to expanding of details elements on this one since it is applying styles that should belong to the component we are creating here.

@lauriii totally happy to defer to your decision here - the main point of #37 was to point out why html was having the details class added. Not targeting HTML tags directly makes sense. So now we have to come up with a good class name...

I would go with accordion, because that describes what it is.

I think details is the best name for the component. Why don't we just prefix details with something? For example, something like .seven-details, or .drupal-details would work. I know it's a weird pattern but I think it's also weird if we call it to something else than what it is. We could also add a @todo about renaming it back to .details after we've managed to fix the polyfill (with a link to an issue where that would be done).

.fieldset sounds like <fieldset>, which is a completely different thing semantically. So <details class="fieldset"> would be a bit mixed up IMO.

Accordion usually describes a group of (related?) collapsible elements, often having constraints such as only allowing one open at a time, or requiring at least one to be open. One details element on its own doesn't sound like an accordion.

.accordion doesn't sound match any HTML5 element name, like fieldset does, but the WAI-ARIA Authoring Practices 1.1 working draft uses the term accordion to refer to a stacked set of collapsible groups.

Why not generalize Seven's .entity-meta details {} as .accordion details {}? The style is only used on node forms just now, but it could be used elsewhere later on. The block placement UI had a similar looking accordion sidebar during 8.x-dev, before it was changed to use a dialog.

I'm fine with the panel. However, I still think we could revert to #43 to avoid bikeshed about the naming :)

Core already uses "panel" class for those admin blocks at /admin/config (see: core/modules/system/templates/admin-block.html.twig), it would make it even more confusing. ;)

Rerolled to 8.6.x and changed the classname from details to seven-details.

Just realized that this does make the modules page look a bit odd:

@vaplas I think the dark grey used on the rows looks a bit strange when placed on top of the other grey. Also between the rows, there's white which doesn't match with the details background color.

Here's another screenshot demonstrating all different shades of grey we have there:

This was mentioned in the #ux Slack channel today. Could do with checking we still have focus states indicated clearly. Cross browser testing is important there, because default browser focus styles vary.

This should address my feedback on the modules page.

Here's still a screenshot of the modules page after.

@lauriii do you have any idea why I can't install that patch on SimplyTest.me - that's generally how I quickly whip up a demo to test.

Alternatively, even posting the new HTML as a sample might be sufficient to see if it is following best practices.

Review of patch #55 follows. It introduces a regression, and I figured out why. The new details.html.twig template in Seven breaks the collapse.js polyfill.

Things work fine in latest desktop versions of Chrome, Opera, Firefox. I didn't test Safari.

However in IE11 and Edge, the details/summary behaviour stopped working:

• After loading a page, all details elements were open.
• Clicking the summary did NOT cause the details to open/close correctly. It remained open.
• The <details open> attribute did toggle correctly, as did the aria properties from details-aria.js
• The disclosure triangle icon toggled correctly between horizontal and down directions.

I reckoned it could be something to do with the collapse.js polyfill, which is used by IE11 and Edge. Chrome, Opera, and Safari have supported details/summary since long before D8.0.0 was released, and Firefox supported it from v49 onwards.

Before Firefox 49, details/summary was experimental, and could be enabled with the dom.details_element.enabled preference. Old Firefox releases are still available (very cool), and sure enough, patch #55 causes a regression in Firefox 48, but enabling the experimental details/summary report made the problem go away.

After lots of summary toggling and rooting around in dev tools, I found the problem. We have this rule in Stable theme's CSS, which controls whether the open details when using the polyfill:

.js details:not([open]) .details-wrapper {
    display: none;
}

The new .seven-details__wrapper in the new template doesn't match this. We can fix the regression by adding this style rule:

.js details:not([open]) .seven-details__wrapper {
  display: none;
}

It worked when I shoved this rule into browser dev tools. I'm pleased I caught this, I feel like a proper front-end developer today :-)

@lauriii - can you update the patch, and I'll retest?

@mgifford - maybe the simplytest.me failure you're experiencing is #2946730: Spawned instances get "The website encountered an error" message? If you get the message "The website encountered an unexpected error. Please try again later", that appears to be a timing issue while simplytest.me does a composer install. I experienced it today, and when I tried again immediately afterwards, I managed to install Drupal successfully.

Forgot, I had another review point for patch #55:

diff --git a/core/themes/seven/css/components/details.css b/core/themes/seven/css/components/details.css

+.seven-details__summary:hover,
+.seven-details__summary:focus,
+.seven-details[open] > .seven-details__summary {
+  color: #004f80;
+}

I don't understand why we change the summary text colour when the details are open, but are not currently being interacted with hover or focus. That seems to diminish the usefulness of changing the colour for the hover and focus styles. The open details are already very clear, just from showing the content, and the disclosure triangle icon helps this. I suggest removing the .seven-details[open] > .seven-details__summary selector here, unless there's a special reason I've missed?

It's not really an accessibility issue though, because we're not relying on colour to tell the states apart.

Patch #55:

What's the text-shadow for here? It's very hard to notice. The dark text colour has very good contrast against the background, so I don't understand what this text-shadow is for. It's in all the patches here, but isn't explained.

+.seven-details__summary {
+  cursor: pointer;
+  text-shadow: 0 1px 0 white;
+  color: #0074bd;
+}

Thank you for the feedback! 😻It seems like there's some other styles attached to that element as well. Even though this is not the cleanest approach for solving this, I think this would be the least likely to break something.

@andrewmacpherson those design decision has been made a long time ago so it might be difficult to track down the specific reasons for those decisions.

Removing some old tags

RTBC:
The patch in #61 fixes the issues I found in #58 with the collapse.js polyfill.

I tested in IE11 (win 10), Edge (Win 10), Opera (linux, mac, win), Chrome (mac, win), Safari (mac), Safari (iOS), Chrome (Android).
Open closed box size, and disclosure triangle icon all work right.

Needs follow-up:
I noticed inconsistency with focus styles - sometimes there was a focus indicator, sometimes not. In some browsers (chrome + opera on mac) the summary element got an underline on when it was focused, but in others (safari on mac?) it didn't. I noticed this halfway though the long list of browsers, and didn't go back to check them all over again ;-)

We need a clear visual focus style for the summary element. In the case where the focused summary doesn't get an underline, the only indication is a slight colour change, which is a failure of WCAG 1.4.1 Use of Color (level A). Since the colour change on focus is so slight, it's also a failure of WCAG 2.4.7 Focus Visible and 1.4.11 Non-text contrast (both level AA).

Now, Proposal: A Style Guide for Seven doesn't actually have a focus style for the summary element. We definitely need to make summary focus visible in all browsers, so lets investigate an fix that in a follow-up.

I filed the follow-up at #2980304: Seven theme's details/summary focus style is broken/missing in some browsers.

Looks like the followup was opened. Also @lauriii sent it for a retest.

Reviewed for commit. There are two @todo comments, the second one does not even sounds actionable. Since neither has an issue link, completing them blocks this issue. If there are issues and they are referenced in the patch (and briefly explained) then this would be good to go.

Good catch! 🕵️‍♂️ I improved the comments and linked them to follow-up issues.

#67 has been done as requested.

Thanks! I wanted to go check what does this patch do, now that I was done with code review. This is what the issue summary suggests:

I did not expect exactly that given the lack of applying that font in core. However the issue summary says this issue is to restyle radio buttons and checkboxes. All changes that I managed to notice were that these triangles are now there. Coloring is still as before, radio buttons are still as before, etc:

@Gábor Hojtsy did you clear caches after applying the patch? It should indeed look like the issue summary.

I don't see any change to the radio button styling either, testing with a brand new simplytest.me installation. Checked on latest Firefox, Chrome, Opera, Edge, and IE11; not Safari though.

In earlier testing I didn't actually realize we we changing the radio buttons; I thought it was just the details/summary elements.

Checkboxes and radio buttons are/were in #2856459: Seven Style Guide: Checkboxes & Radio Buttons which is why I asked what should we be looking for here. Issue summary is definitely not up to date yet.

Updated the proposed resolution.

Thanks for the update and guidance on where to look. I removed the misleading prior screenshot from the summary. Verified the patch looks like what the issue summary says *now* :)

Committed 4e8c573 and pushed to 8.6.x. Thanks!

🚀🙌🎉