As per title, probably changes and discussion here is going to pend on usage of the new sectioning and grouping elements such as <header>, <hgroup> and <footer>. Screenshot added.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dcrocks’s picture

Besides the system module bartik, garland, and seven all have a page.tpl.php, as well as the overlay module having overlay.tpl.php, a page.tpl.php substitute. The latter probably needs a substitute issue but I'm assuming the 3 basic themes will be mirrored in 8.x. Should they each have their own issue?

Jacine’s picture

There are surely differences between them, and so the versions will be different too, but I think they can be handled in one patch. We should concentrate on modules/system/page.tpl.php first and go from there.

rupl’s picture

Assigned: Unassigned » rupl

I'll be attacking this at BADCamp. Rawk.

Jacine’s picture

Issue tags: +sprint
rupl’s picture

FileSize
6.54 KB

Posting my progress so the BADCamp sprinters can pick it up if necessary ;) See you guys in a bit!

Jacine’s picture

just an FYI that @chrispomery is working on #1189822: Convert maintenance-page.html.twig to HTML5, which is related.

scor’s picture

Status: Active » Needs work
+++ b/modules/system/page.tpl.php
@@ -116,9 +116,9 @@
-    <div id="main-wrapper"><div id="main" class="clearfix">
+    <div id="main" class="clearfix">
 
-      <div id="content" class="column"><div class="section">
+      <section id="content" class="column">

did you mean to take off the wrapper elements here? Two divs get replaced with one div/section? In other words, was there no reason to have two div elements before (one of them being the wrapper)?

rupl’s picture

@scor as I understood it, many of the wrappers are in place to cater to older browsers like IE6. Since we're not supporting IE6 in D8, I dropped the redundant wrappers without consulting anyone :)

When I popped in to the summit, I heard everyone discussing this topic, but unfortunately I couldn't stick around. If someone down there could post the consensus then we'll know how to proceed for all of these tpl re-writes.

aquariumtap’s picture

Looks good and it applied for me. I saw #main-wrapper and #page-wrapper divs were removed. Catering to IE6 isn't necessary, and a theme like stark that inherits the system's page.tpl.php shouldn't have any excess div tags for imagined use cases.

Jacine’s picture

+++ b/modules/system/page.tpl.phpundefined
@@ -69,7 +69,7 @@
+    <header><section class="clearfix">

Hey, so why do we need these nested sections? I think those can all go... Doesn't make much sense.

-19 days to next Drupal core point release.

zachattack’s picture

Rupl,

This is sort of a rough first pass based on some ideas I have been having about how to re-craft the default page output. I think that Drupal template by default should not be so markup heavy and also if I had more time to talk to you about this, I think that leveraging the renderable array system introduced in 7 to generate the default appearance of things like site_slogan etc.. In short many of the default items should only be printed without needing to wrap them in tags like h1's and links.

Again, the reason I am making this suggestion is that the default output doesn't need to be so feature rich as developers and users will download other themes to expand upon the features of the site.

I know this is not a patch but I wanted to get you my feedback and thoughts in the most expeditious way I could.

http://pastie.org/private/knce02r79at0fixu0ssgea

Best from AUSTIN

Z

aquariumtap’s picture

It might be easier to list out the differences between the patch in #5 and the pastie from @zachattack.

The zachattack version has:

  • New <hgroup> around title and slogan. Title and slogan no longer has ID selectors #site-name and #site-slogan.
  • Site slogan is <h2>.
  • A single sidebar, instead of two.
  • <aside> tag instead of <sector> for sidebars.
  • <sector> wraps the entire page. Does not use <sector> to wrap regions.
  • The logo is wrapped in <figure>.

Personally, I think the patch as it exists is fine, but the first two bullet points seem useful.

The render() approach will probably have to happen in a larger, meta sort of way. There doesn't seem to be any consensus on whether this was a good idea in D7, or how it should be approached in D8. I'm very interested in that conversation as well :)

rupl’s picture

FileSize
9.76 KB

