Convert Bartik to HTML5

Subscribe.

Should probably wait off on some of the Drupal templates which are still being converted (like #1077602: Convert node.tpl.php to HTML5). Also, extra bonus points if we could get rid of some of the tpl.php files entirely in place for some fancy CSS3. The less code, the better.

Question. Was this issue fixed as a result of #1192044: Convert Bartik's layout to mobile-first and responsive ?

This patch does not include the maintenance-page.tpl.php as that issue is still not resolved: #1189822: Convert maintenance-page.html.twig to HTML5.

Re-rolled patch.

Re-rolled patch against latest pull.

Ok, one template at a time. comment-wrapper.tpl.php is trivial.

Huh? What does this have to do with changing <div> to <section>?

Retest, error has nothing to do with change AFAICT.

#12: comment-wrapper-1179764-12.patch queued for re-testing.

Ok, let's try comment.tpl.php.

#17: comment-1179764-17.patch queued for re-testing.

You will need to change the tests. They are still looking for 'comments' in DIV tags.

I.E.

  function _testBasicCommentRdfaMarkup($comment, $account = array()) {
    $comment_container = $this->xpath('//div[contains(@class, "comment") and contains(@typeof, "sioct:Comment")]');

You have changed to ARTICLE tag.

Ok, this is a pretty important change, but it is going to take someone to invest quite a lot of time in going through all of the *.php files & adjusting all of the affected tests.

It's amazing that even the little comment.tpl.php had 26 failed tests in it. #10 had 49,226 pass(es), 38 fail(s), and 20 exception(s) the last time it was tested.

This does need to get into D8, just not sure who if anyone is assigned to do it. Also might be worth reviewing with #1802566: Add ARIA landmarks for Bartik theme as they should go hand-in-hand.

I also believe the "attribution" should be in a <header> element, whilst the "signature" & "links" should be in a <footer> element. But that is just my personal preference, so ignore me if you think otherwise.

I've got no problem with those suggestions. I'd like to see someone update the patch to deal with the broken simpletests though.

OK I would like to take this on. Will incorporate my suggestion from #23 if no one has any objections. Not sure of the order things need to be done in, will this wait till #1802566: Add ARIA landmarks for Bartik theme gets in, or should I make those changes also here. Would that not then make either one of these a duplicate, seems silly having 2 issues on the same code.

OK here goes, have updated HTML and fixed comment preview test. The previously failing RDFa tests seem to be fixed now, by the new tests not checking HTML tag's directly. Lets see what the bot thinks.

By the way, it looks like i changed a lot more then I did, only because I changed the indentation. I prefer the open rather then compact indentation, as it makes the template more readable for novices.

So have now added test fixes, and also added the new ARIA roles required for such things as 'nav' and 'article'. We may need to close the other issue #1802566: Add ARIA landmarks for Bartik theme as duplicate now.

OK that was a little over zealous, here is revised patch, reverting translation test changes and also reverting action-links to non 'nav' element.

Still expect some translation tests to fail, but fewer of them. Can not see what I have done to make them fail even running tests locally, may need some guidance.

That's the best I can do for now, no idea why the remaining 3 tests are failing. Have tried everything I can think of, if anyone has a clue, note it here, and I will fix it if possible.

Tagging & will call out for more help on this one.

I did not review the patch at all, just rerolled and fix the test.

This included #1802566: Add ARIA landmarks for Bartik theme, which was not a good idea :) But now that's in, so who cares.

Also, for future reference, that's not what the "Needs tests" tag is for.

The changes

+++ b/core/themes/bartik/templates/page.tpl.phpundefined
@@ -87,163 +87,178 @@
-    <div id="messages"><div class="section clearfix">
...
+      <div id="messages">
+        <div class="section clearfix">

Changes like this make this impossible to review :( Is there any chance the indentation and line splitting could be kept separate from the actual changes for this issue?

Thanks for the revised patch & recommendations.

I didn't review the indentation/line splitting, but will look for it in the future.

What is the tag for "Desperately needs help with SimpleTest"? I had thought I'd heard "Needs tests" referenced on Drupal Ladder and thought I'd understood it's proper use..

We can work with the tests you've provided (if needed).

@tim.plunkett thanks very much for your review/test help. Of course it is obvious now about the test. Thanks again. And your comment about the indentation changes has been noted, I do see the reasoning. I made those changes precisely because I could not get my head around the markup to make the HTML5 changes.
Will raise a new issue to correct the indentation, and for now revert this issue back to old indentation for easier review. Also noted about #1802566: Add ARIA landmarks for Bartik theme, a little over zealous there.
Thanks again for your help, I will get the hang of this one day, and stop trying to do everything in one patch.

See #1903048: Revise Bartik template indentation inline with best practices.

oopps lets change the status for now. Will post revised patch ASAP.

OK here is the revised patch, by the way, @tim.plunkett the changes to fix translation errors are no longer needed as they were fixed already in head. Hopefully this should now be green and easier to review.
I will work on the follow up issue when this is in.

Ah yes, #1595028: Convert tests using Standard profile to use Testing profile instead cleaned that up nicely!
Thank you so much for reducing the size of the patch.

I only have one tweak, I think this is good to go now!

Thank for that, silly mistake on my part. OK hopefully this can get in soon, and I will do the follow up. I am hoping to spur on the 'twig' efforts as well.

Unassigning myself to attract reviews.

I think this is good to go. Thanks @tim & @mbrett5062!

Committed to 8.x. Thanks.

I noticed the use of the new html5 semantic element 'main'. I haven't found very much discussion of that element except that it seems to be targeted for html5.1. And I can't find anything about browser support. The element seems to be just ignored.

It is being used in this patch. That's why I commented. But it doesn't seem to have any affect. It seems it is only decorative.