Dream mark up for tabs

Any reason that this shouldn't be as an unordered list?

Markup simplicity; easier to style without the extra elements. However, it would need screen reader testing. We have the navigation ARIA role, we may need additional roles on children.

This article does a good job of explaining the merits of dropping lists on navigation, with a focus on accessibility: http://css-tricks.com/navigation-in-lists-to-be-or-not-to-be/

Did some progression on this one. CSS needs some refactoring. I'll leave that for someone who's more familiar with this.

I'll change the status to Needs Review so that test bot tests if my patch breaks Drupal or not.

refactored css. Please review

Ups, patch shouldn't be here.

Cleaned #10.

+++ b/core/themes/seven/style.css
@@ -146,12 +146,14 @@ ul.menu li.expanded {
+/* TODO */

@@ -163,9 +165,11 @@ ul.menu li.expanded {
+/* TODO */

@@ -235,6 +235,7 @@ li.tabs__tab a {
+/* TODO */

Still some TODO's left in the CSS.

The link that @ry5n posted in #2 has a follow-up about what came out of this discussion online. The conclusion was:

"Personally, I think marking up navigation as lists seems to be the best choice. You can look at the facts and decide for yourself though."

More importantly from webaxe's response:
"The straight answer? Continue to use lists."

Removing them removes semantics that some assistive technology relies on.

The problem I see with this link element-only solution is that creating nested menus is hard. What do you think? I think the usage of lists is pretty much the standard so I would go with that =)

+1 for lists

So much has changed since June....

Im not sure if there's anything to do anymore. I looked how the tabs markup looks like atm and its pretty much the same suggested here.

So does the HTML code sample in the issue summary with the current HTML (including the button)?

Where does the last patch stand? is it no longer required?

Things have changed a lot since I created the last patch. Now we're using templates etc. for generating tabs.

Didnt mean to assign this

Working on that

Here is a first approach:

• Updated markup in both menu-local-tasks.html.twig in Seven and system templates.
• Added 'tabs__tab' class in template_preprocess_menu_local_task() in menu.inc.
• Removed seven_preprocess_menu_local_task() in seven.theme.
• Updated styles.

Unassigning and changing status.

Gonna take look of this

Good job on the patch @b0unty!

1. +++ b/core/themes/bartik/css/colors.css
   @@ -12,10 +12,10 @@ body {
   +nav .tabs--primary li a.active {
   

   Is there a reason for the first nav?

2. +++ b/core/themes/bartik/css/style.css
   @@ -1287,46 +1287,51 @@ div.tabs {
   +nav .tabs--secondary:after {
   +  content: "";
   +  display: table;
   +  clear: both;
   +}
   

   If we really need to do this I'd prefer adding a class in Bartik.

Testing.

The result looks almost perfect.
I applied the patch successfully to current 8.0.x. I compared the screenshots with the actual site with Bartik, Seven and Classy.

I found a problem on pages like Manage fields in the (example: admin/structure/types/manage/article/fields). While without the patch you see a number of list items, they are hidden below the content when the patch is applied. I'm attaching two screenshots to show the problem.

The problem comes from the "height: 35px;" that is set in tabs.css, line 7. What is that line in for?

I identified two problems, the one with the height mentioned in #47, and an issue with a missing border-bottom, which can be seen in the screenshots from comment #47.

I am attaching a new patch and an interdiff file. Please review :)

Adding screenshot for expected look with patch from #48.

testing

Tested again, in Chrome, Firefox, Safari, and iOS Simulator. I'm not sure what the design was for this but it looks different/better in Safari (desktop and smartphone version). Should probably be tested in an android browser and mobile chrome.

Looking good in Android browser as well (tested with browserstack, screenshot attached).

One more tester please :)

First patch in a D8 issue here :)

@subhojit777 Thanks for your work on this! Could you provide an interdiff file to make it easier to review? Here's the d.o documentation I used when creating mine: https://www.drupal.org/documentation/git/interdiff

@aschiwi here is the interdiff

@b0unty @LewisNyman I do not understand what do you mean by "make the selectors a bit weaker" as said in #56 I want to get involved in Drupal 8, so please let me create the patch :)

@b0unty rerolled patch including changes as said in #56. Interdiffs included.

Sorry, incorrect extension for interdiffs uploaded. Uploading them again.

Fixed mobile login view.

@Jens-0 could you please submit an interdiff as well so that we can the difference between last two patches.

@b0unty Sorry but I do not undertand what markup changes have been made. I test applied patch in #76 on 8.0.x branch and it applied without any error. I have asked @Jens-0 to submit an interdiff, may be I can figure out what tab markup changes have been made.

Failed creating the patch last time, so ignore #79. Now interdiff attached.

I wonder why not change TAB (tabs__tab) to Navigation items (navigation__item). And then we can have a shared .class & markup to set these links displayed in horizontal or vertical styling. e.g.: http://getbootstrap.com/components/#nav

If you looking around the real world web, Tabs are switching the content without refresh, not a group of links in tab-like styling. For examples, the top right links in Drupal.org, if it styled with rounded corners, will you call it Tabs?

https://developer.yahoo.com/ypatterns/navigation/tabs/moduletabs.html
http://www.smashingmagazine.com/2009/06/24/module-tabs-in-web-design-bes...
http://getbootstrap.com/javascript/#tabs
http://foundation.zurb.com/docs/components/tabs.html
http://jqueryui.com/tabs

1. +++ b/core/themes/bartik/css/colors.css
   @@ -12,10 +12,10 @@ body {
   +.tabs--primary li.active a {
   

   li -> .tabs__tab ?

2. +++ b/core/themes/bartik/templates/menu-local-tasks.html.twig
   @@ -0,0 +1,27 @@
   +  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
   +<nav role="navigation">
   

   Shouldn't this hidden element put inside NAV tag.

3. +++ b/core/themes/seven/css/components/tabs.css
   @@ -45,58 +45,63 @@
   +.tabs .tabs__tab {
   

   reduce specificity ?

   .tabs .tabs__tab -> .tabs__tab

4. .tabs--primary li {
     float: left;
   }
   

   using .is-horizontal to control float:left ?

5. .tabs__tab.active -> .tabs__tab.is-active ?
   
   

Thanks.

@droplet I agree with you. But, tab switching without refresh means you have to load content of all pages (unless you are loading via AJAX). Loading all content means it will take more time to completely load the page, the load of HTML DOM will increase considerably and hence browser will start taking more memory. If there are less content to show in tabs then tab switch without refresh seems feasible.

@LewisNyman shall we go for the updates as suggested by @droplet

About #83-2

+++ b/core/themes/bartik/templates/menu-local-tasks.html.twig
@@ -0,0 +1,27 @@
+{% if primary %}
+  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
+<nav role="navigation">
+  <ul class="tabs tabs--primary clearfix">{{ primary }}</ul>
+  {% endif %}
+  {% if secondary %}
+  <h2 class="visually-hidden">{{ 'Secondary tabs'|t }}</h2>
+  <ul class="tabs tabs--secondary clearfix">{{ secondary }}</ul>
+</nav>
+{% endif %}

H2 is a part of NAV.

@LewisNyman here goes the patch as suggested by @droplet in #83.

@LewisNyman @droplet I am not sure why do we need the heading before navigation tabs. Does it serves some sort of accessibility.

Is this issue related #2045039: Cleanup HTML heading structure to this one?

It's totally markup problem more than accessibility and the problem is introduced in THIS PATCH

If you do something like this that won't affect accessibility in most cases:
<li><ul></li></ul>
But it's an error in markup.

Please have a look:
http://cgit.drupalcode.org/drupal/tree/core/modules/system/templates/men...

If we think H2 is not related to NAV. Then I think the code should be:

{% if primary %}
  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
 <nav><ul class="tabs primary">{{ primary }}</ul></nav>
{% endif %}
{% if secondary %}
  <h2 class="visually-hidden">{{ 'Secondary tabs'|t }}</h2>
  <nav><ul class="tabs secondary">{{ secondary }}</ul></nav>
{% endif %}

If we think this is related:

<nav>
{% if primary %}
  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
  <ul class="tabs primary">{{ primary }}</ul>
{% endif %}
{% if secondary %}
  <h2 class="visually-hidden">{{ 'Secondary tabs'|t }}</h2>
  <ul class="tabs secondary">{{ secondary }}</ul>
{% endif %}
</nav>

Of course, anyone can explained in their own way, it's hard to say RIGHT or WRONG on HTML5 Semantic World.

Bad markup is bad for accessibility! @droplet I'm a bit confused though how this patch leads to the problems you are describing. I don't see it.

I don't see a problem with the process you are describing. Would make sense to me to wrap a NAV element around both primary & secondary tabs as you've described, but not sure how you'd determine the relationship.

@mgifford,

I have no idea how to explain it in more details because i think it's very clearly and a simple concept. Let me try...

Version A:

{% if primary %}
  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
 <nav role="navigation"><ul class="tabs tabs--primary">{{ primary }}</ul></nav>
{% endif %}
{% if secondary %}
  <h2 class="visually-hidden">{{ 'Secondary tabs'|t }}</h2>
  <nav role="navigation"><ul class="tabs tabs--secondary">{{ secondary }}</ul></nav>
{% endif %}

Version B:

<nav role="navigation">
{% if primary %}
  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
  <ul class="tabs tabs--primary">{{ primary }}</ul>
{% endif %}
{% if secondary %}
  <h2 class="visually-hidden">{{ 'Secondary tabs'|t }}</h2>
  <ul class="tabs tabs--secondary">{{ secondary }}</ul>
{% endif %}
</nav>

Version C:

{% if primary %}
  <h2 class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
  <nav role="navigation">
    <ul class="tabs tabs--primary">{{ primary }}</ul>
 {% endif %}
 {% if secondary %}
  <h2 class="visually-hidden">{{ 'Secondary tabs'|t }}</h2>
    <ul class="tabs tabs--secondary">{{ secondary }}</ul>
  </nav>
 {% endif %}

==============================================

thinking of the relationship between NAV & Primary tab & Secondary tab

Now, I just can tell Version A & B are better than Version C. And Version A & B structures are more close to unpatch version.

Just go ahead if you're think Version C is okay.

Cheers :)

I think @droplet has explained well, and he has a good point. The current twig structure for tabs looks wrong (version C). We should go for either version A or B. I will vote for version A. version A will not harm the accessibility, and its twig structure looks good. @droplet++

@mgifford @lauriii @LewisNyman your call.

Version A or B look good. I don't have a preference. Agreed that C is a problem. @droplet Sorry to have you go into more detail on this.

Let's fix the patch so that we're not getting C?

Here is the patch that follows @droplet's version A suggestion. My explanation for applying version A can be seen here #95. Seems like someone has already corrected Bartik's twig template. This patch fixes system's twig template.

A furthermore fixes to #98.

<h2 id="primary-tabs-title" class="visually-hidden">{{ 'Primary tabs'|t }}</h2>
  <nav role="navigation" class="is-horizontal is-collapsible" aria-labelledby="primary-tabs-title" data-drupal-nav-tabs>

Actually I just found a better dream markup is already in Seven theme. It's more accessibility. (I didn't add into this patch)

1. +++ b/core/themes/bartik/css/style.css
   @@ -1309,36 +1315,24 @@ div.tabs {
   -  .tabs--primary li.active a {
   

   Do we need this empty styling.

2. +++ b/core/themes/bartik/templates/menu-local-tasks.html.twig
   @@ -17,11 +17,13 @@
   -<nav role="navigation">
   

   The indentation is incorrect.

Do we need this empty styling.

Why ? new Code Standard ?? (If Yes, very odd rule)

I'm guessing @subhojit777 was reading it wrong. The styling on the removed code doesn't matter. It happens....

Moving this back to Needs review.

Sorry @droplet my bad I commented on the deleted codes :p - #100

Thanks @LewisNyman for the corrections.

Looks fine for me on OS X 10.10, Chrome 39.0.2171.7.

I also testet it now on several other combinations without noticing any problems. I am not sure if i should post all the screenshots i made but i could attach them if anyone is interested.
Windows 7: IE 11 10 9, Chrome 36 35 34, Firefox 30 29 28, Opera 12.16.
OSX 10.10: Firefox 30 29 28, Chrome 36 35 34, Safari 8.
OSX 10.09 Safari 7.
iPhone 5s & 6 Safari.
iPad Air Safari.

Sorry to push this back but this should be a quick fix:

+++ b/core/includes/menu.inc
@@ -331,10 +331,12 @@ function template_preprocess_menu_local_task(&$variables) {
-    $variables['attributes']['class'] = array('active');
+    $variables['attributes']['class'] = array('active tabs__tab');

Seems these should be two separate strings, i.e. array('active', 'tabs__tab');

The interdiff tool I am using in Ubuntu seems broken. Here is the correct interdiff (Interdiff obtained with the help Stackoverflow: Show all changed files between two Git commits)

YAY!

We need a CR for this and we need evidence of RTL testing.

Looking into the issue.

@b0unty Thanks for testing this. Here is the fix.

Screenshots.

Moving this to "Needs review" for the testbots. Anyone please review this, and change it back to "Needs work" as alexpott has asked for change records (#119).

RTBC +1

Invoking tests..

Invoking bot to test #122

Uploading the patch again..

+++ b/core/themes/seven/css/components/tabs.css
@@ -260,7 +260,6 @@
-  float: left;

I have just removed this styling. Due to this the tabs were not aligning right in RTL (See b0unty's comment in #120). Screenshots taken after applying this patch #123

Rerolled after #2375673: Split Bartik's CSS into SMACSS style components

There are theme breaks.

Lets see what I can do.

@lauriii an interdiff always helps to identify the changes.

Its not possible to create interdiff for reroll

@lauriii I thought you are using interdiff http://freecode.com/projects/patchutils

my bad I did not clear the cache. silly me.

Screenshots:

Thanks all!!

I have missed the changes in page.html.twig

I am not sure this fix should go with this issue. However the same test was passing in my localhost, I have PHP 5.5.9

What is the change on #150 intended for?

#147 was failing due to array syntax. In #150 I corrected it. But later I saw that #147 passed the test. So #150 can be ignored.

There was :focus problem. While replacing the seclectors with new class we missed the :focus attribute style.

@b0unty Thanks for testing