Ok. I have taken feedback from @zachattack, @aquariumtap, and the gang at BADCamp and came up with the following:

  • I discarded <section> as a generic wrapper around the page.tpl.php markup after reading that sections should generally represent records in a database. Page template encapsulates much more, so <div> it is.
  • I liked the idea of <aside> for blocks, but Jacine provided a compelling argument that the region wrappers are generic, and might not always represent asides that are related to content. In a blog, yes, they do. But not always.
  • Wrappers are a no-go for system.module, even if we keep them in other core themes.
  • No <hgroup> because Hixie is probably pulling it out of HTML5 (thx Jeff Burnz). If it stays in the spec, we can add it later.
  • Leaving second sidebar. This issue is to convert the template to HTML5, not alter its general content.
  • Removed slashes from closing HTML comments for increased readability.

Patches and cream!

aquariumtap’s picture

Status: Needs work » Reviewed & tested by the community

Great! And blocks could always get their <aside> wrappers from block.tpl.php at some future point.

I looked over the patch and applied it. Marking RTBC.

Nice meeting everyone today!

chx’s picture

I have my doubts. Doesn't the shiv need to come first?

catch’s picture

Status: Reviewed & tested by the community » Needs review

I'd like to see a couple more reviews before this goes in, as well as get the shiv question answered.

Jeff Burnz’s picture

Status: Needs review » Needs work

General obeservations and thoughts, for discussion mainly, I am tending to think we can do better with more time and iterate this.

  • Roles for header and footer (banner and contentinfo respectively), I think we can do this
  • If we have a role, do we need an ID, for example on main
  • article as the wrapper for columns should be a div, its a style hook (which possibly could be removed)
  • Why do we special case some regions and not others with <div id=""></div>, for example sidebars and footer have wrappers, but header and highlighted do not- do we even need these wrappers, if you think so, what for?
  • CSS is not really accounted for here, are we worried about this, i.e. D7 stark has all the inner div's with section class that provides some default margin, this is not accounted for in layout.css
  • Is div for the site slogan the best we can do? What about p?
  • Is a div wrapper for the site name the best we can do - I question if this borders on being a presentational tag in this context, because afaikt all its doing is providing a non-semantic block level element, can we just use strong and display: block; or should we use p tag with strong inside?

With regards to the shiv, its sort of a yepno type answer - if we are not using an html5 specific tag for styling purposes we are safe, if we do, then ideally we need the shiv. So, what I think is that "no shiv" should not hold up core template conversions until such time as we really need to start caring about design/style, which is not now IMO - we know once that shiv goes in that small style things like some default margin in page.tpl.php is going to be fine (its going to apply in < IE9), however for much bigger changes such as a responsive Bartik or Seven, then yes, we absolutely need the shiv - so that is the point at which it will start blocking. This is my opinion at least on that matter.

chx’s picture

I think screenshots (even just attached not even inlined) would go very far with the "more reviews". Especially from IE which ... few of us... have.

Jeff Burnz’s picture

Yellow = header region
Pink = highlighted
gray = sidebars

The screenshot is in FF7.

I coloured in these particular regions to show the gutters are not accounted for in this patch, but which start originally did have due to the following CSS which now does nothing:

.section {
  margin: 10px;
}

Layout wise this looks the same in IE, with element differences due to no reset in core or Stark.

rupl’s picture

Status: Needs work » Needs review
FileSize
12.11 KB

Agreed on taking more time for this. This template definitely needs more discussion and testing. Here's another round:

  • Roles for <header> and <footer> added. Great catch.
  • I think we should consistently apply IDs along with roles. IDs are a very common and performant method of both styling and accessing DOM. We shouldn't remove that ability.
  • Regarding <div> wrappers for some regions and not others: The wrapper divs for the columns are the only element demarcating that particular piece of content, while the $header is implicitly wrapped by <header>. I can also see the value in explicitly wrapping all regions, so if anyone else has an opinion, please weigh in.
  • Jacine says CSS is a separate issue.
  • Site slogan and site name now have <p>. Good idea.
  • Replaced <article> with <div> for content wrapper. Consensus between front-end people is to push semantics to node or block scope.

Regarding shiv: This in no way affects Stark. It doesn't ship with any styles that hit these tags, plus we're fixing CSS in another issue. This should not block our HTML5 issues. I guess gutters fall into the "CSS later" issue too.

Regarding screenshots: hmm. Stark is stark. Any styles and rendering will come from another place in Drupal. There will always be rendering differences and if someone is actually running a website using Stark, it seems like the visual presentation of the website is a low priority. If consensus is that we really need screenshots to show these minimally-styled markup changes, what should we be showing in them?

