Vertical Tabs style update

Postponed due to missing final specs.

Ready to start working on a patch here. Thanks @archita-arora for all your work on the design!

Crediting Antonella too :)

There's a line separating the 2nd and 3rd tabs (Pages and Roles).
What colour is that? The image doesn't say.
The separator between tabs needs to have 3:1 contrast to satisfy WCAG 1.4.11 Non-text contrast.

In the second picture, what is "background-color: white smoke" for? Does it signify any information or state?

This is still in WIP, but you can check the progress status with this patch.

@ckrina, can you provide some indications about the :hover and/or :focus please?

@quiron Your progress is great, thanks! Suggesting to use pseudo element(s) for either the blue border or the gray ones to not get in conflict with the overall border styling see picture #1. Also, we have to care about accessibility, see #2.

#1

#2

Pushed this forward! I focused on fixing the overall styling and behavior of the vertical tabs and getting the basics right.
We still have to check the details provided by Figma (colors, paddings, font-size etc.), I applied some of them already on the way.

I asked for designs for focus and hover of the tabs from the design team. Tagging this issue accordingly.

I've updated the designs for the hover and focus styles on the Figma file, both for mobile and desktop variations. Is this doable with the existing markup? Here's an screenshot, but please use the Figma file as the reference since this can become outdated after some iterations.

I added the missing hover style, focus is already enabled like it's designed above. I put the vertical tabs "title" in blue color (on hover) because of consistency with the detail boxes which also have a blue title on hover. Let me know if that's wrong.

Fixing stylelint error.

Note: We don't have the accordion behavior on small devices (or any width based change) yet so setting it back to needs work.
The accordion designed for the vertical tabs on small screens isn't possible with the current markup I think.
We have to render the content with the details element to achieve this. Any other ideas?

Has #9 been addressed yet?

vertical_tabs test module added to Clarodist Tools!

I'll continue this.

1. Picking up on #9, the line separating inactive tabs (such as Pages and Roles in the example) does not provide sufficient contrast. The color of that border is rgba(216, 217, 224, 0.8), which on a white background displays as #ECEDF0. The resulting contrast is 1.17:1, so it does not meet the necessary contrast of 3:1. I'll keep this assigned to myself for the time being as there is additional work to do, but that color will need to be updated before this can be committed.
2. This may fall into the realm of it just being subjective, but I'm also concerned about the lack of some borders on inactive tabs. When viewing the vertical tabs menu in its entirety, this isn't as much of an issue, it's relatively easy to intuit the purpose of the tabs based on the surrounding context. However, when only portions of the tab menu are visible, the functionality of inactive tabs are less clear - it's less evident they can be clicked and can look like oddly placed text.
   

   

This patch adds the mobile designs. This required quite a bit of work for a number of reasons:

• The descriptive text that accompanies tabs (such as "Restricted to certain pages" on visibility tabs) is added by JS - not part of the render element and it is only added at widths of 640px and higher. Additional JS had to be added to include this at lower widths.
• On narrower screens, vertical tabs use details, but these do not have classes that indicate they are part of a vertical tabs group. A prerenderer had to be added so these elements could have BEM classes and avoid long selectors.
• The focus ring styling presented several z-index challenges
• Achieving the medium-width spec required changing the display of the tabs/content to flex (which made it possible to remove some float rules 👌 )

Setting to needs review, because it definitely should be looked at by someone else, but there are still some outstanding items:

1. The IE Details polyfill results in the tab descriptive text being placed outside the container.
   
2. The hover background color when hovering below the currently active tab covers the lower portion of the focus ring. Was not able to find a combination of rules that addressed this without introducing several other (more impactful) problems.
   
3. As mentioned in #30.1, The tab divider color still doesn't meet accessibility guidelines.

The comments in the code should hopefully make it clear where these changes were applied.

Sorry because the wrong color was used to define the divider.

I've just changed the color Light Grey to use a color with a ratio that complies with WCAG non-text contrast (1.4.3 Contrast (minimum)). Now the divider should be using #D4D4D8, which has a ratio of 1.478.

The panel border should keep using the current one.

Adding a patch that shows my progress.

