The vertical tabs are "open" on the right so that the user understands that the active tab relates to the content to it's right. The default focus effect "closes" the opening and looks wrong.

Proposed that we add a focus effect to the left of the tab using border and remove the default outline.

Current:

current

Proposed:

new

CSS for above solution

.vertical-tab-button.selected a {
border-bottom-color: #b3b2ad;
color: #004f80;
+ border-left: 4px solid #0074bd;
+ outline: none;
}
CommentFileSizeAuthor
#69 Screen Shot 2015-04-13 at 18.17.32.png55.19 KBlauriii
#69 Screen Shot 2015-04-13 at 18.17.38.png55.05 KBlauriii
#68 ltr-chrome.png60.33 KBkoence
#68 rtl-chrome.png39.73 KBkoence
#67 2368373-67.patch1.32 KBpjbaert
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,407 pass(es). View
#65 2368373-65.patch1.31 KBpjbaert
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,964 pass(es). View
#63 2368373-63.patch1.29 KBrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,702 pass(es). View
#63 2368373-interdiff.txt791 bytesrpayanm
#59 2368373-59.patch1.29 KBrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,597 pass(es). View
#53 focus_effect_on-2368373-53.patch1.29 KBsushyl
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,474 pass(es). View
#49 focus_effect_on-2368373-49.patch1.31 KBrafuel92
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,160 pass(es). View
#47 focus_effect_on-2368373-47.patch1.31 KBrafuel92
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch focus_effect_on-2368373-47.patch. Unable to apply patch. See the log in the details link for more information. View
#46 2368373-45-after.gif14.45 KBidebr
#45 Focus-effect-on-vertical-tabs-2368373-45.patch2.29 KBrafuel92
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,765 pass(es). View
#43 Screenshot at mar 21 15-07-48.png41.16 KBrafuel92
#43 Focus-effect-on-vertical-tabs-2368373-43.patch.txt2.29 KBrafuel92
#39 Screenshot at mar 21 13-38-23.png34.74 KBrafuel92
#39 Focus-effect-on-vertical-tabs-2368373-39.patch1.94 KBrafuel92
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,758 pass(es). View
#38 2368373-36-after.png19.09 KBidebr
#36 Focus-effect-on-vertical-tabs-2368373-36.patch1.74 KBjedihe
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,260 pass(es). View
#28 Screen Shot 2015-01-19 at 14.06.33.png12.51 KBb0unty
#25 firefox-screenshot.PNG9.3 KBarunkumark
#25 chrome-screenshot.PNG8.88 KBarunkumark
#25 safari-screenshot.PNG7.09 KBarunkumark
#24 Screen Shot - Safari with Focus-effect-on-vertical-tabs-Bartik-2368373-24.patch.png67.96 KBBrightBold
#24 Focus-effect-on-vertical-tabs-2368373-24.patch1.69 KBBrightBold
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,011 pass(es). View
#18 Focus-effect-on-vertical-tabs-Bartik-2368373-18.patch1.4 KBBrightBold
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,969 pass(es). View
#17 Focus-effect-on-vertical-tabs-2368373-17.patch987 bytesBrightBold
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,204 pass(es). View
#16 Screen Shot - Safari with Focus-effect-on-vertical-tabs-2368373-12.patch.png39.85 KBBrightBold
#12 Focus-effect-on-vertical-tabs-2368373-12.patch787 bytesb0unty
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,986 pass(es). View
#10 Screen Shot 2015-01-15 at 10.42.43.png20.42 KBb0unty
#10 Screen Shot 2015-01-15 at 10.42.38.png21.93 KBb0unty
#10 Screen Shot 2015-01-15 at 10.45.02.png22.11 KBb0unty
#10 Screen Shot 2015-01-15 at 10.45.08.png19.04 KBb0unty
#10 Focus-effect-on-vertical-tabs-2368373-10.patch782 bytesb0unty
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,939 pass(es), 1 fail(s), and 0 exception(s). View
#8 vertical_tabs_2015-01-11.png7.29 KBManinders
#8 Focus-effect-on-vertical-tabs-2368373-7.patch503 bytesManinders
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,565 pass(es). View
Screen Shot 2014-11-03 at 12.43.44 PM.png67.63 KBtkoleary
Screen Shot 2014-11-03 at 10.00.59 AM.png57.45 KBtkoleary
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

The same solution should also be applied to Bartik theme

tkoleary’s picture

Issue summary: View changes
LewisNyman’s picture

Version: 8.0.0-beta2 » 8.0.x-dev
Status: Needs work » Active
Issue tags: +frontend, +CSS

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

LewisNyman’s picture

Issue tags: +Novice
Maninders’s picture

Assigned: Unassigned » Maninders
Maninders’s picture

Status: Active » Needs review
FileSize
503 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,565 pass(es). View
7.29 KB

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

LewisNyman’s picture

Assigned: Maninders » Unassigned
Status: Needs review » Needs work

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!

b0unty’s picture

FileSize
782 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,939 pass(es), 1 fail(s), and 0 exception(s). View
19.04 KB
22.11 KB
21.93 KB
20.42 KB

But if we use only the a:focus the border will disappear when you click the textfield for example is that intended or?

Anyways heres RTL styling and the padding fixed so content does not get pushed over, also removed the underline as per requested

RTL images :

LTR Images :

b0unty’s picture

Status: Needs work » Needs review

