Problem/Motivation

Content gets hidden in the vertical tabs see the screencast. This only happens in the seven theme.

Uses formtips to create a tooltip but can't escape the box due to overflow: hidden

Proposed resolution

  • Remove overflow hidden
  • check for regressions.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Component: CSS » Seven theme
Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Contributed project blocker
FileSize
419 bytes

Here's a patch. It's only relevant for the seven theme which makes it easier to commit. Also doesn't seem to be any visual regression, I saw some bullets showing in the past.

Adding contrib blocker because it's breaking formtips, which is what I used in the screencast in the issue summary.

joelpittet’s picture

Introduced here: #1989488: Vertical tabs style update, with no specific mention to the overflow: hidden.

occupant’s picture

Patch applies cleanly, overflow issue is no longer occurring.

gnuschichten’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
148.33 KB

I can confirm: The patch applies without errors and fix the overflow issue.
Also I can't find any usage for the property overflow:hidden.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#4 says there is no specific mention of overflow hidden on #1989488-3: Vertical tabs style update but there is in comment 3.

We need to verify that what is was fixing is not broken. From the comment it says:

The corner issue is when the element with border radius also has a background color, and this now has the fix for that which is overflow: hidden; (Ryan has this is the prototype, I didn't realize why at first).
joelpittet’s picture

@alexpott, thanks for noticing that I didn't look hard enough. If you zoom in really close you can see the issue.

Here's an over-exaggerated version to see what is meant:
https://jsfiddle.net/t97gwhvq/

One possible solution is to round the inside elements with background color, which considering the design of this should be fairly simple.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bskibinski’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
54.84 KB

New patch:

  • Removed overflow:hidden
  • Added border-radius to the inside of the elements.
  • Made sure RTL and LTR both have the right kind of border-radius.
  • added list-style-type: none; to the vertical-tabs__menu, to hide the menu bullets.

If I may be so bold, I would like to see this committed as fast as possible (after review of course ;-).
The rounded corners issue, while annoying, is not breaking functionality. But as you can see in my screenshot, a lot of functionality can break because of this. In my case, chosen fields are hardly usable.

If any minor styling bugs occur because of the removal of overflow:hidden, i would recommend opening new bugs-tickets to fix those, but get this in as fast as possible, so sites don't break, function over form :-)

bskibinski’s picture

I've noticed it's better to put the border-radius on the <a> element itself instead of the <li>.
now the 'not-active' first menu item is also properly rounded.

Ekvo’s picture

Status: Needs review » Reviewed & tested by the community

Recent patch works nice. Overflow issue is fixed, borders for seven theme are nice as well.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Sorry for the delayed response. I was on vacation for the last 2 weeks 🏖

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -37,6 +37,12 @@
+[dir="ltr"] .vertical-tabs__menu-item.first a {
+  border-top-left-radius: 4px;
+}

Is there a specific reason we can't specify ltr styles by default, and then just override them in the rtl styles?

bskibinski’s picture

No worries, hope you enjoyed your vacation! Welcome back to reality ;-)

Is there a specific reason we can't specify ltr styles by default, and then just override them in the rtl styles?

No, none other that I personally never like to override styles if I can prevent it, but that's just pedantic me.

But you are right, it's safer/better to just have it as a general rule, and override it for the RTL.

Fixed in the new patch.

bskibinski’s picture

I forgot to unset the border-top-left radius for the RTL languages after the latest change in the last patch.
Fixed in patch #15.

Ekvo’s picture

Status: Needs review » Reviewed & tested by the community

Last change tested. Borders still looking good on Seven theme with RTL language selected.

alexpott’s picture

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -37,6 +37,13 @@
+  border-top-left-radius: 4px;

@@ -93,11 +100,14 @@
+  border-top-right-radius: 4px;
+  border-bottom-right-radius: 4px;

These need a /* LTR */ comment after them. Can be added on commit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed aa4dda4 and pushed to 8.8.x. Thanks!

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -18,6 +17,7 @@
+  list-style-type: none;

This is required because the overflow: hidden was hiding the bullets.

diff --git a/core/themes/seven/css/components/vertical-tabs.css b/core/themes/seven/css/components/vertical-tabs.css
index 2d8f55fb44..0cc3719ba5 100644
--- a/core/themes/seven/css/components/vertical-tabs.css
+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -38,7 +38,7 @@
   border-bottom: none;
 }
 .vertical-tabs__menu-item.first a {
-  border-top-left-radius: 4px;
+  border-top-left-radius: 4px; /* LTR */
 }
 [dir="rtl"] .vertical-tabs__menu-item.first a {
   border-top-right-radius: 4px;
@@ -100,8 +100,8 @@
   margin: 0 0 0 240px; /* LTR */
   padding: 10px 15px 10px 15px;
   border-left: 1px solid #a6a5a1; /* LTR */
-  border-top-right-radius: 4px;
-  border-bottom-right-radius: 4px;
+  border-top-right-radius: 4px; /* LTR */
+  border-bottom-right-radius: 4px; /* LTR */
 }
 [dir="rtl"] .vertical-tabs__panes {
   margin: 0 240px 0 0;

Fixed on commit.

core/themes/seven/css/components/vertical-tabs.css
 20:3  ✖  Expected "list-style-type" to come before "line-height"                         order/properties-order
 45:3  ✖  Expected "border-top-left-radius" to come before "border-top-right-radius"      order/properties-order

Also fixed on commit.

  • alexpott committed aa4dda4 on 8.8.x
    Issue #3028675 by bskibinski, joelpittet, gnuschichten, alexpott,...

  • alexpott committed a5042ff on 8.7.x
    Issue #3028675 by bskibinski, joelpittet, gnuschichten, alexpott,...
alexpott’s picture

Discussed with @catch and @laurii and we agreed that since this is seven only backport is allowed.

Status: Fixed » Closed (fixed)

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