Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Meta issue: #1980004: [meta] Creating Dream Markup
Issue based on: #1961872: Convert html.tpl.php to Twig
Summary
- Removed Region based class names in the body tag from core.
- Region classes in the body tag is now added only if the theme ask for them in its own .theme file (stark now have the stark.theme file like all other themes adding in the class names for a quick 3 col layout).
- The "skiplink" wrapper div is removed from core & added in for bartik as the theme still follow old rules.
More info
- Removed all the sidebar info from the body classes from core!
- The sidebars are just regions & keep having them in default is not optimal. If you theme is build with a sidebar structure, well fine add that into your theme
if not, well woho no more region crap in you body class name. - The sidebars classes are added to seven, bartik & stark as well. Which now has a preprocess. This is to keep "the design" in stark but we dont force a theme thats build from the ground up to use the classes.
Theres a couple of wins:
- Remove the idea of always doing a 3 column layout.
- Remove unused css in body thats not used by your theme when you dont use sidebar first-last etc.
- Show how simple it is to add the into in stark.theme / bartik.theme.
New html.html.twig
<!DOCTYPE html>
<html{{ html_attributes }}>
<head>
{{ head }}
<title>{{ head_title }}</title>
{{ styles }}
{{ scripts }}
</head>
<body{{ attributes }}>
<a href="#main-content" class="visually-hidden focusable">
{{ 'Skip to main content'|t }}
</a>
{{ page_top }}
{{ page }}
{{ page_bottom }}
</body>
</html>
Comment | File | Size | Author |
---|---|---|---|
#74 | html-clean-up-1982256-72.patch | 7.96 KB | ry5n |
#74 | interdiff.txt | 1.96 KB | ry5n |
#59 | 1982256-html-59.patch | 10.72 KB | rteijeiro |
#59 | interdiff.txt | 517 bytes | rteijeiro |
#71 | interdiff.txt | 494 bytes | tim.plunkett |
Comments
Comment #1
oresh CreditAttribution: oresh commentedmoving to core issues.
Comment #2
ry5n CreditAttribution: ry5n commented+1 to scripts at the bottom, but why on earth are they in a wrapper?
Comment #3
oresh CreditAttribution: oresh commented@ry5n,
if there's a need to add more js it's added in that wrapper, and we can keep it cleaner :)
Comment #4
altrugon CreditAttribution: altrugon commentedWe all know the ideal world doesn't exist, there is going to be several scenarios where developer for one reason or another will need to add their JavaScript into the header.
This is proposition is a big limitation under my point of view.
Comment #5
ry5n CreditAttribution: ry5n commented@altrugon Whether it’s above or below, some folks are going to want/need scripts in the other spot. It’s easy to move the print statement in a theme’s html.twig, or add scripts to the head (alter or drupal_add_html_head) in preprocess. Given current best practices (see html5 boilerplate etc.) recommend scripts at the bottom, changing the default placement makes sense to me.
I still have no idea why we’d wrap them in a div though. If you need to add a script to the page from JS, can’t you just document.body.appendChild()? Even if that doesn’t work, you can add the wrapper in a custom html.tpl. I’ve rarely (never) added script tags like this, so unless I’m unusual we should not be adding wrappers like this to default core markup. Goes right against the 'start from nothing' principle.
Comment #6
webthingee CreditAttribution: webthingee commentedI would second ry5n that the we should follow best practice, and place it at the end of the document.
I also agree that there is no reason to wrap it in markup, as it is not a DOM element that should or would need to be managed.
Comment #7
mortendk CreditAttribution: mortendk commented+1 - unless theres a very very very (very) good reason for having a wrapper (or even a class) it should not be there.
Comment #8
LewisNymanI'm a bit concerned about defaulting to all scripts in the footer. There are some scripts that need to go in the header, if you look at HTML5 boilerplate you'll see Modernizr in the header.
It's pretty easy to specify scripts to go into the footer without moving
$scripts
using the scope variable. I'd rather do it there then here.Comment #9
LewisNymanFirst attempt at a patch for this template, testing out our dream mark up process.
Comment #10
LewisNymanComment #11
yannickooWhitespaces
Comment #12
tlattimore CreditAttribution: tlattimore commentedHere is an updated patch with the whitespace mentioned in #11 removed.
Comment #13
star-szrNeeds a reroll:
Comment #14
mortendk CreditAttribution: mortendk commentedRerolled the patch to work with the latest changes
i did stumpled on a little issue if we are removing the layout classes - this is might a meta issue...
Stark & Layout
By removing those now that we don't have any other layout system than block & regions should we add layout specific classes or is this a documentation issue?
if we remove the classes the layout.css file should die as well cause it serve no purpose ?
Comment #15
oresh CreditAttribution: oresh commentedComment #17
mortendk CreditAttribution: mortendk commentedlooking at the last couple of patches:
We cant really remove the layout classes from the body stark, basically cause we have nothing atm to replace em with & we should not end up in a huge bikeshed over names etc.
As i see it we have to let stay there for now, but we can easy clean up the rest of the classname overload in body (.page-node-, page-node-2 etc) those can be placed in bartik or removed completely or added in as documentation.
Comment #18
yannickooSorry guys! The patch was for #2032895: Follow-up use better technique for vertical centering.
Comment #19
yannickooComment #20
mortendk CreditAttribution: mortendk commented@yannickoo does this patch belongs here ;)
Comment #21
mortendk CreditAttribution: mortendk commentedrerolled that patch changed the classname to follow latest changes in core
Comment #22
mortendk CreditAttribution: mortendk commentedbot get to work
Comment #23
mortendk CreditAttribution: mortendk commentedComment #24
tlattimore CreditAttribution: tlattimore commentedThis looking really good to me, @mortendk. I am so glad to see that
<div id="skip-link">
and the.html
gone!Comment #25
mortendk CreditAttribution: mortendk commentedComment #26
markhalliwellLooks great! One small issue:
Is this a typo? If we're not overriding these in the theme, they shouldn't be there (for maintainability purposes). Also, if they are added, then stage them so they are actually part of the patch too.
Comment #27
star-szrWe are adding the "old" templates (and some preprocess) to Bartik and Seven so we don't break markup/CSS there. But this still cleans up core markup and the markup that will be inherited by any contrib themes.
As far as the copy from/to in the patch, I applied the patch locally and it worked fine and created the new files.
Here is my review of the latest patch.
Curious why one-sidebar, sidebar-second, etc. are kept but no-sidebars is removed.
If we're removing this, we need to fix indent of the
<a>
as mentioned on IRC.Remove blank line at the start of the function please :)
I just grepped and Seven is using these classes (search for page- in style.css). So this needs to be added back in seven_preprocess_html(). I didn't see any usage of html, front/not-front, logged-in/not-logged-in or sidebar classes in Seven's CSS. For the record, I also didn't find any front/not-front or logged-in/not-logged-in classes in Bartik's CSS.
Most of this (other than no-sidebars) is already being added in template_preprocess_html() so this actually results in doubled up classes on Bartik:
<body class="one-sidebar sidebar-first front logged-in one-sidebar sidebar-first">
Comment #28
mortendk CreditAttribution: mortendk commentedinspired by cottsers catch about the double classes did some more cleanup ...
sidebars classes
Removed all the sidebar info from the body classes from core!
The sidebars are just regions & keep having them in default is not optimal. If you theme is build with a sidebar structure, well fine add that into your theme
if not, well woho no more region crap in you body class name.
The sidebars are added to both seven & bartik - and for the sake of not making the world explode i have added them to stark as well.
im wondering if we even wanna have "design" in stark ? - right now i dont wanna this overcloud this patch so theres patch create a stark.theme file with the preprocess in.
I think theres a couple of wins here:
- Remove the idea of always doing a 3 column layout.
- Remove unused css in body thats not used by your theme when you dont use sidebar first-last etc.
- show how simple it is to add the into in stark.theme / bartik.theme
Comment #29
yannickoo"AND"?! Use &&
Comment #30
star-szrCool, that's looking much better!
Please change this to say "Implements hook_preprocess_HOOK() for HTML document templates." to be in line with the changes over in #2049207: [Follow up] Replace .tpl.php with .html.twig in documentation.
Comment #31
star-szrCrosspost…
Comment #32
mortendk CreditAttribution: mortendk commentedfixed the AND to && :)
Comment #34
mortendk CreditAttribution: mortendk commentedInspired by cottsers catch about the double classes did some more cleanup ...
NO sidebars classes
Removed all the sidebar info from the body classes from core!
The sidebars are just regions & keep having them in default is not optimal. If you theme is build with a sidebar structure, well fine add that into your theme
if not, well woho no more region crap in you body class name.
The sidebars are added to both seven & bartik - and for the sake of not making the world explode i have added them to stark as well.
im wondering if we even wanna have "design" in stark ? - right now i dont wanna this overcloud this patch so theres patch create a stark.theme file with the preprocess in.
I think theres a couple of wins here:
- Remove the idea of always doing a 3 column layout.
- Remove unused css in body thats not used by your theme when you dont use sidebar first-last etc.
- show how simple it is to add the into in stark.theme / bartik.theme
Comment #35
tlattimore CreditAttribution: tlattimore commentedRe #34:
Nice work here, mortendk. I completely agree with these motivations that the solution here. Having less assumptions made in
template_preprocess_*()
is going to make our markup much cleaner.Can we get an RTBC?
Comment #36
markhalliwellLooks good to me. Several revisions with all issues addressed. Good to go.
Comment #37
yannickooWhy the space after 'page-'? We should remove that ;( Sorry guys.
Comment #38
star-szrNice catch @yannickoo!
Also, these comments are three characters too long, 'page-node-edit' needs to go on the next line…
We should also update the issue summary before RTBC, just showing Stark's markup before/after would be nice.
Comment #39
mortendk CreditAttribution: mortendk commented* me mumbles ... grrr .* oh well here ya go ;)
Doing the update on the issue que as well + a ton of screenshots, on a fresh install for the overlords
Comment #40
dbazuin CreditAttribution: dbazuin commentedLess assumptions for theming in core and moving this to the themes is indeed cleaner. If you need them adding them is easy enough .
You where mentioning on irc "do we need htm.html.twig and pag.htm.twig or can we combine them".
Anybody a opinion about that?
Comment #41
richardj CreditAttribution: richardj commented#40 I would say, keep the html.html.twig and the page.html.twig, the thought behind this is: you put your html and head into html.html.twig so that is kind of a static file, then within that you load all your different page.html.twig files.
I think most framework work with this in mind and personally i don't want to keep track of all my html / head elements in all the different page.html.twig files (as we did in Drupal 6).
Comment #42
yannickooI guess that Morten will say "I give a fuck on Drupal coding standards" to me next time when we'll meet us ;)
Same here.
As mentioned above they need to be go into the next line.
Also this one here.
Comment #43
mortendk CreditAttribution: mortendk commentedheres the screenshots of the changes including on of the source
Comment #44
mortendk CreditAttribution: mortendk commentedi will ;)
Comment #45
mortendk CreditAttribution: mortendk commentedlets put the html.html.twig + page.html.twig into another issue - so we can get this cleaned up first, then take that battle -> https://drupal.org/node/2090967
moving scripts to the bottom is here : https://drupal.org/node/2090991
now lets get this baby ready to fly & make frontenders happy world wide :)
Comment #46
mortendk CreditAttribution: mortendk commentedfixed the 80 chars comments issue that bartik.theme had (yes not created by this patch gentlemen ;) )
added in the new summary for the issue
created a new issue for html + page combination
created a new issue to discuss the scripts in bottom of the page
screenshots for it all ...
now can i get a RTBC :)
Comment #46.0
mortendk CreditAttribution: mortendk commentedupdated the summary
Comment #47
dbazuin CreditAttribution: dbazuin commentedJust I tought after a small discussion on IRC with "peteainsworth"
Why do we still use names like sidebar?
For example in a responsive layout the side bar drops below the main content on a phone and turns in a "Below Bar".
I think we need to think up a other name for them, like "Sub-content-first etc.
Comment #48
star-szr@dbazuin, cool idea. Please create a new issue for that discussion :)
Comment #49
dbazuin CreditAttribution: dbazuin commentedI willI added the issue 2093341
Comment #50
LewisNymanEverything works as expected without changing any CSS. Great! Just a few comment related things, after that it looks RTBC to me.
Because I know you like deleting things, we can probably loose the comments up here.
I think we need to change the comments in the theme template overrides, looking at Bartik's page.html.twig as an example:
“ * Bartik's theme implementation to display a single page.”
Comment #51
rteijeiro CreditAttribution: rteijeiro commentedI will put my love in this one :)
Comment #52
rteijeiro CreditAttribution: rteijeiro commentedDeleted suggested comment in core/includes/theme.inc
@LewisNeyman: Didn't understand your suggestion of changing the comments in the theme template overrides. It seems not to be the same template description and variables in core/modules/system/templates/html.html.twig and core/themes/seven/templates/html.html.twig compared to core/themes/bartik/templates/page.html.twig
As soon as you provide me more details I can finish this issue ;)
Thanks!!
Comment #53
rteijeiro CreditAttribution: rteijeiro commentedThis is the patch after talking with LewisNyman. Hope to have made it right :)
Comment #54
LewisNymanNice. Thanks!
Comment #55
webchickHm. I'd like JohnAlbin to take a look at this prior to commit if possible. My understanding was that Stark was intended to not add *any* extra HTML and instead just expose what core does. This seems to work against this idea.
Comment #56
mortendk CreditAttribution: mortendk commentedthe issue is that stark comes with a "design" - thats the only reason to add the sidebar preprocess.
core should not ship with the assumption of having a 3 column layout :)
the thing is now - do stark ship with a layout ?
if not - which i think makes sense, then let us clean that up :)
Comment #57
dbazuin CreditAttribution: dbazuin commentedJust a idea if we give stark a layout why not give it a full width one?
Comment #58
JohnAlbinFirst off, the patch doesn't apply because Seven doesn't have a seven_preprocess_html() any more. Looks super easy to re-roll.
I'm good with killing the div and the #skip-link ID. However, I would like to see the "skip-link" become a class on the <a> tag. The skip-link is, arguably, a design object that needs _some_ special styling on 99% of sites; if we provide the class by default, we don't have to override the default template. That would allow us to kill the html.html.twig file in both Seven and Bartik (though, we probably need to update some CSS selectors.
I know Seven needs the sidebar classes for the installer. But do we need all those classes for the installer? Let's verify which ones it actually uses and just insert those.
Also, the $suggestions-based classes are almost definitely not needed. Nix them.
Yes, that is still the reason-de-etre of Stark. It's a window into Drupal’s default markup and CSS. But there's always been one exception: Stark's layout. With responsive design, the old layout classes don't make sense for anyone's layout, except for Stark. So either we should move those classes to Stark (like the patch does) or we should delete Stark's layout completely. Stark currently has a responsive layout, so I'd lean to leaving the layout in Stark for D8, but I wouldn't oppose removing it if that's consensus.
Overall, I'm really pleased with the direction of this patch.
Comment #59
rteijeiro CreditAttribution: rteijeiro commentedPatch re-rolled. These are my points about @JohnAlbin comments:
1. Added "skip-link" class to anchor element.
2. seven_preprocess_maintenance_page function doesn't exist anymore in seven.theme file.
Comment #60
tim.plunkettI don't know a single custom theme I've ever had for a client that didn't use this stuff. You're crippling contrib/custom themes with this change.
Comment #61
dead_armI don't personally use the sidebar classes, but would definitely miss front, not-front, logged-in, not-logged-in and the body classes.
Comment #62
jibranPlease update the summary. After seeing #60 and #61 -1 for me as well for this change.
Comment #63
bayousoft CreditAttribution: bayousoft commentedAgree with tim.plunkett and dead_arm.
Comment #64
tim.plunkettThis still needs work to ensure there is only 1 html.html.twig file, and that all #skip-link are .skip-link.
But here is a discussed compromise from IRC.
Proof-of-concept, naming is up for debate, and it needs docs.
Interdiff was meaningless :(
Comment #65
yannickooI would name it $classes instead of $class because it returns an array with multiple classes. Otherwise see the function name ;)
Same here.
And again.
Comment #66
tim.plunkettI replaced $variables['attributes']['class'][] with $class[].
I really don't care that much though.
Comment #68
rcaracaus CreditAttribution: rcaracaus commentedI would prefer not to have .no-sidebars class added to body when I install a custom theme or contrib theme that already handles layout. I think layout logic should be left to the theme layer and not specified in core.
As far as .logged-in and .front < these classes will be used by people pretty commonly to do different display related things and I see no hurt in leaving them.
Comment #69
marcvangendI agree with #68. We can do without the .no-sidebars class. It comes from the assumption that every site has a 3-column, >=960px layout. That's simply not true, especially now that mobile and responsive are becoming mainstream. Often, a sidebar isn't even a bar on the side. Usually when I need such a class, I still need to tweak the logic behind it because the default doesn't exactly match my use case.
The other two are really useful, especially .front is needed in almost every theme I make.
Comment #70
LewisNymanIn Portland, we figured that the easiest way to clean up the core templates is to copy the old templates into Seven/Bartik and open up a follow up issue to clean up those templates. Otherwise we end up having to test for regressions over two/three themes with every issue. I think that's referenced in this issue.
If we agree that some classes are majority use case classes then we should keep them in but we shouldn't be trying to accommodate every situation, that the objective of dream markup.
.front
is a good example of a really common use case but Drupal shouldn't assume that every theme uses a three column layout. These classes are easy to add back in and it would be easy to create a base theme or a module that adds this stuff back in for upgrading D7 themes easily. This is the front-end equivalent of an API break so we should be careful.Comment #71
tim.plunkettOkay, based on the response after #60, here is a compromise:
<div id="skip-link">
wrapping the link is removed and a skip-link class is added to the link itselfBecause the .skip-link class is now alongside .visually-hidden and .focusable, it has to combat their rather forcible CSS:
The attached interdiff shows just the change needed to overcome that, the rest of the interdiff was pretty meaningless.
Comment #72
tim.plunkettComment #73
markhalliwellLooks really great! Thanks @tim.plunkett! This is indeed a very acceptable compromise. One tiny thing:
We should add a @todo here for #2091399: [META] Remove menu_get_object().
Comment #74
ry5n CreditAttribution: ry5n commentedThe compromise in #71 makes sense to me, and I think strikes the right balance for the various audiences. Patch looks good, too. Only one thing stands out, and it’s very minor:
We don’t need to qualify the class selector with the element unless it’s resolving a specificity issue (in which case we can leave it for CSS cleanup). Otherwise this can just be `.skip-link`. Patch attached with just this change.
(As a separate issue, it seems like the visually-hidden styles need further review, since obviously needing
!important
here is not a good sign.)Comment #75
markhalliwellDiscussed in IRC and no @todo is fine, tests would discover it. This looks great to me, if no other objections, I'll go ahead and RTBC.
Comment #76
markhalliwellThe removal of the
a
tag in the CSS selectors is also good, will keep this RTBC, just wait for patch to come back green.Comment #77
tim.plunkettManually tested #74, the a qualifier was not needed for specificity.
Comment #78
mortendk CreditAttribution: mortendk commentedi wont block this patch but im gonna open up a new issue so we can get rid of
"page- page-foo-bar front not-front logged-in"
etc.imho They don't belong in Core! - and should only be used if you want em in your theme. to get them there is a documentation issue.
Please let us get a super super super clean drupal & make it easy to add what's needed when its needed.
But lets get this sucker in :)
im gonna create a follow up isssue to clean up core classes for realz
Comment #79
alexpottCommitted 91d867f and pushed to 8.x. Thanks!
Comment #80
alanburke CreditAttribution: alanburke commentedFollowup issue for broken layout at #2117441: Stark layout broken as 'generic' classes removed from html.tpl.php
Comment #80.0
alanburke CreditAttribution: alanburke commentedUpdated issue summary.
Comment #81
marcus7777 CreditAttribution: marcus7777 commentedLets put the main content first and drop the
for dreamcode :0)
Comment #82
Jeff Burnz CreditAttribution: Jeff Burnz commented@mortendk did you open the issue for removing the other body classes, I can't find it, be great if we could remove those horrid page-suggestion classes.
Comment #83
rteijeiro CreditAttribution: rteijeiro commentedAdded markup changes from: https://drupal.org/node/2011578
Cleaned an improved styling.
Comment #84
rteijeiro CreditAttribution: rteijeiro commentedForget about last changes :(
Comment #85
rteijeiro CreditAttribution: rteijeiro commentedChange notice added here: https://drupal.org/node/2155761
Mixed with change notice for #2011578: Markup for Stark's page.html.twig + maintenance-page.html.twig as per @alexpott suggestion in #2011578-67: Markup for Stark's page.html.twig + maintenance-page.html.twig
Comment #86
star-szrThanks @rteijeiro!
Comment #88
shgrettenberg CreditAttribution: shgrettenberg commentedThis was the single worst decision in Drupal programming history. Making changes that broke somewhere between tens and hundreds of thousands of sites in the name of clean code severely damaged Drupal's reputation. Decisions like this keep businesses from trusting Drupal. As a tech, having taken over maintenance for a site that was poorly designed and not documented, it has been a bloddy frustration. The site has hundreds of modules, multiple directories, and more. Trying to discern how to patch it up after "security" updates has so far taken over thirty-six hours. It still does not work.
Why is there no clear link here to "This is how you can modify your broken themes to work after some busy-body coders thought it was fine to break Drupal's reputation mid cycle for little apparent reason"?
So frustrated.....
Comment #89
amateescu CreditAttribution: amateescu commented@shgrettenberg, the patch committed here is for Drupal 8, which is not even released yet. What "hundreds of modules, multiple directories, and more" are you talking about?
Comment #90
mortendk CreditAttribution: mortendk commentedwow "This was the single worst decision in Drupal programming history"
i guess we broke a new record :P
Comment #91
RumpledElf CreditAttribution: RumpledElf commentedAnd remember to always test Drupal upgrades on a copy of the site, never the production site ... why that comment is in a Drupal 8 thread at all is a bit baffling though.