adnasa’s picture

Status: Needs review » Needs work

If we have a role, do we need an ID, for example on main

I still think that we need an id or class attribute even when adding a role attribute.

catch’s picture

While css might be a different issue I'd prefer it if we didn't leave completely redundant css in core because things are going out of sync. If there's a guaranteed way that won't happen then all well and good.

Jacine’s picture

There is some misunderstanding going on here. I thought we were talking about something else, so sorry about that. Let me try to clarify: CSS that we are directly affecting in any given patch needs to be updated along with the patch. We can't break stuff or leave in garbage code or we may never find it again.

Here's what is separate:

  1. Testing this stuff in IE and providing working screenshots for that. We need #1077878: Add HTML5shiv to core to land before that is possible on a per-issue basis. And of course that needs #865536-136: drupal_add_js() is missing the 'browsers' option, which is almost ready.
  2. The CSS cleanup issues are separate. That's where major CSS refactoring will happen. However, like we've done for previous patches, if the CSS is directly affected by the markup changes, it should be adjusted to prevent breaking things (think quick fixes).

If people really want to see side by side screenshots where nothing changes, that can be arranged, but I worry that in most cases it's probably more likely to confuse people than to actually be helpful.

Hopefully that clarifies things?

Patch review:

  1. Why are there 4 versions of the patch in the one patch file? It took me a good 5 minutes to figure out there was more in there, and I almost posted a patch review on the first version. I still can't do a quick review in Dreditor because it's showing changes from one patch to another. Can you please post a regular patch next time?
  2. Can we please just use #header, #footer instead of #banner and #contentinfo?

Thanks! :)

Jacine’s picture

BTW, there's an issue to tackle the default display properties for these that is RTBC so obviously that part should not be handled in this patch: #1321678: Provide default styling for block-level HTML5 elements.

rupl’s picture

Status: Needs work » Needs review
FileSize
4.72 KB

ARIA roles

banner and contentinfo are Landmark ARIA roles which carry explicit meaning, so they are more appropriate than header and footer, which aren't included in the allowed values for ARIA roles. I had kept the HTML IDs the same as the role if present, but I can see where that may be confusing. This patch has IDs matching the tags instead of roles.

I also added the heading role to the h1. I'm not entirely sure if this is correct, but I didn't see anything advising against roles on heading tags.

Wrappers

I don't want to add wrappers back in just for .section, so for this patch I removed the CSS rule Jeff pointed out in #19. If someone feels strongly about having wrappers for those 10px margins please chime in.

Ok here's another patch, this time just a straight diff:

rupl’s picture

FileSize
4.69 KB

Added the "navigation" role for <nav>, removed "heading" from <h1>.

mgifford’s picture

Issue tags: +aria

Patch looks great. @rupl thanks for adding ARIA. Agreed that the H1 is better without the "heading".

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Hmm, so this is ready from a markup standpoint IMO. The only outstanding issue is that the way the regions are being printed is completely buggy, e.g. an empty <div> always prints for tabs, etc. However, that's technically a separate issue. There's been a lot of discussion on it so far, but mostly for a solution that works for D7 too, so it shouldn't hold this up. So, I'm going to go out on a limb and mark this RTBC. :)

That issue is here: #953034: [meta] Themes improperly check renderable arrays when determining visibility for anyone interested.

Jacine’s picture

BTW, here are before and after screenshots.

catch’s picture

Issue tags: -markupmarines, -aria, -html5, -sprint

#26: 1077578-page-tpl-26.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +markupmarines, +aria, +html5, +sprint

The last submitted patch, 1077578-page-tpl-26.patch, failed testing.

rupl’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

Rerolled following /core directory commit!

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

That was quick. Thanks @rupl. ;)

catch’s picture

Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -sprint

Committed and pushed to 8.x, thanks!

Needs a change notification I think, or possibly adding to http://drupal.org/node/1328756, not sure.

aspilicious’s picture

Title: Convert page.tpl.php to HTML5 » Change notification: Convert page.tpl.php to HTML5
aspilicious’s picture

Issue tags: +sprint

Changing tag

Jacine’s picture

