Vertical tabs broken

Just took a took a quick look - seems the problem is in vertical-tabs-rtl.css, the selectors don't have enough specifity. Changing component.

When you can't create content, it's a critical bug. :(

Oh boy... Good catch.

I haven't reviewed the CSS in detail yet, but this is making me nuts right now.

+++ themes/seven/template.php	28 Sep 2010 19:17:55 -0000
@@ -97,6 +97,9 @@ function seven_css_alter(&$css) {
   if (isset($css['misc/vertical-tabs.css'])) {
     $css['misc/vertical-tabs.css']['data'] = drupal_get_path('theme', 'seven') . '/vertical-tabs.css';
   }
+  if (isset($css['misc/vertical-tabs-rtl.css'])) {
+    $css['misc/vertical-tabs-rtl.css']['data'] = drupal_get_path('theme', 'seven') . '/vertical-tabs-rtl.css';

The fact that this is required in the first place just goes to show how broken CSS file handling in themes is right now. Just one more reason this #575298: Provide non-PHP way to reliably override CSS should go in now.

This is not the right way to do it. We need to wait for #575298: Provide non-PHP way to reliably override CSS

screenshots please for all browsers/all themes (please? :p)

Maybe a "this is how it should look" screen-shot is good, so testers can compare, so they know what they are supposed to be looking at.

IE7 bartik after has a border problem with the active tab

+++ themes/seven/style-rtl.css	29 Sep 2010 12:07:33 -0000
@@ -18,3 +18,15 @@ ul,
+/**
+ * Vertical tabs.
+ */
+div.vertical-tabs {
+  background: #fff url(images/fc-rtl.png) repeat-y top right;
+}
+div.vertical-tabs div.vertical-tabs-panes {
+  margin-left: 0;
+  margin-right: 265px;
+  padding: 10px 0 10px 15px;
+}

Not so sure about this, seems a bit inconsistent with how we override the standard stylesheet, while sun doesnt like the hook_css_alter stuff for Seven I would prefer it since its consistent - we should fix this now with the current tools, and worry about the info file stuff later should that make it in.

Attached a screenshot in IE7, see there is no padding/margin between the tip and the vertical tabs, I had this problem before, could be a hasLayout issue in RTL.

Powered by Dreditor.

Jeff, I'm not seeing the spacing issue you mention in #20

IE7 Mode in IE8 on Windows 7: http://skitch.com/jesse.beach/d3ggc/windows-7-running

IE7 on XP: http://skitch.com/jesse.beach/d3geq/windows-xp-sp2-ie7-native-running

Overall, the patch works. I'd rather not introduce selectors like

div.vertical-tabs ul.vertical-tabs-list {
 float: right;
 margin-right: 0;
 right: 0;
}

where div.vertical-tabs and ul.vertical-tabs-list declare unnecessary degrees of specificity at the cost of efficiency. .vertical-tabs is sufficient unless we're explicitly contrasting div.vertical-tabs with something like ul.vertical-tabs. Same for .vertical-tabs-list.

The spacing issue in #20 is a pita, I know it doesn't always occur, I found this also when we were working on the vertical tabs make-over, sometimes it occurred, sometimes not, I couldn't put my finger on it but I am pretty sure its a hasLayout bug, but as to why, well...

If we step back to the shorter more efficient selectors (yes I agree here) does it still work OK, because one of the issues I found earlier was the specification not being sufficient enough - we don't want other CSS messing around with VT, say a themes styling for list items.

+++ themes/bartik/css/style.css	30 Sep 2010 18:56:30 -0000
@@ -1153,6 +1153,15 @@ div.password-suggestions {
+div.vertical-tabs ul.vertical-tabs-list li.selected a {
+  height: auto;
+}
+div.vertical-tabs .form-item {
+  width: 408px;
+}
+div.vertical-tabs input.form-text {
+  width: 408px;
+}

This will break if two sidebars are active - I'd prefer to see this tackled in another Bartik specific patch that fixes this whole issue of form items overflowing fieldsets.

+++ themes/seven/ie6.css	30 Sep 2010 18:56:30 -0000
@@ -18,3 +18,6 @@ ul.links li a,
+div.vertical-tabs .form-item {
+  width: 36em;
+}

again, this hard wired width on form items irks me

+++ themes/seven/vertical-tabs.css	30 Sep 2010 18:56:30 -0000
@@ -5,6 +5,7 @@
+  height: 1%;

seems like something for ie.css

Powered by Dreditor.

Jacine review maybe?

I started reviewing this yesterday, but I'm not done yet. There are quite a few problems here, and this is not only broken in RTL. When I am finished I will repot back.

Also removing a bunch of irrelevant/unnecessary tags.

Ok, well this issue has uncovered a bunch of issues in IE6, as well as the issue with textareas and input[type=text] that have been long plaguing us inside vertical tabs. There were also a few issues introduced by recent accessibility improvements that were not exposed (at least I did not notice them) until trying to address the issues in this patch. We will need the accessibility team to test this and make sure all the tabbing they added still works. I think it's fine, but better safe than sorry.

After a couple of hours of trying to debug this, I'm done with Stark (the base styles are now good). This patch is not finished because unfortunately Seven takes it upon itself to implement its own version of vertical tabs. From what I am seeing, this is not needed at all. The design is not different enough, by any means, that we need to start from scratch and maintain all this extra code. I think that it's fine to provide a vertical-tabs.css within the Seven theme, if the maintainers want that, but that this should not override the core file entirely, and the overrides should not cause the need for a vertical-tabs-rtl.css It's too much maintenance, and again, not necessary.

I'm the against hard-coded pixel widths for IE6 introduced in the previous patch here. It's not necessary. I also think that we should attempt to provide a core fix for the issue with certain form elements overflowing the vertical tabs content area. It makes no sense to make every theme deal with this, as it occurs in most browsers. The following lines in vertical-tabs.css do a very nice job at resolving this, and it's the same way we fixed up resizable textareas:

/**
 * Prevent text inputs from overflowing when container is too narrow. "width" is
 * applied to override hardcoded cols or size attributes and used in conjunction
 * with "box-sizing" to prevent box model issues from occuring in most browsers.
*/
div.vertical-tabs .form-type-textfield input {
  width: 100%;
  -moz-box-sizing: border-box;
  -webkit-box-sizing: border-box;
  box-sizing: border-box;
}
* html div.vertical-tabs .form-type-textfield,
* html div.vertical-tabs .form-textarea-wrapper {
  width: 95%; /* IE6 */
}

I am posting this now, because both Stark and Bartik are ready for testing, and I many not have enough time to continue on Seven today.

Thoughts?

If the CSS for vertical tabs in Seven can be merged into the main stylesheet and the design not changed that would be fine. A lot of time was spent refining VT in Seven, its is quite different in the details and importantly does not have that ugly gap below the tab list when the pane is longer than the tabs, which visually isolates the submit buttons.

Can you clarify what these issues are in IE6 that we are trying to fix? I have reviewed the patch but would like to discuss the issues we're trying to address first, as there are some new issues with this patch that I do not get with the original CSS (IE6 & 7).

If not already this will fail, there was an update the system.base.css just recently.

Well, I'm not familiar enough with Seven, and that's not my queue, so I wont make that call. I took another look at it, and it appears it will need to have an RTL file after all since it decides to apply a different/pixel width to the tabs.

I think that should be handled in Seven's queue, not here, given the current state (Bartik and Garland make use of core code, and Seven does not).

The way this code (core Drupal CSS) needs to be tested is with Stark first. I don't think that happened in the case of vertical tabs. It's totally broken in IE6 in both LTR and RTL versions. The issues are pretty obvious, just enabling a theme and editing a node, and clicking through the tabs, but I will provide detailed reasons for the changes. This just ended up taking hours to write/fully test, and I don't have any more time today. Not sure what update in system.base.css you are referring to, nor why you think this will fail without trying it, but I will provide more details as soon as I get a chance.

EDIT: It's obvious in Stark with when both sidebars filled with blocks.

Yea, I applied it, originally it failed with the hunk in system base, which is kind of weird because now it applies just fine!

OK, so my IE6 was acting real weird when I first tested this and now I have those issues sorted out (thank goodness for virtual machines):

1. Awesome IE6 fixes here, I bask in your excellence.

2. The display block property for tab anchors is not working in IE7. We ran into this same problem with Seven, after much hacking I found the margin: 0 -100% -1px 0; line in the original CSS fixes this. Its a very rare hasLayout bug afaict.

3. I see some odd issue with outlines in Bartik, will keep digging, however this is looking very good indeed.

@jacine

Can you, or someone else, please provide a summary of the issue so that the accessibility folk know what they're looking for?

Generally speaking the following is required.

1. Focused, selected, and non-selected tabs must be visually distinguishable on all themes.

2. There was some weird keyboard stuff added that I wasn't a fan of, but essentially pressing enter on a tab should take you to the first field in the sub-form and pressing enter there (except for a multiline text field) should take you back to the selected tab.

@32 - yes thats basically it. Visually they look good - core does the focus styles with outline and underlining the link, Seven uses background color on the tab. Non-selected tabs are coloured gray, while the selected tab is white - the same color as the panes (to visually connect an active tab to its pane).

For me the automagic first selected field works, as does pressing enter again to go back to the tab. I can see small issue with Seven in this regard because it does not highlight the active tab on focus/hover, we should look at that in the Seven queue.

From my limited testing in Firefox and IE8 the behavior appears to hold up and the styling supports the behavior (for core/Stark). For me its easier to use these tabs with the keyboard than a mouse, it seems quite intuitive.

@Jeff

Thanks for the review. Do you know if there is currently an open Seven VT issue to which it would be appropriate to add the bug about active highlighting, or should I open a new issue against Seven?

There is no open issue, but an old one you could re-open that dealt with the redesign of vertical tabs in Seven - better carry on that discussion in Seven queue and not go off-topic to much in this critical.

Glad to be rid of the Seven css_alter, however we've lost all the design style for Seven - the nice spacing, some of the colors etc, lets bring those back in.

@reglogge your last patch reverts many of the IE fixes I spent hours on.

Implementing faux columns to achieve the background color is not only a design change, but it causes other potential problems. Sure, maybe these could be addressed, but they are far from critical, and just because some people think it's ugly doesn't mean it's even a bug. I would really prefer that we don't delay fixing a critical for something like that. The patch in #36 also does not take into account many of the changes Seven makes to the tabs. If you guys think the Seven should be fixed here, which I still disagree with, we need to get everything else working well first.

I'm in the process of taking screenshots comparing HEAD vs. patch in #28 and patch in #36, and writing up an explanation for what I did. I'm assigning this to myself at least until I get that done. Standby.

OK, lets re-establish the focus:

1) The critical issue is core handling of vertical tabs - this is priority one and the only thing we should address in this patch.

2) Seven is off the table for now. If problems arise we look at that in the Seven queue as per any normal bug.

Right now the focus should be firmly on the critical issue. Once we have that down we could reconsider Seven.

Jacine - that extra wrapper could make sense for themers, perhaps we can consider it here (I have not tested beyond a cursory glance), certainly it opens the door for us in Seven to remove the crufty css alter.

Issues #28 Fixes (see PDF files)

• Form elements extend beyond the container (all browsers)
• Layout borked in RTL to the point where content can't be properly edited. (all browsers)
• Empty space underneath active tab (IE6 & IE7)
• Outline for focused tab link extends beyond the tab (IE7)
• Line-height on link causing Text jumping when focused in tabs (IE6)

Issues introduced in Patch #36 (see PDF files)

• Borders are off on container in RTL (all browsers)
• Borders partially lost on other non-active tabs on hover
• Introduced more z-index code which is not desirable
• Faux column div sometimes appearing inside content area (all browsers)

Changes since patch #28.

• Removed the line-height that was causing text to jump in IE6
• Thanks for explaining the keyboard tabbing functionality, Everett. That's exactly what I needed to know. Tracked down the source of the problem causing the container to need the position relative and the holly hack. Removing this code fixed the problem and all is still working well. I'm pretty sure the odd positioning/hasLayout hacks applied to the links was the reason this was added in the first place.

div.vertical-tabs ul.vertical-tabs-list li a:focus,
div.vertical-tabs ul.vertical-tabs-list li a:active {
  position: relative;
  z-index: 5;
}

Attached PDF files detail the problems with screenshots, showing HEAD, patch in #28 and #36.

The display block property for tab anchors is not working in IE7. We ran into this same problem with Seven, after much hacking I found the margin: 0 -100% -1px 0; line in the original CSS fixes this. Its a very rare hasLayout bug afaict.

I'm not seeing a problem like this. How are you testing this? It seems that display: block is being applied just fine on my end.

I tested both patches thoroughly, posted screenshots and details for everything I found (in the PDF and in the comment) except for one issue (Faux column div sometimes appearing inside content area (all browsers), which I forgot to screenshot), and provided a working patch for the critical issues at hand here.

I also explained why I don't think the Seven related issues should happen here. This is also not a regression at all. 1 - There were no vertical tabs in Drupal 6, and 2 - My patch does not change the design of the tabs that are in HEAD. What you are proposing does. Also, I'm not saying that you can't try to fix this in another issue, just that it's not critical, and since Seven is doing its' own thing entirely, it's not relevant to this specific issue.

I don't know that there's anything else I can do here at this point. You guys can hash it out. I tried.

I've done a quick review of both patches, testing only in Stark and IE6 and IE7, since those are the hard ones to get right.

#41
- looses display block in IE7 & IE6
- If the browser window is reduced in width a certain point the tabs list will loose layout (and jump left or right, depending on the text direction), this can be fixed with position: relative; on div.vertical-tabs

I could get display block to work in IE6 by setting the width on div.vertical-tabs ul.vertical-tabs-list li a
I could get display block to work in IE7 by setting the width on div.vertical-tabs ul.vertical-tabs-list li

Sounds so simple but, alas of course its not as setting two widths causes all sorts of hilarity. I could live with IE6 not having display block working to be frank, as this is perhaps the only real issue I have with this patch. Two small fixes and either concede on IE6 or find another fix, either way this is looking very robust and very clean.

#42
- in RTL IE7 looses the display block
- IE7 can get some extra "padding" when the last tab is selected (right below it, even when the pane is very short)
- in IE6 when hovering inactive tabs they can loose the border right (which is actually set on the panes)
- on page reload in IE6 sometimes the panes are mostly #eee, until I click something
- no border bottom on the last tab
- changes the design

This patch is harder to understand because of the z-indexing and I am having a hard time debugging it.

Conclusion: both need work, however why are we competing here - can we please hold fire for a second and work out the specification and collaborate on a grade solution. This is a lot of work to test and debug two patches.

If I have to pick on it would be Jacines approach, its a lot cleaner and fixes the critical issue with some very minor issues to clean up. The design change is just that, a design change, and not a requisite of fixing the critical.

EDIT: Jacine, atm I am testing with the IE browsers from spoon.com, I also use IE Tester but its very unstable on my system, however both give the same results.

I've had a go at reworking the patch from #41, however fixing the layout issues (the tabs list can jump out of alignment when you resize the browser in IE6/7 - only for fluid width themes) proved to be rather tricky so in effect the patch has changed a fair amount:

1 - fixes the layout problem in #41
2 - fixes the display block problem in #41 (tabs can loose display block in IE7)

There is one caveat I am yet to solve - in IE7 and Opera if the browser is reduced to a very narrow width the fieldsets can wrap under the tabs. Not sure if this is a huge problem because it has to be very narrow, more narrow than Bartik with two sidebars.

Also you might think the way the layout works in this patch is a little magical - it uses a combination of floats, position relative with negative values and negative margins... well, IE6/7 are a huge PITA and we need a little magic sometimes...

Regarding Bartik, I made the tabs 12em wide instead of 15, I think in Bartik with 15em tabs they take up too much room.

The patch does not include the extra system.base.css addition for IE6 textarea nor does it change anything in Bartik. We can get to those later, right now the critical is what we should fix afaict.

Jeff, I didn't realize you responded about the testing. When I was asking how were testing, I was referring specifically to the display: block issue. I'm a little baffled by this, because I am seeing it work just fine on my end. I would really like to see what you are seeing. Maybe a screenshot or some other code you are trying to apply? From what I can tell, the position: relative (unrelated) was still needed, like you said, so I added it back to my last patch, and went and downloaded IETester. I have 2 virtual machines that I use to test IE in native Windows environments. It's painfully slow at times, but I have found no better way to test IE6 especially. I test IE6 on XP SP2 and IE7 and IE8 on Vista.

I'm getting display: block in IETester as well. I would really l like to figure out exactly what is going on with that, before adding widths for links, and negative margins for fieldset wrappers, personally, but I don't have Windows 7. However, IE6 users definitely don't have Windows 7 either, and isn't IE8 what's installed by default on Windows 7? I think it's worth trying to figure out if the discrepancy we have here is a result of inconsistencies.

Anyway, here's a bunch of screenshots showing display block clearly working on my end in IE6. I also included 2 that show what happens if I manually set display: inline or display: inline-block on one of the links and a screenshot of IETester, which shows the same thing. Maybe it will help?

TBH, after spending the majority of my weekend working on this, I'm not thrilled to have yet another completely different patch here to review. This issue is currently sucking all the will I have to work on Drupal 7 right now, but since you've been so courteous as to review my patch, I will do the same for yours.

Oy, removing duplicate comment of #46.

Can't view the images :s

http://drupal.org/files/issues/ie6-display-block-focus-outline.png
http://drupal.org/files/issues/ie6-display-block-dev-toolbar.png
http://drupal.org/files/issues/ie6-setting-display-inline-manually.png
http://drupal.org/files/issues/ie6-setting-display-inline-block-manually...

Thanks @aspilicious & @Bojhan. Not sure what happened there.

I forgot to attach the IETester screenshot I referred to, so I'm attaching it here.

Oh shivers, I made a screencast the other day about this, seems I forgot to add it - its done in IE7/Stark with the patch from #41 - http://bit.ly/c6A8R4

I get this in IE7, navigating with the mouse. If I navigate with the keyboard the outlines appear as if display: block; is working, but with the mouse no worky (see the video above).

Your patch is much more simple code and would be more preferable, agreed, but we need wider testing. I can't spend too much more time on this (with regards to writing code), I'm up for reviews of course. I'll let you drive this forward, just threw a patch in there as a POC + fall back if we can't nail this nasty little IE7 bug.

Thank you Jeff. That was perfect.

I made a copy of my Vista VM and installed "IE Collection" to triple check. Good thing I made a copy of the VM, cuz it hijacked my pristine copy of IE6. And well, whaddya know... I was able to reproduce this in their "IE 7.0 (7.00.5730.13)", but not in Stark. Only in Garland and Bartik! Fun.

EDIT: Eventually I did see this in Stark too.

Ok, I got this sucker...

Jeff, like you said the width: 100% was working on the li, but it was also causing other undesirable issues, like actually messing with the width (which of course IE sucks at) and applying it also borked IE6.

Luckily, min-width: 0 works too, and doesn't affect anything else. :)

Bartik is still having a slight issue with the left border on the selected tab appearing, except when you actually click the tab, but Garland and Stark are perfect.

Here's what else has changed since the last patch:

• Added back the position: relative on div.vertical-tabs
• Changed all occurrences of div.vertical-tabs to .vertical-tabs, except for the first div.vertical-tabs, where it was needed. I found the original issue where this happened and it seems they just went overboard because they were having issues with the first div. From my testing the first was the only one that needed that specificity. I know this whole file could probably be a lot less specific, and I did try to slim them down more than this, but I ran into problems with core themes.
• Removed the change in system.base.css, it shouldn't have been there in the first place.

That's about it.

Sorry I thought the new patch needed a new screenshot but it seems hovering screenshots are needed and those are not really possible.

Ok... here's a screencast: http://vimeo.com/15556578

To summarize:

These are the problems the patch in #53 fixes:

1. Form elements extend beyond the container (all browsers)
2. Layout borked in RTL to the point where content can't be properly edited. (all browsers)
3. Empty space underneath active tab (IE6 & IE7)
4. Outline for focused tab link extends beyond the tab (IE7)
5. Line-height on link causing Text jumping when focused in tabs (IE6)

This screencast demonstrates the issue found by Jeff in IE7 with a previous iteration of this patch as being resolved:
http://vimeo.com/15556578

I get this in IE7, navigating with the mouse. If I navigate with the keyboard the outlines appear as if display: block; is working, but with the mouse no worky (see the video above).

http://bit.ly/c6A8R4

I've also taken new "after" screenshots to prove that everything is still good since the last round. Those are attached in the PDF.

@timplunkett noticed an issue with a comment that should read LTR instead of RTL ;)

Fixed.

Testbot has completely stalled. Also, can pure CSS changes even fail tests?

Anyway, I've gone through this twice, and it's ready to go.

+++ misc/vertical-tabs.css	5 Oct 2010 21:34:33 -0000
@@ -1,70 +1,78 @@
+.vertical-tabs ul.vertical-tabs-list span.selected strong {

This selector targets nothing, I think you want to following if the intention is to style the tab "title".

.vertical-tabs ul.vertical-tabs-list .selected strong

Still testing, but yes this looks really good, and oh my - min-width: 0;, very cunning indeed :)

Powered by Dreditor.

Thanks Jeff!

Yes, good catch. That's how it was before, and I'd really love to remove it entirely, but that's another story.

Here's an updated patch that fixes it :)

Great, back to RTBC. This is a massive improvement and fixes the critical issue.

Hrm, hello testbot?

Same patch again, for the testbot.

RTBC based on #63 and #65?

The problem exist with vertical tab CSS, stylesheet needs to be adjusted.

Wow, I think my CSS IQ just shot up 12 points as a result of this issue. Great work, folks!

Committed to HEAD.

This commit introduced a FAPI regression: #970426: Regression: Vertical Tabs styling on input elements overrides specifically set FAPI keys