Markup for Stark's page.html.twig + maintenance-page.html.twig

This looks good, I was wondering if we should rename the page class to something more descriptive and inline with our CSS standards? I was thinking .l-container

Yes let's do that, some of these variables are dependant on #1053648: Convert site elements (site name, slogan, site logo) into blocks.

The ones that are already available as blocks should be removed from the page.tpl.php and placed in a new or appropriate region. It's a complete WTF for new users.

Do we need separate issue for this? I don't think so, we should be affecting the output of Bartik and Stark with this patch.

Tagging.

This is definitely looking much better! Will be really nice if we can get the region stuff figured out and get rid of these @todo's.

I will look into what is causing this to fail so many tests.

Here is a patch to correct some of the errors reported for #10. Just changing some xpath()'s and correcting some variables in maintenance-page.html.twig that were causing the exceptions to be thrown.

Tagging for review. The patch in #16 still has lots of work to be done, but would be nice to see some test results with out all those PHP exceptions being thrown at least.

I've gotten all but one of these test errors resolved I believe. Hopefully will get the other one finished in the next couple days and submit an updated patch.

Here's is an updated patch that should resolve the remaining test failures.

go ahead test bot.

Why are you replacing ids with classes? I'm not sure what additional flexibility it gives you, but feels like a bit of a loss as far as structure goes.

It's part of our new CSS standards:

Avoid using the id selector in CSS. There is no general benefit to using the 'id' attribute as a CSS hook, and it has serious downsides in the form of increased selector specificity.

The avoidance of ID selectors I think goes back to http://smacss.com/book/type-layout

I'm wondering what if any evaluation is being done though to address when they are useful elements? From that SMACSS page:
"To be clear, using ID attributes in your HTML can be a good thing and in some cases, absolutely necessary. For example, they provide efficient hooks for JavaScript. For CSS, however, ID selectors aren’t necessary as the performance difference between ID and class selectors is nearly non-existent and can make styling more complicated due to increasing specificity."

This looks like a great initiative though, thanks for the link. Just the call to avoid using ID selectors is different than don't use ID selectors. When do we know if they are being leveraged or not?

I've been trying to explore this, as I frankly haven't come across ID's as a problem before. You've definitely done a good job raising awareness as to why they have caused a problem for themers.

That being said there are solid reasons to use ID's for:

• form elements to relate labels & inputs as well as fieldsets
• other WAI-ARIA elements which actually add semantics
• tables to provide header/cell relationships
• javascript elements (if required)

Simply getting rid of all ID's from Core isn't an option, but that's what you are arguing for.

Some other related links I found interesting when digging into this.

Thread with references to WebAIM & changing thoughts of Jeffrey Zeldman:
http://www.impressivewebs.com/ids-not-make-documents-semantic/

I like how they talk about ARIA here:
http://oli.jp/2011/ids/

Regarding "Getting rid of all id's" - totally my sloppy reading/responding. Early in the comment that's what you seemed to be suggesting, but that was definitely clarified by the end. I should have edited my comment more clearly.

I can't make a good argument (yet) for the sidebars, so until I'm convinced I won't be arguing for them....

Thanks for explaining this to me.

And ya, I'll take the discussion about forms/tables to the principles page and leave this to:

stark page + maintenance page.

So, way back in #20 @tlattimore produced a patch that the bots liked. What's the process to get this new clean code approved and brought into Core? Testing & screenshots. What's the best way to help move this ahead?

@morten I did a cursory review of #32 and it looked good to me. I am not qualified though to give this a RTBC go-ahead =).

Markup looks good, meshes with the new CSS/markup standards. Patch needs to be redone to remove various @todo's within the patch.

New patch looks good. Assuming testbot likes it as well, RTBC.

Adding NYCcamp2013 tag

A couple points of inconsistency, such as in install-page.html.twig:

compare

{% if title %}<h1 class="title" id="page-title">{{ title }}</h1>{% endif %}