Hmm, do we really need a change notification for every template we touch? Isn't it assumed that we break themes from major version to major version? There's no functionality or variable name changes here, just markup. I think the fact that we have an HTML5 initiative implies that we are changing markup in these templates. The html.tpl.php was a different story because we did make significant changes to the attributes and stuff. So, I don't know, but it would be good to set some guidelines for this stuff.

Everett Zufelt’s picture

From #1183042-28: Regression: Add WAI-ARIA roles to Core blocks

"I believe role="main" should go in the page.tpl and maintenance-page.tpl because the page content's title is in those templates and is part of the "main" content."

With which I agree. Can we open a followup issue to make this small template change and then rol that into this change notification please?

Jacine’s picture

@Everett Zufelt It's already there. See line #118 of page.tpl.php.

catch’s picture

Priority: Critical » Normal
Status: Active » Fixed

[#1328756] might well be enough and we can just let that get updated as time goes on. Marking back to fixed then :)

catch’s picture

Having said that did we send out a /core announcement for the html.tpl.php patch? That is quite big news and worth spamming people :)

Jacine’s picture

mgifford’s picture

@Jacine just wondering about the order here: <div id="main" role="main" class="clearfix">

Is it documented that it's id, role & then class? Should it be? I just asked as I was just proposing it in a different order for a different patch.

It doesn't matter, other than that consistency makes it easier to read and more professional.

Jacine’s picture

@mgifford We don't have any rules for order of attributes ATM. That'd be hard to do in some places, but in cases like this where we're hard-coding them, we could certainly have coding standards in place for that. If you created an issue for this already, just link it back here and tag it "coding standards."

catch’s picture

Title: Change notification: Convert page.tpl.php to HTML5 » Convert page.tpl.php to HTML5

@Jacine and I had seen that too, but it was so long ago!

Status: Fixed » Closed (fixed)

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

Snugug’s picture

FileSize
4.75 KB

Identical to patch in comment #32, but added a clearfix class to the tag, as it seemed to be the only item that lost a class from the nested

that didn't have one added to the new single tag.
mgifford’s picture

@Snugug - going to change the Status?

Snugug’s picture

Status: Closed (fixed) » Needs review

Um, yah, about that… woops! Done.
Again, don't quite know if this is 100% needed, and I realize it's a bit of an old thread to be resing, but I thought it was strange as I was backporting for the HTML5 Theme

aspilicious’s picture

this patch is alrdy fixed and in core....

???

Snugug’s picture

Status: Needs review » Closed (fixed)

Ya know what. sorry, you're right. Was in D7 land yesterday and new to Core contrib so that's the whole patch instead of just the part I wanted to update. Cloning into D8 now and will reroll and make a new thread.

mjohnq3’s picture

Assigned: rupl » Unassigned
Status: Closed (fixed) » Needs review
FileSize
947 bytes

Add the HTML5 element aside to sidebars.

Several years ago the W3C revised the specification to allow aside to be used for secondary content: "...a section of a page that consists of content that is tangentially related to the content around the aside element, and which could be considered separate from that content. Such sections are often represented as sidebars in printed typography."

HTML5 Doctor explains the use more fully.

Several widely used contrib themes; Zen, Boilerplate, Aurora, Sasson, Boron, and, Basic, are already using aside for sidebars.