Styles are worse than in #31, but the JS is closer to the end imho.

Missing/known problems with the JS:

1. Uncovered: tabShow() and tabHide() functions.
2. Focus is left on the programmatically-closed details, and not on the one that was just opened.

I feel that the design definition of the medium-sized vertical tabs should be reevaluated, see the attached video.

#31 has added a Claro-specific override for the vertical tabs JS. That wasn't present in patches prior to #31, but it isn't explained in that comment.

1. How does the claro JS behaviour differ from the core vertical tabs behaviour?
2. What problem (if any) is it trying to fix compared with the core vertical tabs JS? It may be better to file an issue for the core JS.
3. Has this had assistive tech testing?

My understanding was that Claro was intended as a visual refresh, which wouldn't involve changing any functionality.

@huzooka As you mentioned in slack channel, there is a problem about the medium width design in Figma. The video you recorded strictly follows the rule but as @ckrina mentioned interrupts the flow/editing experience with jumping layout and is therefore not possible. I would suggest setting the width to 40% / 60% of the two containers in medium width (768px -> 1200px) and let the content flow. With this solution, we kinda follow the rule and have a reliable layout. And you are still assigned, is that correct?

@fhaeberle, yes, I'm still working on this.

This is now functionally complete.

Todo:

1. Clean up code (PHP, JS, CSS)
2. Use properly named variables
3. Create screenshots

Edit: this patch addresses #32.

This patch fixes the CS of #38.

Addressing #38.1

This patch addresses #38.2, and:

• Fixes the hover shadow of the unselected vertical tab menu links.
• Fixes a (not-yet-reported) autofocus issue [summary, issue description and bug report needed]

Tomorrow I'll generate and check our usual screenshots. Hopefully that's the only remaining task here.

Minor improvement of the menu hover styles.

This is the last patch that uses flexbox, I have to replace that approach. Sadly, it is impossible to solve some situations with flexbox, like this one:

I think that this is ready for review!

#48 fixed, screenshots attached.

@huzooka Thanks for your work on this. You did an amazing job and this is a big push forward. I did a brief review of this and didn't found code improvements, but things we might want to consider.

1. The focus styles are a little off. On the left side (talking about LTR but same for RTL) of the menu item there is a gap / space indicating the menu item which is focused is also active but isn't in every case shown by the screenshot but the focus border has this little extra gap. Also, on the right side of the menu item, the focus style is covered/hidden by the pane. Also, the focus border overlays the active menu item on the picture (not that bad in my opinion). Is this intentional or should we fix that?

   

2. On mobile I found out that the active tab isn't collapsable. On the real details element however, this is possible (they are all collapsed by default). We might to change the behavior here to also collapse all items or let the users decide to collapse the item and set the first one active by default. Explicitly on mobile collapsing content in the height direction is very important and it's also about consistency that elements which looks the same, kinda works the same. I'm interested in your thoughts.

   

Changed z index for focus and active item, so now:
1) default
2) focus
3) active

Looks better imo

it will be hard to test patch, without patch...
Confucius

Re #51 (and #53 / #54):

1. This is the behavior that was specified in Figma. And I also think that the focus indication (the partial green retangle) should have the highest z-index level, especially for accessibility reasons. Because of these, I'd ignore comment #53 and the patch provided by #54.

