Details style update

I think that the Basic site settings page (/admin/config/system/site-information, the initial test page of #3023291: Fieldsets style update) is better for testing since it contains only details as root elements.

Test module: https://github.com/zolhorvath/details

Hi, I have updated the Details style.

Some more style updates. Some cases are still to be checked, but I guess it can already be reviewed.

Re #6:

1. Default details marker isn't removed in every browser (Chrome, Safari, IE11, Edge have that)
2. RTL icon is wrong
3. Focus outline does not follow Figma (radius missing)
4. Icon transform origin is not the expected

Stylelint:

$ yarn lint:css
yarn run v1.10.1
$ stylelint "css/src/**/*.css"

css/src/base/variables.css
 90:30  ✖  Unexpected unit   length-zero-no-unit

css/src/components/details.css
 24:32  ✖  Expected a leading zero   number-leading-zero

Updated

Fixed last #6 #8 patches + cleanup.

@kostyashupenko: from the outside the last 2 comments feels hostile. Usually if an issue is assigned to someone it's customary to ask and wait for an answer before reassigning to yourself. Someone assigning an issue to themselves means the person wants to take responsibility for moving the issue along. With the last two comments you ignored that and rewrote a big chunck of the patch. It doesn't matter if your code is better if you steamroll people along the way.

We all want to get this moving but not at all costs. Next time please make the effort and take the time and explain what should be changed if the issue is assigned to someone. It's not everyone that make the step to assign an issues to themselves, please be considerate of that and help accompany them.

Could you explain what the cleanup were please?

@nod_ ah, patch in #8 was added and status updated, so i just thought @kami-amiga forgot to unassign this issue as it was here for example
https://www.drupal.org/project/claro/issues/3024049#comment-12934803 - assign
https://www.drupal.org/project/claro/issues/3024049#comment-13040831 - unassign (2 months later)

I just did that to speed up development. I'm sorry if i made a mistake, i just had some free time to quickly improve last patch. it's not hostile at all.

Cleanup was mostly to re-define logic for setting arrow-icon and all related states (hover, focus, open, etc), and also removed [dir="ltr"] prefix. Everything is in interdiff.

Thank you for the details of what you changed.

#10 seems to be promising, I'll generate cross-browser screenshots.

Talked with @ckrina and there's a mistake in the current designs. The arrow should only reflect whether the details element is open or not. The arrow icon should remain the same regardless of focus and hover status. Updated the specs in the issue summary as well.

Update on #10:

1. Firefox, IE11 and Edge don't display the focus ring.
2. IE11 and Edge both display the basic menu-explanded.png (inherited from Classy, see @classy/base/collapse-processed.css) and display our custom marker only on hover. I strongly suggest unsetting Classy's collapse-processed.css and add a replacement by extending core/drupal.collapse.
3. Some colors are used without instantiating a variable, and .claro-details border is defined with px unit instead of rem.
4. I'd be happy to have a variable for the transition duration 0.12s (where is it defined?).

This patch:

- Alphabetize Details style variables.
- Provide variables for Details style properties.
- Use `rem` instead of `px`.
- Add new dependency to package.json postcss-calc, unfortunately IE11 don't render box-shadow when calc() is used https://caniuse.com/#feat=calc.
- The arrow icon is up to date.

For next steps, we will need to replace collapse-processed.css instead of unsetting it. The core/drupal.collapse will turn the summary into a clickable link (The default link color will override our summary color),

One question what do you mean by the focus ring ?

Firefox, IE11 and Edge don't display the focus ring.

Re #19:

The focus ring is the green outline around the summary (if it's focused), this:

The team just introduced a standard that changes things, sorry about that! https://drupal.org/node/3045783

also i found that details body should have own padding. we will not always have form item inside

Also it has sense to add modifier to details. with bigger paddings and left border for using in fieldsets and entity meta. https://www.drupal.org/project/claro/issues/3023291

Because as for me they are equal.

 --color-details-shadow: var(--color-accordion-shadow);
  --color-details-wrapper-padding-bottom-md: 1.75rem; /* space-m + space-s */

seems evil copypaste

1. moved post-css calc to devDependencies.
2. Fixed margins and alignment.
3. Cleaned variables.
4. Added claro-details--accordion modifier for usage in meta tabs. Cleaned previous implementation.
5. Added claro-details__wrapper--spaceless modifier for usage in system-modules list and in views UI. Cleaned previous implementation.

From my point of view we need to follow BEM guildelines as much as possible. and all what related to component should be in component.

This looks good to me. I did some functional/visual testing and reviewed the interdiff. Nothing jumping out at me that needs fixing from my perspective, but caveat that this is my first day devving on Claro =). I'm an optimist though, so marking as RTBC.

Just checked with extra large browser font size:

1. Details marker does not scale with the font size (it should to scale)
2. Details border scales with the font site (it shoudn't)
3. Focus outline scales with the font site (it shoudn't)

I think that #2 and #3 are the good solutions (A11Y), but they conflict with our current standard:

Elements that should not scale on any occasion should use px as a unit for dimensions. Examples of this include borders, border-radius, and outline.

gonna implement fixes for #28

Re #30:

Majors:

1. Firefox, IE11 and Edge don't display the green focus outline.
2. Safari displays the summary text below the marker.
3. The custom details markers is distorted on Firefox – (i think if the background-size property just repeats the define the values of the viewBox property, this will be fixed)
4. IE11 and Edge both display the basic menu-explanded.png and display our custom marker only on hover.
5. IE11 and Edge collapsed states' rotation is wrong, and the transform-origin is misaligned

See the attached screenshots & video.

Minors:

1. +++ b/css/src/base/variables.css
   @@ -14,6 +14,7 @@
   +  --color-green-focus: #26a769;
   

   I think that instead of adding a new value we should update the `--color-focus` variable.

2. +++ b/css/src/components/details.css
   @@ -5,22 +5,130 @@
   +.entity-meta .claro-details {
   ...
   +.entity-meta .claro-details__summary {
   ...
   +[dir="rtl"] .entity-meta .claro-details__summary {
   ...
   +.entity-meta .claro-details__wrapper {
   

   I think that it would be better to keep these in entity-meta.css

(no status change, NW is still valid)

…I said needs work…

@huzooka

I think that it would be better to keep these in entity-meta.css

on entity meta level we can only add something related to geometry or positioning of Details component. But not to component itself. Here we have modifier of component. So better to keep all related to component in one place.

In ideal case we need to preprocess that details in entity meta. and add modifier class.

fighting with details summary and its polyfill, will add patch tomorrow morning.

content: url(data:image/svg) and background-image: url(data:image/svg) are so buggy things)

Re #35:

We can add the modifier in a form preprocess (we inherited the node form alter from Seven...):

use Drupal\Core\Entity\ContentEntityFormInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element;

(…)
function claro_form_alter(&$form, FormStateInterface $form_state) {
  if (
    $form_state->getFormObject() instanceof ContentEntityFormInterface
    && isset($form['advanced'])
  ) {
    foreach (Element::children($form) as $form_key) {
      $item = &$form[$form_key];

      if (
        (isset($item['#group']) && $item['#group'] === 'advanced')
        && (isset($item['#type']) && $item['#type'] === 'details')
      ) {
        $item['#attributes']['class'][] = 'claro-details--sidebar';
      }
    }
  }
}

In this case, we would have a modifier, adn its' modifier styles can get into our details.css.

Just added patch without preprocess for modifier (Re #37)

Few notes about FF, IE, Edge: It looks like focus is work "strange" in these browsers -> focus is available only on "Tab" key and "tap" touch, so if you will click on it - you can't see focus. Not sure how to fix this issue and should we actually?

Also in IE and Edge we have different structure of details/summary tags, since related to modernizr test and core/misc/collapse.js
So if Modernizr.details is true, we have (Chrome, FF for ex.):

<details class="js-form-wrapper form-wrapper claro-details" data-drupal-selector="edit-menu" id="edit-menu">
  <summary role="button" aria-controls="edit-menu" aria-expanded="false" aria-pressed="false" class="claro-details__summary">
    ...
  </summary>
</details>

if false, then (IE, Edge):

<details class="js-form-wrapper form-wrapper claro-details collapse-processed" data-drupal-selector="edit-menu" id="edit-menu">
  <summary role="button" aria-controls="edit-menu" aria-expanded="false" aria-pressed="false" class="claro-details__summary">
    <a class="details-title" href="#edit-menu">
      ...
    </a>
  </summary>
</details>

So in case of IE/Edge we have additional link inside summary to make it work (expected thing). Focus is not work for summary there, it works only with link itself, it means with the current styles impossible to see focus green border and blue colours on hover.

Question is should we have a "complete" restyle in case of IE/Edge failback? It's possible to do that with the help of "collapse-processed" class which appears only when link inside. So we may have smth like

.claro-details.collapse-processed .claro-details__summary {
  box-shadow: none;
  padding: 0;
}

.claro-details.collapse-processed .claro-details__summary::before {
  display: none;
}

.claro-details.collapse-processed .details-title {
  ...re-init box-shadow, padding, backgrounds, etc...
}

.claro-details.collapse-processed .details-title::before {
  ...re-init arrow-icon...
}

So it looks like "duplicated" code, but it's provided by core i'd say...

Or we can actually add "text-decoration: underline" to a link on hover/focus for IE/Edge to highlight it somehow and it will be enough maybe ?

Sorry...

Re-rolled patch attached.

Let's descope browsers that have native support but don't provide focus effect for pointer devices; it seems like a pre-existing problem that we could try to solve in a separate issue. In those browsers, this problem exists even with native styles. However, I do think we should try to provide the correct experience for browsers relying on the polyfill, even if it leads into slightly more complex code or duplicated rules.

Forgot to assign this to myself...

Contacted @huzooka to make sure I wasn't disrupting anything by browser testing while this is assigned to him. He's working on the focus ring and IE11 issues mentioned in earlier comments, so I just looked for things that aren't those.

1. I know that in #22 top-padding was added to details since the first element won't always be a form item. In the cases where it is a form item, could the whitespace be reduced a bit with a rule like

   
   .claro-details__wrapper > .form-item {
       margin-top: 0;
   }

   ?

2. in IE11 high contrast mode, the blue disclosure triangle has very low contrast with the black background. Changing this to match the yellow color of the summary element while in high contrast mode will take care of this
3. The disclosure triangle alignment with the summary text doesn't match the Figma comps. It may not be possible to match it exactly since system fonts get the highest priority, but even getting it closer would be great.
   
4. RTL Looks fine
   

Addressing #32 and #45.

Working on screenshots.

Safari won't animate the marker, I'll add a followup with my partially-working shim as soon as I'm done with the screenshots.

Screenshots attached.

1. +++ b/css/src/components/details.css
   @@ -5,22 +5,223 @@
   +  padding: var(--space-m) var(--space-m) var(--space-m) var(--details-wrapper-padding-desktop-left);
   ...
   +  transition: background-color var(--transition-duration-details-background-color) ease-in-out, box-shadow var(--transition-duration-details-box-shadow) ease-in-out;
   

   The box-shadow transition doesn't work smoothly on Firefox. I found a blog post suggesting to use a pseudo element instead of animating box-shadow.

2. +++ b/css/src/components/details.css
   @@ -5,22 +5,223 @@
   +  padding: var(--space-m) var(--space-m) var(--space-m) var(--details-wrapper-padding-desktop-left);
   ...
   +  transition: background-color var(--transition-duration-details-background-color) ease-in-out, box-shadow var(--transition-duration-details-box-shadow) ease-in-out;
   ...
   +  padding: var(--space-l) var(--space-l) var(--space-l) var--details-wrapper-padding-desktop-left);
   ...
   +  transform: rotate(0);
   ...
   +  left: var(--space-s);
   ...
   +    transform: rotate(45deg);
   ...
   +  padding: var(--space-m) var(--space-m) var(--space-m) var(--details-wrapper-padding-desktop-left);
   

   We should document that these are for LTR.

3. +++ b/css/src/components/details.css
   @@ -5,22 +5,223 @@
   + * Even that it is possible to provide a javascript workaround for it (by
   + * adding/removing a helper class with javascript if the Details is
   

   Nit: s/javascript/JavaScript

4. +++ b/css/src/components/details.css
   @@ -5,22 +5,223 @@
   + * toggled), that hack will make rtl details worse than without the hack.
   

   Nit: s/rtl/RTL.

5. +++ b/css/src/components/details.css
   @@ -5,22 +5,223 @@
   +.claro-details__wrapper--spaceless,
   +.system-modules .claro-details__wrapper,
   +#views-ui-add-form details details .claro-details__wrapper {
   ...
   +  .claro-details__wrapper--spaceless,
   +  .system-modules .claro-details__wrapper,
   +  #views-ui-add-form details details .claro-details__wrapper {
   

   Could we add a comment explaining what these are for?

6. +++ b/css/src/components/details.css
   @@ -5,22 +5,223 @@
   +  text-decoration: none; /* override a:hover { text-decoration: underline; } at ../base/element.css */
   

   I don't think we should provide comments on what we are overriding. It is hard to keep these kinds of comments up to date because of the location we are referring to or the setting could change in the future.

7. +++ b/js/claro.details.es6.js
   @@ -0,0 +1,43 @@
   +  Drupal.behaviors.claroDetails = {
   +    attach(context) {
   +      $(context)
   +        .once('claroDetails')
   +        // Workaround for Firefox.
   +        // Firefox applies the focus state only for keyboard navigation.
   +        // We have to manually trigger focus to make the behavior coherent
   +        // across browsers.
   

   This should be documented according to our JavaScript coding standards. I think the existing comment could be moved there.

8. +++ b/js/claro.details.es6.js
   @@ -0,0 +1,43 @@
   +  Drupal.behaviors.claroDetailsToggleShim = {
   +    attach(context) {
   

   This behavior should be documented as well.

Addressing everything from #48.

Re #48.5:

1. I wasn't able to find any related usage of .claro-details__wrapper--spaceless wrapper, removed.
2. I think that .system-modules .claro-details__wrapper looks better without any mod. Since modules page is out of the scope of this issue, I removed that as well.
3. #views-ui-add-form details details .claro-details__wrapper is a legacy (and obsolete) selector (View UI add form's id was changed years ago and it operates with <fieldset>, not <details>), removed.

Re #48.7 and 8: I guess we should update our packages.json and yarn.lock to follow Drupal core changes?..

Anyway, this is ready for the next review.

Fresh screenshots attached.

1. IE11: Claro disclosure triangles are not showing up in the default state. When hovered over, they are visible
   
2. IE11: Focus ring is not visible on details.
3. IE11: The closed details triangle (currently only visible on hover) points up instead of to the right
4. IE11: RTL disclosure triangle (currently only visible on hover) points down whether details are open or closed.
5. This may be acceptable variation between browsers, but pointing out just in case. In Chrome/Safari, when opening/closing details with a mouse click, the element retains the focus ring after the click. In Firefox, the focus ring is only present while clicking. After the click it is no longer visible. Not sure about IE11 since focus rings aren't appearing at all.

6. +++ b/claro.libraries.yml
   @@ -196,3 +196,7 @@ drupal.shortcut:
   +details-focus:
   +  js:
   +    js/claro.details.js: {}
   

   If this library is specific to focus, perhaps the js filename could be focus-specific as well.

   Or, if the JS file could potentially be home to other details-related JS, could the library be renamed to something more general to make it clear this isn't exclusive to focus behavior?

7. +++ b/templates/details.html.twig
   +++ b/templates/details.html.twig
   @@ -24,6 +24,7 @@
   
   @@ -24,6 +24,7 @@
        {%
          set summary_classes = [
            'claro-details__summary',
   +        'js-claro-details-summary',
            required ? 'js-form-required',
            required ? 'form-required',
          ]
   

   I don't see the js-claro-details-summary class used anywhere

8. +++ b/js/claro.details.es6.js
   @@ -0,0 +1,59 @@
   +   * We have to manually trigger focus to make the behavior coherent
   

   Should "coherent" be "consistent" instead? (there are two instances of this in the file)

Re #52:

• I can't reproduce #52.1, #52.2, #52.3, #52.4, #52.5. Are you sure that you compiled the codebase (and emptied caches)?..
• #52.6: this is not focus specific and addresses details (fixes the inconsistencies that you highlighted in #52.5 AND adds a missing trigger for IE and Edge).
• #52.7: Fixed, thanks!
• #52.8: Fixed :)

We discussed #52.5 earlier and we should honor the default browser behavior on that. See #38 and #42.

I can also reproduce #52.2. I don't see the focus ring for details when tabbing to details element on IE 11. I could see it when clicking with a mouse but only sometimes. Maybe it's helpful that it works correctly to me with #47, but not with #50.

@huzooka apologies for occupying your time with the not-reproducible IE stuff in #53, not exactly sure why my Virtualbox IE11 was doing that.

#52.2 was still happening for both Lauri and I. This was addressed by removing a box-shadow rule set on the polyfill selector .collapse-processed > .claro-details__summary .details-title that didn't seem to be doing much other than overriding the default focus ring rule

I also noticed that in IE, details in modals did not get a focus ring when focused. A good place to see this happening is in views, doing something such as editing a header. This was addressed with a change that may be too all-encompassing for some tastes. I added an additional selector to the default claro focus ring rule.

-.page-wrapper *:focus {
+.page-wrapper *:focus,
+.ui-dialog *:focus {
   box-shadow: 0 0 0 2px var(--color-white), 0 0 0 5px var(--color-focus);
   outline: 2px dotted transparent;
   position: relative;

This takes care of it, but it's possible there will be a preference for something with less broad scope.

Also fixed a few nits I spotted while working on the above

+++ b/css/src/base/elements.css
@@ -188,7 +187,8 @@ img {
+.ui-dialog *:focus {
...
   position: relative;

I think adding this selector here is fine, but it seems like we accidentally left position: relative; which causes some problems in the dialogs. We could probably remove this here.

Fixed #56.

Reviewed the latest patch, I agree the position: relative; should be removed.

Confirmed in ie11 that the focused detail in a ui-dialog is now showing a focus state and works when tabbing onto it.

+++ b/css/src/components/details.css
@@ -5,22 +5,279 @@
+  color: var(--color-absolutezero);

This rule .claro-details[open] > .claro-details__summary:focus does not apply in ie11, probably the <summary> tag isn't recognized by that browser. The fallback to black looks good and its barely noticeable.

Marking RTBC, this is really good!

Re #57, #58:

Details of dialogs in High contrast mode needs some minor enhancements. This is what I see now when I focus a details on Edge:

Edit:

The rule claro-details[open] > .claro-details__summary:focus won't apply on ie11 or edge because these don't support <details>. We have to use the .details-title anchor element to support the states (see core/misc/collapse.es6.js).

Just a reroll, we still have to address #59.

I'll fix this.

Re #54 ... #60:

I toke back .collapse-processed .details-title enhancements (and hope that the actual additions will fix #52.2 AND #59).

Before (with the patch from #60):

With the current patch I see:

Please verify that this time it works as expected :)

Tested manually with Chrome, Safari, Firefox, IE 11 (normal + high contrast) and Edge (normal + high contrast) and this works consistently across all browsers except the missing animation on Safari which already has code comments explaining why. Thank you everyone who has helped on this. Committed and pushed!