Focus effect on vertical tabs conflicts with tab affordance

I like this! I will write a patch soon..

I worked on the border part of it. And its working fine for me. And screenshot is also attached for your reference.

This patch is changing the selected styling, not the hover styling, this is the selector we want to use:

.vertical-tab-button a:focus {

Also bear in mind that an underline is being added to links by default, so we will probably also need a text-decoration: none to prevent that.

The other problem with this implementation is that the border pushed the content inside a bit, so there is an indent, instead of being aligned with the rest of the tabs. We can reduce the padding-left styling by 4px to fix this.

Lastly, we need RTL styling as well. Thanks for this!

Looks good to me. @BrightBold are you working on the patch for the Bartik theme?

In Safari with the patch applied the outline style is still present, in addition to the focus styles added by the patch, so that needs to be overridden. (The patch looks great in FF & Chrome on Mac, but I also can't reproduce the original problem there.) See screenshot. I'll work on a revised patch.

OK, the problem was that Safari puts the focus style on the <li>, and we were only overriding outlines on the <a>. Attached is a patch with outlines removed for .vertical-tab-button.selected:focus. I left the outlines in place for the :active pseudoclass, thinking that is probably still useful for accessibility purposes. If we want to remove the outlines for :active also, we should apply the focus styles (or some other distinguishing style) to active as well.

I'll work on Bartik next.

Same change for Bartik. Same issue with :active.

#17 looks good to me. I'll check it on some Windows OSes.

#18 doesn't apply. Have a look at the patch, you'll see it.

The outline was already removed in another issue: #2368393: Fix focus effect on summary details

I got it. #18 doesn't apply after #17. Cool.

@esod re: #22 - right. #18 contained the Seven and Bartik changes. However, I forgot to port Kevin's style changes to Bartik, so one more patch is coming.

@idebr re: #21 - That issue affected the summary and details elements, but the markup for vertical tabs appears to be different. I was able to reproduce this issue today against head, whereas the issue you reference was committed a week ago.

OK, here it is again with the blue border stripe in Bartik.

Tested and Reviewed in Safari, Opera, IE 11, Chrome and Firefox on Windows.
Attached Screen shot for Safari:

Attached Screen shot for Chrome:

Attached Screen shot for Firfox:

Nice!

Hi, thanks for the work here. I found a few issues that would be good to fix

1. --- a/core/themes/bartik/css/components/vertical-tabs.component.css
   +++ b/core/themes/bartik/css/components/vertical-tabs.component.css
   

   I don't think we should add this styling to Bartik. The visuals of the two themes are very different. The design suggestion in this issue is for Seven

2. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -42,6 +42,9 @@
   +.vertical-tab-button.selected:focus {
   

   This should probably apply to the button all the time, not just when it has the .selected class

3. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
    .vertical-tab-button.selected a {
      border-bottom-color: #b3b2ad;
      color: #004f80;
   +  border-left: 4px solid #0074bd; /* LTR */
   +  outline: none;
   +  padding-left: 11px;
   +}
   +[dir=rtl] .vertical-tab-button.selected a {
   +  border-left: 0;
   

   This styling here isn't actually for focus, we need to use the :focus selector

4. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
   +li.vertical-tab-button.selected a:focus strong {
   

   It seems like this should be on the link element, not on the strong? Also, we should remove the li from the beginning of this selector

Ah ok, well nothing we can do about that here. We will have to fix it in #2408511: Rewrite vertical-tabs component inline with our CSS standards

@tkoleary Thanks, but also credit to maninder & b0unty — I just continued their work.

@LewisNyman Another question — if we remove the .selected from

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -42,6 +42,9 @@
+.vertical-tab-button.selected:focus {

then shouldn't we also remove it from

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -63,6 +66,19 @@
.vertical-tab-button.selected a {
   border-bottom-color: #b3b2ad;
   color: #004f80;
+  border-left: 4px solid #0074bd; /* LTR */
+  outline: none;
+  padding-left: 11px;
+}
+[dir=rtl] .vertical-tab-button.selected a {
+  border-left: 0;

as well? (So instead we'd have .vertical-tab-button:focus a.) Otherwise, there won't be any focus styles at all when the tab's not selected.

Right, but what I'm saying is don't we want the blue border stripe in the opposite case too, when it's :focus but not .selected? Seems like:

• :focus => blue vertical border on outside of tab;
• .selected => white background, box-shadow bottom border, and no vertical border on inside of tab.

There's no need for us to be involving .selected in any of this :focus-related code unless we need it for specificity, right?

Reroll attached. Merge was automatic.

1. index 444fd9d..a414c68 100644
   --- a/core/themes/bartik/css/components/vertical-tabs.component.css
   
   --- a/core/themes/bartik/css/components/vertical-tabs.component.css
   +++ b/core/themes/bartik/css/components/vertical-tabs.component.css
   

   @Lewis Nyman mentioned in #27.1: I don't think we should add this styling to Bartik. The visuals of the two themes are very different. The design suggestion in this issue is for Seven

2. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
   +  outline: none;
   

   outline: 0; is already inherit by a:focus in elements.css, so it can be removed here.

3. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
   +  padding-left: 11px;
   

   According to the Drupal CSS coding standards LTR specific styling should be appended with a /* LTR */ comment

4. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
   +  outline: none;
   

   Outline is not prone to LTR/RTL specific styling, so it can be removed here.

5. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
   +  text-decoration: none;
   

   This declaration is already inherit by .vertical-tab-button.selected a and can be removed here.

6. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +66,19 @@
   +  padding-right: 11px;
   

   When overriding the padding-right, the padding-left should be reset to its default (15px)

7. +++ b/core/themes/bartik/css/components/vertical-tabs.component.css
   @@ -9,3 +9,18 @@ ul.vertical-tabs-list {
   +  border-left: 4px solid #0071b3; /* LTR */
   

   Since .vertical-tab-button a already has a border-bottom, the blue border bleeds into the grey border:

   

   This can be fixed by using a different selector for the border or using a pseudo element (eg, :before or :after).

This patch fixes this behavior by using a different selector for the border.

@rafuel92 Thanks for working on this issue!

In your screenshot the border pushes the vertical tab button out of its container and the bottom border should be fully grey instead of starting with a bit of blue border. The points mentioned in #38.1 through #38.6 also still need to be addressed. Would you like to have a shot at that as well?

Ok, sure i'll check this today, thanks for your support.

Ok, those problems are now fixed.

Sorry, this is the correct patch file

@rafuel92 Thanks for your work, your changes are looking great! Just a few more things before this is ready to be committed:

1. The shadow below the last vertical tab is more pronounced after applying the patch, but this is not part of the suggested focus effect. For illustration, I made a before/after screencap:

   

2. +++ b/core/themes/bartik/css/components/vertical-tabs.component.css
   @@ -9,3 +9,18 @@ ul.vertical-tabs-list {
   +.vertical-tab-button:focus {
   +  outline: none;
   +}
   +.vertical-tab-button.selected a {
   +  border-bottom-color: #999;
   +  border-left: 4px solid #0071b3; /* LTR */
   +  outline: none;
   +  padding-left: 11px;
   +}
   +[dir=rtl] .vertical-tab-button.selected a {
   +  border-left: 0;
   +  border-right: 4px solid #0071b3;
   +  outline: none;
   +  padding-right: 11px;
   +}
   

   The suggested focus effect is for the Seven theme. These changes are for Bartik, but they are not part of the scope of this issue and can be removed.

3. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +67,19 @@
    .vertical-tab-button.selected a {
      border-bottom-color: #b3b2ad;
   ...
   +  border-bottom: none;
   

   When removing the border-bottom, the attribute for border-bottom-color can be removed.

4. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -63,6 +67,19 @@
   +  outline: none;
   +  text-decoration: none;
   

   These attribute are not specific for RTL, so should be moved to the LTR selector if applicable or removed if not applicable.

5. +++ b/index.php
   @@ -1,5 +1,7 @@
    <?php
   -
   +ini_set('display_errors',1);
   +ini_set('display_startup_errors',1);
   +error_reporting(-1);
   

   These changes should not be included in the patch, as they are unrelated to the scope of this issue.

Ok, thank you for your support, this patch should include those changes.

Sorry, this is the correct patch :)

The patch in #49 no longer applies after #2408511: Rewrite vertical-tabs component inline with our CSS standards was committed.

Rerolled patch on #49 for comments on #51.

This looks great!

Don't know why testbot reran and failed that after I saw it was greeen.

Needs a reroll.

git ac https://www.drupal.org/files/issues/focus_effect_on-2368373-53.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1318  100  1318    0     0   1129      0  0:00:01  0:00:01 --:--:--  1131
error: patch failed: core/themes/seven/css/components/vertical-tabs.css:59
error: core/themes/seven/css/components/vertical-tabs.css: patch does not apply

@rpayanm Thanks for the reroll! I did a manual test to confirm there were no changes since #53

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -60,8 +67,17 @@
+  padding-left: 11px; /* LTR */
...
+  padding-right: 15px;

Why the different values - I think the padding-right should be 11px.

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -60,8 +67,17 @@
+  padding-right: 15px;

Correct, this should padding-left: 15px; The RTL version should be padding-right: 11px;

1. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -60,8 +67,17 @@
   +  padding-left: 15px; /* LTR */
   

   This should be padding-left: 11px; instead to correct for the border of 4px

2. +++ b/core/themes/seven/css/components/vertical-tabs.css
   @@ -60,8 +67,17 @@
   +  padding-right: 11px;
   

   In addition to setting the padding-right: 11px; the padding-left should be reset to its default value: padding-left: 15px;

I fixed the remarks mentioned by @idebr

@pjbaert Thanks for picking this up! I missed something with the last review, sorry about that:

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -60,8 +67,18 @@
 .vertical-tabs__menu-item.is-selected a {
...
+[dir=rtl] .vertical-tabs__menu-item.selected a {

The RTL selector does not match to LTR selector. The correct state is .is-selected

Wow, didn't notice that either.
Fixed in this patch.

Quick review from chrome, looks OK to me.
Anyone for other browsers?

Change looks clean and it works! :)

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed dba9518 and pushed to 8.0.x. Thanks!

So a little bit funny, this solution doesn't actually solve the problem from this issue. There is still no accessible hover state on vertical tabs.

So reopening, don't know how that got past everyone :)

#2487704: Use underline as the focused state (not border left/bottom)

Also when aesthetically pleasing hover and focus should be the same.

The original issue description doesn't ever reference a hover state. I think the issue at https://www.drupal.org/node/2487704 would cover this functionality. Could we possibly close this to focus on the other issue, or is there something that is needed further to satisfy this issue?

The change made in this issue changes the .selected state when it should of only changed the focus state.

Lets add updated screenshots and update the issue summary to show how things look now after the patch was committed and why it doesn't resolve the issue.

In order to keep consistent with the Drupal style guide, shouldn't the focus state simply have an underlined title/link? This is mentioned on the issue with a similar ask here: https://www.drupal.org/node/2487704#comment-9917960

This would go against the suggestion in the screenshots above. Thoughts?

IMO the focus state should be made consistent with Drupal Style Guide and have an underlined link.

I think this is fixed - the tabs have focus and hover indicators, and the focus effect is consistent with the tabs