(The attached patch may fail the testbot but I'll deal with that when it does.)

Anonymous’s picture

Status: Needs review » Active

Since there was already a patch committed for this issue, you should create a new issue that describes what you want to update and post the patch there. You can reference this issue from the new issue that you create.

mjohnq3’s picture

The last time I tried that I was asked to just re-open the original issue. #1880202: Add ARIA landmarks for Bartik theme (con't)

mjohnq3’s picture

Status: Active » Needs review
mgifford’s picture

I don't know if we need a 2013 reference for <aside> but the article you referenced was from 2009. How's adoption going? We're reconsidering <main> so just want to make sure that this still has good support.

Also, the patch has to deal with the shiv's for backward compatibility, right?

mjohnq3’s picture

All browsers, with the exception of IE 8 and lower, provide support for the aside element. http://caniuse.com/#feat=html5semantic

The html5shiv, including the one provided with D8, supports the aside element.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
341.76 KB

Well, I tested the patch in simplytest.me and Opera, FF, Chrome & Safari in the Mac. It could probably due with testing in IE too, but It's a simple change and seems to be supported.

Screen shot with aside

xjm’s picture

#52: 1077578-page-tpl-sidebar-52.patch queued for re-testing.

xjm’s picture

Title: Convert page.tpl.php to HTML5 » [Followup] Convert page.tpl.php to HTML5
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Hmm, I'm not sure we want to put this in without testing IE 8/9. :)

mjohnq3’s picture

FileSize
57.32 KB
72.84 KB

Patch in #52 causes no visible problems in IE8 or in IE9. See attached.

mgifford’s picture

But does IE8 always look this bad https://drupal.org/files/aside-ie-8.jpg

mjohnq3’s picture

This issue doesn't actually concern Bartik but since it's the default theme, what you see is what you get. IE8 doesn't support media queries so you always get the "mobile-first" layout; content stacked over the sidebars.

At one time it was determined that we wouldn't be adding add various polyfills to support media queries and related items (CSS3, etc.) in older browsers. I'm not sure if that's still the case as D8 now ships with things like Moderizr, html5.js, etc.

xjm’s picture

But does IE8 always look this bad https://drupal.org/files/aside-ie-8.jpg

Yep, only a single-column layout is supported for IE8 in core themes:
http://drupal.org/node/1722502

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Ok, then I'm putting this back to RTBC.

mgifford’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.73 KB

Talking with @Lee Rowlands about WAI-ARIA roles, i came across this recent documentation about HTML5 & WAI-ARIA:
https://dvcs.w3.org/hg/aria-unofficial/raw-file/tip/index.html#what-does...

They recommend using role="complementary" with <aside> as "the default semantics are not implemented across browsers, so the default implied role, state, property, or suggested semantics (if no ARIA default) may be used."

mgifford’s picture

FileSize
1.7 KB

I think this is the missing interdiff.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Assuming bot is ok, last patch only adds additional aria landmark roles

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Wait. I'm a little confused by #66. At the top of the patch, we're making things asides, but we're not adding the complementary role. Is that a bug? Or why do we do it in the footer and not the sidebar?

mgifford’s picture

That was an oversight. Not sure how I missed core/modules/system/templates/page.tpl.php when building #66. Sorry.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs reroll

Needs a reroll now that page.tpl.php is page.html.twig.

Edit: Not sure what to do with the issue title here :/

mortendk’s picture

i have just looked at this reroll and it seems to not follow at the principles that we have agreed upon by keeping the markup to a minimumetc.
- The plan is to keep stark as clean as possible & build bartik to be a basetheme, but those princippels grew out of the Twig initiativ :)
https://drupal.org/node/2008464#principles

give bartik its own branch to make it a strong starter theme
i would suggest that bartik is getting its own issue & that we fix stark first. then build on top of that - Bartik needs an epic reroll both in its markup and css, so it can become a truly good starter theme.

aside for the sake of aside ?
I must admit that i dont agree with the use of asides as defaults sidebars, its ment for "related content but not as related to the main article on the page".
Are we assuming that everything (and always) in the sidebars is related to the article. that is a thing we honestly dont know, untill we have content.

i would argue that a login field, a feed from a nother site, banner adds etc isnt related to the article but to the overall site and therefor they should be default be a div (i cant belive i just argued for using a div ;) So by forcing on the sexy "new" aside tag, we are misinforming by default. If a user wants aside it should be easy for him/her to change it in the markup. Lets not make up assumptions about the data.

I have rolled a patch that takes starks page.twig + the maintainence-page.twig and moves the secondary menus in the preprocess to bartiks.theme
#2011578: Markup for Stark's page.html.twig + maintenance-page.html.twig dunno if that should be added into this issue or ?

mgifford’s picture

Just looking at the patch you provided in #2011578 and realized that you hadn't included role="complementary".

That's actually one of the more prominent pieces of the patch in #70 above.

The HTML that needs to be added to make web pages accessible is changing. Limitations in screen reader software & inevitable delays in user behaviors means that to meet the WCAG 2.0 AA guidelines with Drupal 8 there is going to be some redundancy.

With HTML5 & WAI-ARIA more semantic meaning is being passed through to the screen reader. Some of the elements that were added in Drupal 7 are needed because it was the only way for screen reader users to navigate between sections of a page.

There is some overlap between the semantic information provided by HTML5 & WAI-ARIA. There is at least one person who has argued that <aside> should have default implicit ARIA semantic of "complementary". Sadly that thread didn't have any public follow-up.

