This is a follow-up to #925350: Vertical tabs broken, where we fixed the display of vertical tabs in RTL languages generally. Seven uses its own styling for vertical tabs (employing faux columns with a background-image), which is not compatible with the new generalized RTL solution (screenshot attached). The attached simple patch serves as a starting point by completely removing Seven's own styling of vertical tabs and adding a vertical padding of 1em to vertical-tabs panes so that the first element in a tab sits nicely within the pane.

As a consequence, /themes/seven/vertical-tabs.css and /themes/seven/images/fc.png could be removed from core.

In effect, we also partially revert #896828: Vertical tabs - no visual connection between the active tab and its content, which should be scanned as to whether important stylings introduced there shoul be re-incorporated here.

This has been tested to work in IE7, IE8, FF and Chrome.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Category: bug » feature
Priority: Major » Normal
Status: Needs review » Closed (won't fix)

Not major.
Not a bug (its a design decision).
Changes the design (feature request).

We're in beta, the design was hashed over for a long time by all involved, hook_css_alter and doing what we're doing in Seven it totally legit theming (calling this a bug is incorrect, maybe a limitation of Drupal CSS handling, but that's not Sevens problem).

So no, I do not agree with any of this and we should not be removing design from Seven with Drupal 7 now in beta. If you had concerns regarding the design these should have been raised quite some time ago - around the end of August - post CPH DC when the redesign of Sevens vertical tabs really got into full swing.

I have marked this as a won't fix. If you think this can legitimately be reopened please move it to D8 - however I think there are significant other patches for D8 that block this already (broader info file level handling of CSS as opposed to nefarious hook_css_alters...). I do not see that the design needs to be changed, we did that for a reason and I do not see any benefit of re-opening that issue.

At some point we lock these things down and say "that's it, this is what we're rolling with in D7". Right now this works flawlessly in LTR and RTL and satisfies the design requirements.

reglogge’s picture

Category: feature » bug
Priority: Normal » Major
Status: Closed (won't fix) » Needs review
Issue tags: +RTL
FileSize
50.22 KB
3.08 KB
517 bytes
76 bytes

Right now this works flawlessly in LTR and RTL and satisfies the design requirements.

No, it doesn't. Not in Seven. Do you really want to ship Drupal with vertical tabs in the default admin theme looking this way (attached screen vertical-tabs-rtl-seven-before.png) for RTL languages?

Something clearly needs to be done here. If you'd rather prefer to adapt the faux columns technique to RTL, that's fine. An alternative patch doing that is attached. But doing nothing and just closing this as won't fix is wrong.

The attached patch needs the image fc-rtl.png put into /themes/seven/images and the new file vertical-tabs-rtl.css put into /themes/seven.

The main difference between the two patches is that the first just resets Seven's styling of vertical tabs to the core standard for all themes. This second patch preserves Seven's different styling with faux columns and also incorporates the adjustments made to the width of input elements from #925350: Vertical tabs broken. This is necessary since we override /misc/vertical-tabs.css. As to all the other improvements from #925350: Vertical tabs broken, I'm not sure whether they are preserved.

So I think this is:
a) an obvious bug,
b) major, since the design is totally borked without a fix and
c) definitely D7 material.

Jeff Burnz’s picture

The patch and proposal I see in the OP is why I closed this, and because we have at least two other issues dealing with either vertical tabs or RTL styling in Seven.

#896828: Vertical tabs - no visual connection between the active tab and its content
#766458: Seven theme lacks rtl styling

What I meant is that afaict it works ok, maybe not looking great but working.

OK, so maybe closing won't fix was a bit harsh, but there is no way want to be rolling this back to core styles and starting over. The design is the design and atm we want minimal fix for the issue at hand - RTL, not a total rework which is what I see proposed in the OP.

reglogge’s picture

Proposal: Let's fix the vertical-tabs issue for Seven here. It's separate from #766458: Seven theme lacks rtl styling insofar as it deals with separate stylesheets (namely vertical-tabs.css and vertical-tabs-rtl.css n /themes/seven). No need to further burden #766458: Seven theme lacks rtl styling with this.

From a maintainability POV, I still think the patch in the OP is better, since we wouldn't have to maintain separate stylesheets for vertical tabs in Seven at all and no overriding the stylesheets would be necessary. Seven works just fine in both LTR and RTL with the default styling from the vertical-tabs stylesheets in /misc. Any additional bugfixing for issues such as Everett raises in #896828: Vertical tabs - no visual connection between the active tab and its content could then also be made in the core stylesheets and then benefit every theme a user decides to use as the admin theme.

If we want to preserve Seven's different way of handling vertical tabs, then #2 can serve as a basis for further patches.

The attached screenshot shows the differences between current HEAD, the patch from the OP and the patch from #2.

Jeff, you're the maintainer for Seven, you decide...

Jeff Burnz’s picture

Fix it here, with the patch in #2, I really don't want to be redoing things at this stage (we want to preserve the visual style), so its just about bug fixing. I don't see an overridden stylesheet as a bug, not at all. I think the maintainability angle is getting a bit over cooked of recent times, we're talking about 80 odd lines of CSS, not 800.

reglogge’s picture

OK, I tested the patch in #2 with IE6, 7 and 8, Firefox, Chrome and Opera both on Mac and Windows and found nothing wrong with any of the browsers. The patch in #2 just adds some RTL-styles where necessary, a new stylesheet together with the override and a new backgound-image for vertical tabs in RTL. It also adds Jacines solution for fieldsets that can overflow their container from #925350: Vertical tabs broken. It changes nothing else. To me this looks ready to go in. Review anybody?

reglogge’s picture

Issue tags: -RTL

#2: vertical-tabs-rtl-seven-1.patch queued for re-testing.

reglogge’s picture

#2: vertical-tabs-rtl-seven-1.patch queued for re-testing.

XiaN Vizjereij’s picture

#2: vertical-tabs-rtl-seven-1.patch queued for re-testing.

reglogge’s picture

Issue tags: +RTL

#2: vertical-tabs-rtl-seven-1.patch queued for re-testing.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
76 bytes
517 bytes
3.3 KB

... let's try to push another rtl patch again ...

1) needed reroll
2) tested on IE6-IE9, chrome, opera and firefox
3) patch attached that contains the adjustments + new file
4) new rtl file attached if it can't be done in a single patch
5) new picture needed (put into seven/images)
6) definitly RTBC (I only did a reroll)

7) COMMIT THIS :)
8) Look also at #740182: Toolbar and shortcuts lack RTL styling and #766624: Dashboard lacks rtl styling

EXTRA INFO
-----------
9) I use d7 latest dev version
10) read #6 and #5 if you're still not convinced
10) what else do you want to know before getting this commited...

aspilicious’s picture

FileSize
19.83 KB

AND I made an IE7 screenshot just to proove I actually test IE.

tsi’s picture

Looks great, not thoroughly tested but RTBC from what I saw, this is a huge improvement, thanks.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -RTL

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