2. Originally, with Seven or even with Stable theme, if the page was loaded with lower browser canvas width, vertical tabs remained simple details, even after the user swithed to higher browser width (withouth refreshing the current page). And vice versa, if the vertical tabs was rendered with a big-enough browser width, the component remained vertical tabs even after changing to a lower width.

   We agreed with @lauriii that we will change this dynamically, without forcing users to refresh the page.

   Because of this improvement, we always need an active (and only one active) vertical tab (that's content is expanded). Otherwise we won't know which tab content (details content) should be shown as active when the page is wide enough to show the vertical tab design instead of the accordion design.

How about we use similar focus effect as we use for tabs? This would mean that the focus border renders on top of other borders. This makes the design look much less busy and would remove some of the complexity of having to manage different layers

Good idea, I think your proposal will look much better. And if it also simplifies the implementation huge +1 to it. Let's find someone to work on the design for this.

Ok, here's the design updated for the focus for the Vertical Tabs. I've integrated your suggestion so we only change the border. Just take into account the blue border for the active one stays there to make more evident which is the active one. Let me know if there is any problem on the implementation.

Unassigning for now since I heard that @junaidmasoodi isn't working this at the moment.

Improved patch #50.

Screenshots have to be re-generated.

Re-rolled #65.

I'll remove the CSS class settings from claro.vertical-tabs library and hard-code our new CSS classes instead.

1. +++ b/css/src/components/details.css
   @@ -68,12 +74,12 @@
   +  word-wrap: break-word;
   
   +++ b/css/src/components/vertical-tabs.css
   @@ -1,126 +1,423 @@
   +  word-wrap: break-word;
   

   Should we add hyphens: auto as well?

2. +++ b/css/src/components/details.css
   @@ -163,7 +172,7 @@
   + * [1] Safari (at least version 12.1) cannot handle our details marker
   

   Why are we adding [1] here?

3. +++ b/css/src/components/details.css
   @@ -346,38 +366,80 @@
   +.claro-details__content--vertical-tabs-item .form-element {
   +  max-width: 100%;
    }
   

   Should we apply this rule more globally?

4. +++ b/css/src/components/fieldset.css
   @@ -52,7 +52,7 @@ _:-ms-fullscreen,
   -  margin-top: calc(var(--space-xs) / 2); /* 4px */
   +  margin-top: 0; /* IE11 and Edge do not collapse this margin. Ideally this would be 4px */
   
   +++ b/css/src/components/form--checkbox-radio--ie.css
   @@ -9,7 +9,7 @@
   -  background-image: url("data:image/svg+xml,%3Csvg width='12' height='10' viewBox='0 0 12 10'xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M4.18182 6.96572L1.97655 4.64855L1.79545 4.45826L1.61436 4.64855L0.818904 5.48437L0.654878 5.65672L0.818904 5.82907L4.00072 9.17235L4.18182 9.36263L4.36291 9.17235L11.1811 2.00817L11.3451 1.83582L11.1811 1.66347L10.3856 .827651L10.2045 .637365L10.0234 .82765L4.18182 6.96572Z' fill='white' /%3E%3C/svg%3E");
   +  background-image: url("data:image/svg+xml,%3Csvg width='12' height='10' viewBox='0 0 12 10' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M4.18182 6.96572L1.97655 4.64855L1.79545 4.45826L1.61436 4.64855L0.818904 5.48437L0.654878 5.65672L0.818904 5.82907L4.00072 9.17235L4.18182 9.36263L4.36291 9.17235L11.1811 2.00817L11.3451 1.83582L11.1811 1.66347L10.3856 .827651L10.2045 .637365L10.0234 .82765L4.18182 6.96572Z' fill='white' /%3E%3C/svg%3E");
   
   +++ b/css/src/components/form--checkbox-radio.css
   @@ -65,7 +65,7 @@
   -  background-image: url("data:image/svg+xml,%3Csvg width='12' height='10' viewBox='0 0 12 10'xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M4.18182 6.96572L1.97655 4.64855L1.79545 4.45826L1.61436 4.64855L0.818904 5.48437L0.654878 5.65672L0.818904 5.82907L4.00072 9.17235L4.18182 9.36263L4.36291 9.17235L11.1811 2.00817L11.3451 1.83582L11.1811 1.66347L10.3856 .827651L10.2045 .637365L10.0234 .82765L4.18182 6.96572Z' fill='white' /%3E%3C/svg%3E");
   +  background-image: url("data:image/svg+xml,%3Csvg width='12' height='10' viewBox='0 0 12 10' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M4.18182 6.96572L1.97655 4.64855L1.79545 4.45826L1.61436 4.64855L0.818904 5.48437L0.654878 5.65672L0.818904 5.82907L4.00072 9.17235L4.18182 9.36263L4.36291 9.17235L11.1811 2.00817L11.3451 1.83582L11.1811 1.66347L10.3856 .827651L10.2045 .637365L10.0234 .82765L4.18182 6.96572Z' fill='white' /%3E%3C/svg%3E");
   

   These seem like unrelated changes. Should we open separate issue to discuss these?

5. +++ b/css/src/components/vertical-tabs.css
   @@ -1,126 +1,423 @@
   +  .toolbar-tray-open:not(.toolbar-vertical) .vertical-tabs__menu,
   +  body:not(.toolbar-tray-open) .vertical-tabs__menu {
   +    display: block;
   +  }
   

   This doesn't support settings tray which works in similar way as the toolbar tray. I'm wondering if it would be better UX to just use details until we can show vertical tabs even when toolbar tray and settings tray are open. This should be also easier to implement.

6. +++ b/css/src/components/vertical-tabs.css
   @@ -1,126 +1,423 @@
   +.is-selected .vertical-tabs__menu-link {
   ...
   +.is-selected .vertical-tabs__menu-link:hover {
   ...
   +.is-selected .vertical-tabs__menu-link::before {
   ...
   +[dir=rtl] .is-selected .vertical-tabs__menu-link::before {
   ...
   +  .is-selected .vertical-tabs__menu-link {
   ...
   +  .is-selected .vertical-tabs__menu-link::before {
   

   It might be a good idea to target this with .vertical-tabs__menu-item.is-selected since .is-selected isn't specific to this component.

7. +++ b/css/src/components/vertical-tabs.css
   @@ -1,126 +1,423 @@
   +  top: -1px; /* Need to hide the pane wrapper clearfix's height */
   

   I'm not sure I understand how this is related to the clearfix 🤔

8. +++ b/js/claro.vertical-tabs.es6.js
   @@ -0,0 +1,393 @@
   + * Define vertical tabs functionality.
   

   Let's explain here why we are not using the core vertical tabs. We should also open follow-ups to move these changes to core and add them as @todo comments here.

I still have to report the issues that lead my to replace the whole vertical-tabs.js.

Re #72

1. Done.
2. Done. (Good question, without any answer. Removed.)
3. Selector replaced by .claro-details__content--vertical-tabs-item .form-element.
   I hope you thought so.
4. The margin issue is related, since the elements that we use here are details elements.

   I agree that the checkmark and the radio mark changes are not related. But they're causing XML parsing errors in IE11 (maybe in Edge as well?.. can't really remember), and - as I remember - we don't really open new issue for these kind of issues (neither for bigger ones).

   If you insist, I will open a new issue for every situation similar to this, but in that case, I'll ask everyone to do the same from now.

5. I'm in! @todo: We will need a new breakpoint value for the switch between the mobile and the medium layout.
6. Done.
7. The general 'clearfix' pseudo works in a way that it will be rendered after all floated element (because its clear: both|left|right property. (In general, if I say clearfiy, I mean a (pseudo)element that has clear: left, clear: right or clear: both.)

   Because of this, the content of the .vertical-tabs__item--processed will be one pixel higher than the vertical menu next to it.
   And that's why we need that negative pixel.

   The related clearfix pseudoelement follows the rule you highlighted here.

   Visual explanation:
   

8. Partially done. Bug reports, follow-ups (and their issue numbers) are still missing.

Needs review: only for testing the new patch.

Back to needs work because of #72.8.

#74.3 I was wondering if we could apply it to .form-element but the currently used selector is already better.

#74.4 I think it's fine to fix them as part of this issue. It was mainly that I wasn't sure if we were fixing something, or if it was an accidental change because I didn't find any mentions of them in the issue.

#74.5 I'm still not sure if we should actually switch between mobile / desktop views when settings tray and toolbar tray are being opened since this might lead into confusing UX. It makes sense that the layout changes when users browser width changes, however, opening setting tray or toolbar tray doesn't actually lead into browser width change, therefore users probably wouldn't expect the layout of the page to change.

Update: the new breakpoint will be 85(r)em.

Every bug and feature request are reported to core. Follow-up issue: #3081521: Refactor vertical tabs, remove vertical-tabs.js replacement when core issues are resolved.

Patch fixes everything from #72 (so it addresses #76).

Re #76:

For #74.3: It was there :). Removed.
For #74.5: 85rem is used for the breakpoint.

+1 For 85rem breakpoint.

diff --git a/js/claro.vertical-tabs.es6.js b/js/claro.vertical-tabs.es6.js
index 01029d8..5d805f8 100644
--- a/js/claro.vertical-tabs.es6.js
+++ b/js/claro.vertical-tabs.es6.js

+ * 2. Fixes accessibility bugs:
+ *    - The original Drupal.verticalTab object doesn't take care of the right
+ *      aria attributes. Whatever happens, every details summary element are
+ *      described with aria-expanded="false" and aria-pressed="false".
+ *    - The original Drupal.verticalTab object use a non-unique CSS id
+ *      '#active-vertical-tab' for the marker of the active menu tab. Sadly,
+ *      this will be very wrong on the filter format and editor configuration
+ *      form where multiple vertical tabs may appear
+ *      (/admin/config/content/formats/manage/basic_html).
+ *      Fearing the duplicated id, if the user changes the active tab, this
+ *      marker will be removed from the other vertical tab's active menu item
+ *      as well.
+ *    - Auto-focus bug: if the vertical tab is activated by pressing Enter on
+ *      the vertical tab menu link, the original Drupal.verticalTab object tries
+ *      to focus the first visible :input element in a vertical tab content. But
+ *      the implementation is wrong: on the 'Filter format and editor' form
+ *      (/admin/config/content/formats/manage/basic_html), if the user presses
+ *      the Enter key on the last vertical tabs element's menu link ('Filter
+ *      settings'), the focused element will be the first vertical tabs
+ *      ('CKEditor plugin settings') active input, and not the expected one.

Why is Claro fixing these in it's own JS file? These should be addressed in the core/misc library - have bug reports been filed? If not, please do so.

If Claro (in core) has theme-specific fixes like this, it means other themes won't benefit, and gives Claro a privileged status. Contrib admin themes would be at a disadvantage to Claro, or would have to re-implement the same fixes.

@andrewmacpherson We have already filed follow-ups to core issue queue (see link in #78 for more information). Since we are already overriding this JavaScript for other reasons, it seemed fine to implement these fixes here. I agree that we have to fix these in core as well, but it's much faster to make this type of fixes in Claro since we don't have to worry about BC implications.

I made some minor improvements to the documentation 😇

Re #82: +1
@lauriii, thanks for these!

In the process of reviewing, but manual testing indicated a reroll was needed so here's that.

The progress since I last looked at this issue is great.

1. The 85rem breakpoint is pretty rough on my 13" laptop display. It switches to accordion when my browser is at 94% of the full screen width something that is quite common. The viewport has to get much narrower before the accordion becomes the preferred experience. This may just be my subjective take on it but it would be good to have another look with smaller-screen desktop browser users in mind.
2. Probably everywhere, but found at on admin/structure/types/manage/article. There is a label element, presumably for screenreaders <label for="edit-additional-settings" class="form-item__label visually-hidden">Vertical Tabs</label>. This is not detected as the for attribute is looking for an ID that is not present on the page. The value it is looking fordoes appear in a div's data-drupal-selector attribute, an element that can't receive focus.

   Either the id should be added and the labeled element should be focusable. Or, if the label is unnecessary (which it could be if item #3 is addressed) the label can be removed.

3. There are existing wai-aria guidelines for tabpanels that are not implemented. More info here, and a nice example here.

   At the very least a core issue should be created (or this should be added to one of the already-created followups). It would be great to include this in Claro with a @todo to remove once it is supported in core, but @lauriii and/or an accessibility maintainer should probably determine if that is in scope for this issue. This would significantly improve the screenreader experience.

Thank you for the review @bnjmnm!

#85.1: Did you try vertical tabs both settings tray and settings tray open simultaneously? If we want to support that, I'm not sure we can support the desktop version of vertical tabs on narrower screens than 85em. 😕

#86

Did you try vertical tabs both settings tray and settings tray open simultaneously? If we want to support that, I'm not sure we can support the desktop version of vertical tabs on narrower screens than 85em

I see now there's a mention of this in #76 that I overlooked. I can reproduce this if I make Claro the default theme and add a front-end page with vertical tabs, but I'm curious if there a scenario on the admin end where the system tray (or any other right-side dialog on a page with vertical tabs) is available? It sounds like we're probably stuck with the 85em but I'd like to at least experience it on admin to see if an idea for a solution emerges.

Re #85:

#85.2: This isn't specific to Claro. Do we have an issue for this?.. (Let me check!)
#85.3: I think this would be a really big feature to solve here. Based on @andrewmacpherson's comment on #3081500-3: Accessibility bugs with vertical tabs, this might be solved by #2893640: Modernize ARIA usage, in line with ARIA 1.1 and the ARIA Authoring Practices guide. sometime in the (distant?..) future.

Patch #84 cannot be applied anymore.

Re-rolled #8 (which was a re-roll of #82).

RE #85.2:
I added this bug into the list of #3081500: Accessibility bugs with vertical tabs.

I'm afraid we cannot fix this (in a nice way) in Claro. I tried to remove the for attribute from the label, but I cannot do that without adding ugly hacks in the codebase.
Since VerticalTabs only have $element['#id'], this is the value that is added to the label as a for. But this is not a focusable element, and for example on the Node type edit for (where you noticed this), this id isn't added to the vertical tabs attributes array. Setting #label_for to NULL for vertical tabs isn't an option since it is added to the form_element_label renderable only if it is not empty. But the form_element's id will be always passed to the label theme function. And there, this happens.

So, the summary is:
#85.1: explained/answered in #86
#85.2: Global (core-scope) issue, and we cannot fix it nicely.
#85.3: Global (core-scope) feature, we won't fix this only for Claro.

Back to NR.

#90 provides a reroll. Accessibility review was done on #85.

1. +++ b/claro.theme
   @@ -262,6 +271,62 @@ function claro_element_info_alter(&$type) {
   +    array_unshift($vertical_tabs_pre_renders, '_claro_vertical_tabs_prerender');
   

   Should we document why we have to run this first?

2. +++ b/claro.theme
   @@ -262,6 +271,62 @@ function claro_element_info_alter(&$type) {
   +  if (
   +    isset($element['group']['#type']) &&
   +    $element['group']['#type'] === 'details' &&
   +    isset($element['group']['#groups']) &&
   +    is_array($element['group']['#groups'])
   +  ) {
   

   According to Drupal coding standards, control structure conditions shouldn't be wrapped to multiple lines.

3. +++ b/claro.theme
   @@ -262,6 +271,62 @@ function claro_element_info_alter(&$type) {
   +        $type = isset($element['group']['#groups'][$group_key][$child_key]['#type']) ? $element['group']['#groups'][$group_key][$child_key]['#type'] : FALSE;
   

   Nit: Maybe null would be a more accurate representation of no type?

4. +++ b/css/src/base/variables.css
   @@ -10,8 +10,8 @@
   +  --color-lightgray: #d4d4d8;
   +  --color-lightgray-o-80: rgba(212, 212, 218, 0.8);
   

   Why are we changing this? 👍 This was explained in #32.

5. +++ b/css/src/components/vertical-tabs.css
   @@ -1,123 +1,330 @@
   + * This wrapper div is added by JavaScript.
   

   Is there a technical reason why this needs to be added by JavaScript? If there is, we should explain that here.

6. +++ b/css/src/components/vertical-tabs.css
   @@ -1,123 +1,330 @@
   +  border-top: 1px solid transparent; /* Need to hide the pane wrapper clearfix's height */
   

   I know I asked about this before, but I don't really understand how this is related to the clearfix? AFAIK our clearfix shouldn't cause this behavior.

7. +++ b/css/src/components/vertical-tabs.css
   @@ -1,123 +1,330 @@
   +  width: var(--vertical-tabs-menu-width);
   

   According to the design system, this is the maximum width, not a static width. If this seems difficult to change, we can move this to a follow-up.

8. +++ b/css/src/components/vertical-tabs.css
   @@ -1,123 +1,330 @@
   +.vertical-tabs__menu-item::before,
   +.vertical-tabs__menu-item::after {
   ...
   +  z-index: 1;
   ...
   +.vertical-tabs__menu-item::after {
   ...
   +  z-index: 2;
   ...
   +  z-index: 4;
   ...
   +  z-index: 0;
   ...
   +  z-index: 3;
   ...
   +  z-index: 1;
   ...
   +  z-index: 1;
   ...
   +  z-index: 1;
   

   Can we document the design requirements this is matching so that it's easy to confirm how we expect this to work?

9. +++ b/css/src/components/vertical-tabs.css
   @@ -1,123 +1,330 @@
   +  .is-selected .vertical-tabs__menu-link::before {
   

   Should we add .vertical-tabs__menu-item in the beginning of this selector as well?

10. +++ b/js/claro.vertical-tabs.es6.js
    @@ -0,0 +1,447 @@
    +          $this
    +            .wrap($(Drupal.theme.verticalTabsWrapper()).addClass('js-vertical-tabs'))
    +            .before(tabList);
    ...
    +            tabList.append(verticalTab.item);
    ...
    +  Drupal.theme.verticalTabsWrapper = () =>
    +    '<div class="vertical-tabs clearfix"></div>';
    ...
    +  Drupal.theme.verticalTabListWrapper = () =>
    +    '<ul class="vertical-tabs__menu"></ul>';
    

    Any thoughts on trying to explicitly pass the content as a variable to the theme function rather than use .wrap and .append?

11. +++ b/js/claro.vertical-tabs.es6.js
    @@ -0,0 +1,447 @@
    +            // open the details manually for non-upporting browsers.
    

    Nit: s/upporting/supporting

12. +++ b/js/claro.vertical-tabs.es6.js
    @@ -0,0 +1,447 @@
    +            // tabFocus.data('verticalTab').details.attr('open', true);
    

    Could we link to the follow-up where this is supposed to be fixed? I also think the commented code can be removed.

13. +++ b/templates/navigation/details--vertical-tabs.html.twig
    @@ -0,0 +1,77 @@
    + * Theme override for a details element.
    

    Let's add mention that this is for the vertical tabs variation.

Re #93:

1. No, we shouldn't, because it isn't needed at all. 😁 (Refactored, we just add our prerender without tricks.)

2. Done.

3. Done.

5. I've no idea. That's an inheritance from the original .js. I don't really think that the current scope allows me writing a brand-new code. (A similar – rhetorical – question: why is the vertical tabs menu assembled and inserted by the .js? My answer is the same.)

6. See #74.

7. Short version: This is fine as it is right now.

   TLDR – Longer story: Well, let me be honest: We don't really follow the this part of the design system. Especially, even the originally used breakpoints where defined by @fhaeberle in #36 and since nobody made an objection, we followed those values initially.

   Since we (I mean you and me) agreed on that we won't change anything dynamically (based on the available width), it would be hard to detect where we should width the percentage-based menu with to the specific width. We cannot use max-width, because for the vertical tabs content element (that wraps the details and inserted by javascript) I'm unable to use max-margin.

   Initially, two months ago, I tried to solve this issue with a flex-based layout, but with that way we had usability issues (see #65) and ran into unresolvable cases like the one described in #48.

   So I returned to the float-based solution, and whit that, I just simply calculated that breakpoint width (cases: with vertical toolbar and without) where I was able to smoothly switch from the proportional menu width to the fixed one (Smoothly means without a 'jump' caused by a menu width change).

   Since you asked me (in #72.5) to remove the media queries that were used for implicitly checking the available container with (and I guess we don't want to bring those back), I don't see how I should (could) implement the rest of the original design. If we still need that, I'm happy to get the mid-sized layout back, but in that case I need an another breakpoint value where the VerticalTabs will switch from the mid-sized to the wide layout.

   (BTW on my laptop I only see the mobile design because of the 85rem that the team specified...)

8. Done.

9. I guess we should.

10. I want to remove the hard-coded markup.

    Besides that, Drupal.verticalTabs still needs to know which element is its own main tabs wrapper (for fixing #3081500: Accessibility bugs with vertical tabs.2 and maintaining the first and last modifier classes of the VerticalTabs menu items – example can be found on the text editor/ filter format settings form).

    The verticalTabListWrapper() is needed because the menu items are constructed and added in an each loop (see point 5 above).

    I don't really think that the current scope allows me writing a brand-new code.

11. (F)ixed 😉

12. Well, we don't need follow-up, it works. Comment reworded and clarified.

13. Added.

#96.5 I guess the follow-up question is then, why do you think it's important to document that the class is added by JavaScript? Does it matter from the CSS point of view?

#96.6 👍

#96.7 Let's open a follow-up to discuss the max-width and the breakpoint. Maybe we could improve the designs which would allow us to use a narrower breakpoint.

#96.10 👍

1. +++ b/css/src/components/vertical-tabs.css
   @@ -1,123 +1,348 @@
   +.vertical-tabs__menu-item::after {
   +  border-color: var(--color-white);
   +  z-index: 2;
    }
   ...
   +.vertical-tabs__menu-item.is-selected::after {
   +  content: '';
   

   Would it be possible to see a screenshot highlighting what is this supposed to fix? I was trying to fiddle this on my browser but I couldn't figure out what is this supposed to solve. Also, why are these added in two separate selectors?

2. +++ b/js/claro.vertical-tabs.es6.js
   @@ -160,9 +160,9 @@
   +            // The core/misc/collapse.js won't be ready at this time. We open
   +            // the details 'manually' for non-supporting browsers by the 'click'
   +            // and the 'keydown' event handlers attached to the details summary.
   

   This is still unclear. How is collapse.js not ready and where is the manual opening happening?

I guess this needs work.

Re #97:

#97.#96.5:
Why I think that it is important? Because when I started the implementation, it took me a while to find where the original .vertical-tabs__panes CSS class was coming from. I strongly believe that this comment will help others (contributors) as well: we won't forget that its only added if we have JavaScript interpretation. And I strongly believe that since we will have the related issues be fixed for core in the near future, these kind of comments are really useful (DX). Like to the ones you asked for in #93.8 imho.

#97.#96.7: Done: #3085787: Discuss the layouts and the layout breakpoints of VerticalTabs element .

#97.1: Yeah, it isn't easy to spot.

Why separate: I want to keep the first single-selector targeting to the merged one as close as possible. The second selector themes a state of the element.

#97.2:
While I was trying to explain this more deeply I noticed that this comment does not make sense anymore. (Especially after #38, where I refactored the way how the tabs are shown and hidden. Removed.

Wanted to review this, but this is above my CSS paygrade 😅😆

+++ b/js/claro.vertical-tabs.es6.js
@@ -0,0 +1,447 @@
+ * This file replaces core/misc/vertical-tabs.js to fix some bugs in the
+ * original implementation, as well as makes minor changes to enable Claro
...
+ * 1. Replaces hard-coded markup and adds 'js-' prefixed CSS classes for the
...
+ * 2. Fixes accessibility bugs (https://www.drupal.org/node/3081500):
...
+ * 3. Consistency between browsers (https://www.drupal.org/node/3081508):
...
+ * 4. Help fulfill our custom needs (https://www.drupal.org/node/3081519):

😵

#97.1: Yeah, it isn't easy to spot. [image]

🕵️‍♂️👏👏🤯 That’s eye for detail, wow! Beautifully demonstrated too.

LGMT! Thanks all for the great work!

Awesome work guys 🤟

Re. #85 and #88:

#85.2: The vertical tabs label is a bit annoying from a markup purist's view, but is actually completely harmless for accessibility. Inputs without accessible names are bad, but a label without an input isn't. It may as well be a span. I'd leave it in place. It's not Claro's problem to deal with.

#85.3: I would like to see our vertical tabs re-worked to follow the ARIA pattern. This isn't for Claro though; we should do it in core so all themes can benefit. Vertical tabs are mostly used in admin pages, but there could be times when it is used with the front-end theme. In particular, sites can be configured to use the default theme for node forms. In such a case, it would be quite strange if an admin user was exposed to two different versions of the vertical tabs interface.

This looks great! Committed and pushed! 🚀 See you on the follow-ups! 👋