So, I think everyone likes clean, uncluttered code. Unfortunately, that can present accessibility problems if it isn't carefully considered.

pwieck’s picture

Status: Needs review » Needs work
FileSize
2.51 KB

Changed to twig files with an issue besides above comments:

The <aside> tag is used differently throughout patch and HEAD twig files. Was this intended?

  • <aside> alone with removal of <div> tag.
  • <aside><div>
  • <div><aside>
pwieck’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Status change

pwieck’s picture

Status: Needs work » Needs review

#74 Passed and ready for review, testing, or rework.

mortendk’s picture

Status: Needs review » Needs work

looking at the principels which were trying to build the 2 base themes for D8 this patch do try to add more than neede into stark it still adds classes thats not needed into stark and dosnt add em to bartik.

see https://drupal.org/node/2008464#principles

leslieg’s picture

The last submitted patch, 70: 1077578-page-tpl-sidebar-70.patch, failed testing.

The last submitted patch, 70: 1077578-page-tpl-sidebar-70.patch, failed testing.

Jeff Burnz’s picture

+++ b/core/modules/system/templates/page.html.twig
@@ -129,15 +129,15 @@
+      <aside id="sidebar-first" class="column sidebar" role="complementary">

Why are certain regions special cased? This makes no sense, it's old school/IE5/pre-mobile/pre-html5 thinking.

#73, as per #72 it's not possible to assume content relationships at the region level in a generic template. It's merely speculation that content appearing in "sidebar" regions bear a complementary relationship to the main content. The idea that special casing two regions achieves anything meaningful is seriously doubtful and needs to be questioned.

mgifford’s picture

Issue summary: View changes
Issue tags: +Needs reroll
visabhishek’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

For core/modules/system/templates/page.html.twig changes are already committed.

So except that re-roll are

Risse’s picture

Issue tags: +drupalcampfi
FileSize
1.61 KB

Last patch seemed to be missing the last quote in 'role="complementary"'

sqndr’s picture

Looks like a good patch!

Nikolay Shapovalov’s picture

diff --git a/core/themes/bartik/templates/page.html.twig b/core/themes/bartik/templates/page.html.twig
...
+    <aside id="featured" role="complementary"><div class="section clearfix">
       {{ page.featured }}
     </div></aside> <!-- /.section, /#featured -->

Is it good idea to change order of div and aside?
Because in all other places in page.html.twig first goes div and then aside.

Nikolay Shapovalov’s picture

Patch with changed order of div and aside in #featured.

mortendk’s picture

May i ask why there still used ID's here ?
Its badpractice & all that jazz

Besides of that this patch makes little sense afaik ? this is a bartik patch the page.twig.html is fixed, so bartik should follow that template

dcam’s picture

Issue tags: -Novice, -sprint, -Needs reroll, -drupalcampfi

Updating in prep for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....

Removed old tags. The Novice tag was added in #71 because the previous patch needed a reroll, which was done in #74.

mgifford’s picture

Status: Needs review » Needs work
FileSize
5.23 KB

@mortendk - I'm definitely not a themer but trying to help nudge this issue along.

Wanted to see if this was closer to what you were looking for with the HTML.

I used meld to compare it with core/system/templates/page.html.twig and this is what I came up with.

It's definitely more than just an HTML5 conversion, but it looks a bit more consistent.

It's going to totally mess up the CSS though. Figured I'd get clarification on the HTML5 first.

star-szr’s picture

Component: system.module » markup

I think the worthwhile changes to Bartik might be adding the ARIA roles but not sure anything else will be beneficial. The consensus in the recent past (~2 years) seems to be that Bartik is not worth saving as far as markup cleanliness.