forgot to update status..

b0unty’s picture

FileSize
787 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,986 pass(es). View

Aand my vim config was bit wrecked so tabs werent using spaces.. heres a correct one.

The last submitted patch, 10: Focus-effect-on-vertical-tabs-2368373-10.patch, failed testing.

BrightBold’s picture

Assigned: Unassigned » BrightBold
esod’s picture

Status: Needs review » Reviewed & tested by the community

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

BrightBold’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +SprintWeekend2015
FileSize
39.85 KB

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.

BrightBold’s picture

FileSize
987 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,204 pass(es). View

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.

BrightBold’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,969 pass(es). View

Same change for Bartik. Same issue with :active.

esod’s picture

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

esod’s picture

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

idebr’s picture

Status: Needs review » Needs work

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

esod’s picture

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

BrightBold’s picture

@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.

BrightBold’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,011 pass(es). View
67.96 KB

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

arunkumark’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
7.09 KB
8.88 KB
9.3 KB

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

aspilicious’s picture

Nice!

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs work

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

b0unty’s picture

#27 your @your 4. point, it needs to be on the a:focus strong as the underline is stated like that selector for it is .vertical-tab-button.selected a:focus strong i added the li in front so no need for !important;

LewisNyman’s picture

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

b0unty’s picture

So we should just leave the underlining in then on this patch?

tkoleary’s picture

@BrightBold cool, Looks/works MUCH better now

BrightBold’s picture

@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.

b0unty’s picture

i think what lewisnyman meant is that when theres :focus selector the color of the border is #0074bd and when its not focus but just .selected #004f80

BrightBold’s picture

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?

b0unty’s picture

Assigned: BrightBold » Unassigned
Issue tags: +Needs reroll

Unassigned & addded needs reroll

jedihe’s picture

Issue tags: -Needs reroll
FileSize
1.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,260 pass(es). View

Reroll attached. Merge was automatic.

lauriii’s picture

Status: Needs work » Needs review
idebr’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
19.09 KB
  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).

rafuel92’s picture

FileSize
1.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,758 pass(es). View
34.74 KB

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

rafuel92’s picture

Status: Needs work » Needs review
idebr’s picture

Status: Needs review » Needs work

@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?

rafuel92’s picture

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

rafuel92’s picture

Ok, those problems are now fixed.

rafuel92’s picture

Status: Needs work » Needs review
rafuel92’s picture

FileSize
2.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,765 pass(es). View

Sorry, this is the correct patch file

idebr’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
14.45 KB

@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.

rafuel92’s picture

FileSize
1.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch focus_effect_on-2368373-47.patch. Unable to apply patch. See the log in the details link for more information. View

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

rafuel92’s picture

Status: Needs work » Needs review
rafuel92’s picture

FileSize
1.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,160 pass(es). View

Sorry, this is the correct patch :)

The last submitted patch, 47: focus_effect_on-2368373-47.patch, failed testing.

idebr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

sushyl’s picture

Assigned: Unassigned » sushyl
sushyl’s picture

Assigned: sushyl » Unassigned
Status: Needs work » Needs review
FileSize
1.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,474 pass(es). View

Rerolled patch on #49 for comments on #51.

kerby70’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

This looks great!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: focus_effect_on-2368373-53.patch, failed testing.

Status: Needs work » Needs review
kerby70’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,597 pass(es). View
idebr’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

idebr’s picture

+++ 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;

rpayanm’s picture

Status: Needs work » Needs review
FileSize
791 bytes
1.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,702 pass(es). View
idebr’s picture

Status: Needs review » Needs work
  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;

pjbaert’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,964 pass(es). View

I fixed the remarks mentioned by @idebr

idebr’s picture

Status: Needs review » Needs work

@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

pjbaert’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,407 pass(es). View

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

koence’s picture

FileSize
39.73 KB
60.33 KB

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

lauriii’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
55.05 KB
55.19 KB

Change looks clean and it works! :)

  • alexpott committed dba9518 on 8.0.x
    Issue #2368373 by rafuel92, BrightBold, b0unty, rpayanm, pjbaert,...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Bojhan’s picture

Status: Closed (fixed) » Active

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 :)

Bojhan’s picture

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

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

mark.labrecque’s picture

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?

LewisNyman’s picture

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

disasm’s picture

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.

mark.labrecque’s picture

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?

kfitz’s picture

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

tkoleary’s picture

@kfitz, @mark.labrecque

In order to keep consistent with the Drupal style guide, shouldn't the focus state simply have an underlined title/link?

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

While I appreciate the desire to maintain consistency, this would be consistency at the expense of good design.

The underlined link style of the horizontal tabs is appropriate to those precisely because they are horizontal and as the eye scans across them it stops at the one that is in focus. Here the tabs are vertical, the users eye is scanning across them vertically and therefore it makes sense to stop the eye with a vertical line proximate to the beginning of the text.

"simplifying" it to just an underline would be a design regression.

Also, we do not need to add an additional underline on the text when in focus, the underline is an affordance which indicates "link" and should appear only on hover. The fact that we allow the underline to serve a dual purpose in the horizontal tabs (with two different colors for focus and hover) does not mean we should muddy the waters here.

  • alexpott committed dba9518 on 8.1.x
    Issue #2368373 by rafuel92, BrightBold, b0unty, rpayanm, pjbaert,...
malcomio’s picture

Status: Active » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.