with

<div id="content" class="column">

ID's should come first, then classes, yes?

Apart from the new issue in #40, this looks good.

edit: just waiting for testbot to clear. Otherwise, looks RTBC to me.

+++ b/core/modules/system/templates/maintenance-page.html.twigundefined
@@ -12,80 +12,73 @@
+      <div class="l-messages">

I'm not sure why we're using a layout-only class for messages here. Can someone fill me in? I can't find any CSS that styles #messages or .l-messages in system or stark.

+++ b/core/themes/stark/css/layout.cssundefined
@@ -9,129 +9,131 @@
+  body.two-sidebars .l-sidebar-second {

Do we need the body declaration in the selector?

+++ b/core/themes/stark/css/layout.cssundefined
@@ -9,129 +9,131 @@
-  #sidebar-first {
+  .l-sidebar-first {
...
-  #sidebar-second {
+  .l-sidebar-second {

We could use a common class here like l-sidebar to cut down on repetitive CSS. But also see:

+++ b/core/modules/system/templates/page.html.twigundefined
@@ -110,40 +101,58 @@
+    <div class="l-content">
...
+      <aside class="l-sidebar-first" role="complementary">
...
+      <aside class="l-sidebar-second" role="complementary">

I'm not sure if l-content and sidebar makes sense here. Should they be more generic? If I wanted to add a background colour to either of these columns I wouldn't naturally think to add another class, but I would have to if I wanted to conform to our CSS standards.

What about something like l-column or l-major/l-minor?

That's fine with me, we can have class naming discussions in a follow up

Looks good to me, and bot likes it. Marking as RTBC.

Actually marking RTBC.

added info about main tag + removing the wrapper for messages

A couple small things:

+++ b/core/modules/system/templates/html.html.twigundefined
@@ -36,11 +36,9 @@
-    <div id="skip-link">
       <a href="#main-content" class="visually-hidden focusable">
         {{ 'Skip to main content'|t }}
       </a>
-    </div>

This patch shouldn't be touching html.html.twig.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.phpundefined
@@ -115,7 +115,10 @@ function testThemeSettings() {
-      $elements = $this->xpath('//*[@id=:id]/img', array(':id' => 'logo'));
+      $elements = $this->xpath('//header/a[@rel=:rel]/img', array(
+        ':rel' => 'home',
+        )
+      );
       $this->assertEqual((string) $elements[0]['src'], $expected['src']);
     }
     $unsupported_paths = array(
@@ -163,7 +166,10 @@ function testThemeSettings() {

@@ -163,7 +166,10 @@ function testThemeSettings() {
     $uploaded_filename = 'public://' . $fields[0]['value'];
 
     $this->drupalGet('');
-    $elements = $this->xpath('//*[@id=:id]/img', array(':id' => 'logo'));
+    $elements = $this->xpath('//header/a[@rel=:rel]/img', array(
+        ':rel' => 'home',
+      )
+    );

The first one of these should be indented like the second one (the rel line), and the second last parenthesis on both should have a trailing comma per http://drupal.org/coding-standards#array.

Edit - Like this:

+    $elements = $this->xpath('//header/a[@rel=:rel]/img', array(
+        ':rel' => 'home',
+      ),
+    );

Reviewed with dreditor and coder. Everything looks golden. Great job @mortendk!

@Cottser's comment in #48 is inaccurate. This would leave a trailing comma right before the signature closure (ie: for another parameter in the $this->xpath() method). Patch in #51 is correct and RTBC :)

Good point @Mark Carver, my mistake! Follow-up "evil eyes" review below ;)

Also, has this been manually tested? CSS/markup changes need manual testing per https://drupal.org/core-gates#testing.

+++ b/core/includes/theme.incundefined
@@ -2763,10 +2763,6 @@ function template_preprocess_page(&$variables) {
-      '#attributes' => array(
-        'id' => 'main-menu',
-        'class' => array('links', 'inline', 'clearfix'),
-      ),

@@ -2777,10 +2773,6 @@ function template_preprocess_page(&$variables) {
-      '#attributes' => array(
-        'id' => 'secondary-menu',
-        'class' => array('links', 'inline', 'clearfix'),
-      ),

+++ b/core/themes/bartik/bartik.themeundefined
@@ -40,6 +40,7 @@ function bartik_preprocess_page(&$variables) {
   }
   if (!empty($variables['secondary_menu'])) {
     $variables['secondary_menu']['#attributes']['id'] = 'secondary-menu-links';
+    $variables['secondary_menu']['#attributes']['class'] = array('links', 'inline', 'clearfix');
   }
 
   $site_config = config('system.site');

This removes the 'links inline clearfix' classes from both menus but only adds the classes back to Bartik's secondary menu. Can this change be backed up? I thought we didn't want to change Bartik/Seven markup in these issues.

+++ b/core/modules/system/templates/page.html.twigundefined
@@ -64,39 +64,30 @@
-      <div id="name-and-slogan">
+      <div class="name-and-slogan">
...
+      </div>{# #name-and-slogan #}

The closing comment here should be /.name-and-slogan I think to be consistent with the others.

Went through everything again, the template files look great.

+++ b/core/modules/system/templates/page.html.twigundefined
@@ -108,41 +105,55 @@
+      {% if page.highlighted %}
+        {{ page.highlighted }}
+      {% endif %}
...
-      {% if page.highlighted %}<div id="highlighted">{{ page.highlighted }}</div>{% endif %}

Since we no longer have a wrapping div around highlighted or tabs I think we can drop a couple if's to clean things up further.

Also fixed a very minor whitespace discrepancy inside a Twig comment.

Tag monster eats tags.

#58: page-stark-2011578-donedonedone.diff queued for re-testing.

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -423,7 +423,7 @@ function testUrlLanguageFallback() {
-    $fields = $this->xpath('//p[@id="site-name"]/strong/a[@rel="home" and @href=:url]', $args);
+    $fields = $this->xpath('//strong[@class="site-name"]/strong/a[@rel="home" and @href=:url]', $args);

Just remove the second strong/ tag in the testUrlLanguageFallback xpath and I bet you this would work;)

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -423,7 +423,7 @@ function testUrlLanguageFallback() {
-    $fields = $this->xpath('//p[@id="site-name"]/strong/a[@rel="home" and @href=:url]', $args);
+    $fields = $this->xpath('//strong[@class="site-name"]/a[@rel="home" and @href=:url]', $args);

Rerolled with @joelpittet suggestion of removing second /strong tag.

Looks good to me. All issues have been addressed. Patch is green, good to go for RTBC

#64: page-stark-2011578-64.patch queued for re-testing.

Okay... let's do this. HTML5 here we come.

Committed cc7c70e and pushed to 8.x. Thanks!

There should be one change notice to cover all the issues that are part of #1980004: [meta] Creating Dream Markup

+++ b/core/modules/system/templates/maintenance-page.html.twigundefined
@@ -12,80 +12,69 @@
-<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="{{ language.langcode }}" lang="{{ language.langcode }}" dir="{{ language.dir }}">

Sorry, I was away while this patch got RTBC'd. Is this line correct?

This patch seems to mirror the current maintenance page, maybe language or more specifically {{ html_attributes }}, is not defined?:

maintenance-page.html.twig

<!DOCTYPE html>
<html>
  {{ head }}
  <title>{{ head_title }}</title>
  {{ styles }}
  {{ scripts }}
</head>

html.html.twig

<!DOCTYPE html>
<html{{ html_attributes }}>
  <head>
    {{ head }}
    <title>{{ head_title }}</title>
    {{ styles }}
    {{ scripts }}
  </head>

Actually, is that another error in the maintenance page, no opening <head> tag?

I guess we don't have tests to check the markup then if this got past the test bot?

Yep, Output below using stark:

<!DOCTYPE html>
<html>
  <meta charset="utf-8" />
<link rel="shortcut icon" href="http://d8dev.local/core/misc/favicon.ico" type="image/vnd.microsoft.icon" />
<meta name="Generator" content="Drupal 8 (http://drupal.org)" />

  <title>sSite under maintenance | d8dev.local</title>
  <link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.module.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.theme.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.admin.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/modules/system/css/system.maintenance.css?mrmlc7" media="all" />
<link rel="stylesheet" href="http://d8dev.local/core/themes/stark/css/layout.css?mrmlc7" media="all" />

  
</head>
<body class="maintenance-page in-maintenance no-sidebars">

@LewisNyman, let me know if you are happy to go with the <html{{ html_attributes }}>, as per html.html.twig and I'll produce a new patch since the previous patch already got added. I'm not entirely sure if <html{{ html_attributes }}> contains anything at this point, when I tried it didn't output any info but probably good practise if there are attributes in scope.

Soulston, you're right. The maintenance page is missing a head tag. Probably oughta fix that...

Yay, six-character patch!

I can confirm that the patch in #71 adds back the missing head tag. Good catch guys!

D'oh. :P

Committed and pushed that to 8.x. Good catch! Thanks!

Moving back to active for the change notice... I think the html.attributes stuff should probably be discussed in another issue though; it seems like a less straight-forward fix that probably requires some discussion and this sucker's already ~75 comments.

Thanks, Angie -- I've opened a new issue at https://drupal.org/node/2011578#comment-7762747 to clean up the last bit of this.

The issue opened by @eatings is #2067915: Restore html attributes to maintenance-page.html.twig :)

#2083415: [META] Write up all outstanding change notices before release

...

Writing change notice...

What do you think about this?

Change notice

Markup for Stark's page.html.twig + maintenance-page.html.twig

Summary

Stark is bloated with unnecessary markup & css that makes it hard for new themes to create the markup & css structure thats needed. Cleaning up Stark will not mean that we remove the markup structure & classes for Bartik.

Changes include:

• Menu attributes removed from Stark and added to Bartik's template.php.
• #id's removed if they are useless.
• Layout css follows the css layout rules.
• maintenance-page.html.twig includes html5 & follows the same structure as page.html.twig.
• <div role="main"> got the appropriate tag <main role="main">

Before
<div id="main" role="main" class="clearfix">
After
<main role="main">

• Messages wrapper div removed as it's not necessary.

Before

{% if messages %}
  <div id="messages">{{ messages }}</div>
{% endif %}

After

{{ messages }}

Before

<p id="site-name"><strong>
  <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
</strong></p>

After

<strong class="site-name">
  <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
</strong>

Change notice ready for review ;)

I discussed briefly with @rteijeiro on IRC that the change notice should be created to encompass all markup changes and we can just keep adding to it/updating it, as mentioned by @alexpott up in #67. For now this might be as simple as giving it a more generic title.

So we can add this initial change notice, then for example add information about the changes in #1982256: Clean up html.html.twig markup.

(double post)

That sounds like a "needs work." But please just go ahead and create the change notice, rather than leave it in a comment. Even if it's not 100% perfect, it's still better than searching and not finding it. :)

Ok, just added Change notice here: https://drupal.org/node/2155761

Sorry if it's wrong but it's my first change notice :)

Thank you @rteijeiro! I made a few revisions, mainly to note that this wasn't about converting markup to Twig but about cleaning up markup. Converting markup to Twig already happened in #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch and is ongoing in #1757550: [Meta] Convert core theme functions to Twig templates :)

Also I really want to add a link to this, can someone please do so? It feels wrong to say it follows a set of rules and not reference those rules :P

Layout CSS follows the CSS layout rules. (source?)

Done