This either needs a re-title and issue summary update or (better IMO) to be moved to a new, focussed issue and this one finally closed out. This "followup" has gone on for over a year now with no commits (see #52 / #53 where I agree with @linclark that a new issue should have been created).

mgifford’s picture

@Cottser - agreed. So let's deal with the bigger followup issues that @MortenDK mentioned here #2286371: [Followup] Clean Up Markup for Bartik

So back up to @zniki.ru's post we'll focus on adding in ARIA.

Wanted to check in about this switch of div/aside.

-    <aside id="featured"><div class="section clearfix">
+    <div id="featured"><aside class="section clearfix" role="complementary">

It's more standardized for sure.. Seems fine to me.

Can we get #87 RTBC?

Niklan’s picture

Hello there.

page.html.twig conains

 <main role="main">
     ...
  </main>

But w3c says:

It should not contain any content that is repeated across documents such as sidebars, navigation links, copyright information, site logos, and search forms.

For now (beta1) main tag contains sidebars, which contains navigtaions, search forms etc.

mgifford’s picture

Issue tags: +Needs reroll

I think this is a better W3C reference, but it doesn't alter your comment in any way.

This patch doesn't alter the placement of the <main> tag. Although it could. Might be better to be a follow-up issue. This would allow us to focus on moving the page.tpl.php to HTML5.

The patch needs a re-roll and we could look at whether we can clean this up at the same time.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Reroll from #87.

mgifford’s picture

Issue tags: +Accessibility

Nobody has addressed @mortendk point in #88 about use of ID's.

I have added this follow-up issue for the #2361047: [Followup] Move <main> in page.tpl.php to respect HTML5 content used in the <main> tag.

I'm for getting this in now. This patch at this point mostly adds an ARIA role role="complementary". This should insert the semantics into the page to clearly mark that as complementary content.

mortendk’s picture

Title: [Followup] Convert page.tpl.php to HTML5 » [Followup] Convert bartiks page.tpl.php to HTML5

change the issue name so we know its about bartik

Jeff Burnz’s picture

The reason we used ID's in D7 Bartik was because we wanted to build a super-robust theme that would not break easily, no matter what CSS was thrown at it from contrib modules and the like. We felt it was more important to build something reliable and near enough to unbreakable since it was core and lots of sites were likely to use the default theme, as many in the past were using Garland. If our core theme broke under minimal pressure it would not be a good look for Drupal. That was the thinking.

Fast forward to today where that thinking has not been applied and we have breakages - for example Toolbar. It has an ID that could be used to isolate its styles, but it's not used, but this is one place where it should be. Now think about how many CSS related issues are posted against Bartik - virtually none. I hear people complain about Bartiks CSS but they all seem to forget how absolutely robust it turned out to be. Right?

My point is, yes remove them, given the idea that Bartik should become more of a base theme, however, do not make it so lame that it breaks the first time a contrib module does something weird and crazy, which of course, they do.

And fwiw, I do not consider or agree that id's are a bad practice, they can be badly used, but so can any technology. ID's have a purpose and its to give big specificity when required. Its what they were designed for.

I will say this again - yes I support removing them, but with caution against breakages, don't let it break easily, lots and lots of sites are going to use this theme.

mortendk’s picture

yeah gotta be honest i don't really care about bartik ;)

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Ok, in that case.. Let's RTBC this issue.

The last submitted patch, 90: 1077578-page-tpl-sidebar-90.patch, failed testing.

mgifford queued 95: 1077578-95.patch for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/bartik/templates/page.html.twig
@@ -125,9 +125,9 @@
-    <aside id="featured"><div class="section clearfix">
+    <div id="featured"><aside class="section clearfix" role="complementary">

Seems incompatible with the css rules in bartik's layout.css which use #featured div.section

I really wish this followup had had a new issue. The original patch was committed in #34 we are now on comment #103. That's ridiculous.

Jeff Burnz’s picture

lol, yeah, its ridiculous, lets fix the selector, add the roles and be done with this, looking at the layout CSS makes me shudder, I can't bear it again. please, let this be over :/

sqndr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.28 KB
464 bytes
415.4 KB

Changed the layout.css to be compatible with the new selector, as mentioned in #103.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
343.52 KB

Thanks @sqndr - I prefer your JPG, but thought I'd include one that the UX folks could use.

Let's get this in and close this issue.

sqndr’s picture

Woops … Included the wrong file. Sorry about that. Thanks for creating a new screenshot @mgifford!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs reroll

Let's end this. This issue is a unfrozen change as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 7b1e2c1 and pushed to 8.0.x. Thanks!

  • alexpott committed 7b1e2c1 on 8.0.x
    Issue #1077578 by mgifford, mjohnq3, sqndr, zniki.ru, pwieck, rpayanm,...

Status: Fixed » Closed (fixed)

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