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.Problem/Motivation
After the removal of CSS classes from the core templates, many unnecessary HTML tags were left behind. They were not removed in the process of removing classes and other work, because removing them requires paying particular attention to any affect on the output.
Proposed resolution
Go through each core template and remove any unnecessary markup. This will require looking at the output to see if things shift around in unwanted ways. Various CSS files may also need checking. For example, if there is CSS like .some_class > div > .some_other_class the CSS should be adjusted, or deleted.
Evaluation criteria, not set in stone but up for discussion:
- divs and spans with no additional attributes should be removed mercilessly unless they justify their existence.
- Semantic tags (footer, article, etc.) should be retained even if they have no attributes.
Beta phase evaluation
| Issue category | Task |
|---|---|
| Issue priority | Normal because nothing is broken. |
| Unfrozen changes | Unfrozen because it only changes templates and possibly CSS which are both unfrozen. |
| Prioritized changes | The main goal of this issue is to improve themer experience. |
| Disruption | There should be minimal disruption. |
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff-meta_remove-2483319-18.txt | 4.18 KB | brahmjeet789 |
| #18 | meta_remove-2483319-18.patch | 0 bytes | brahmjeet789 |
| #17 | 2483319-BackstopJSReport.pdf | 551.14 KB | LewisNyman |
| #17 | 2483319.backstop.json_.txt | 1.07 KB | LewisNyman |
| #12 | meta_remove-2483319-12.patch | 10.03 KB | davidhernandez |
Comments
Comment #1
star-szrThanks @davidhernandez :)
Comment #2
star-szrDon't want to split this too small, first pass looking at the templates on HEAD I only found 16 potential candidates. I've verified these are all in Classy so we should be free to change them in core.
Maybe that could be split by name into A-H and I-Z or something? Some of them seem like very straightforward changes, others less so.
Comment #3
davidhernandezI took a look at the remaining banana phase 2 issues #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme and there will be a few more templates left with divitis, but it doesn't look like there will be a huge amount. We can follow up after those have their classes removed.
What do we want to do about things like
<h2>? Do we consider that to be similar<article>and<footer>; serving as more than just a wrapper?Comment #4
davidhernandezdouble post
Comment #5
davidhernandezthat was weird
Comment #6
star-szrYeah I think we should keep h* tags even if they have no attributes, they at least have semantic meaning.
Comment #7
LewisNymanYeah, anything with semantic meaning would need a conversation so for ease this issue could stick to
<div>and<span>elements only. We could move the trickier ones to a follow upComment #8
star-szrAgreed! This is just to remove the cruft and should be fairly quick and painless. This is not #dreammarkup.
Comment #9
LewisNymanIf all we are doing are removing unstyled divs without class, I wonder if we could do this in one patch? There isn't really any scope for regressions.
Comment #10
star-szrWe should remove text-format-wrapper.html.twig from my list because it actually does need some classes on that div for JS functionality:
#2473945: Prefix form-item classes with js-
#2473957: Prefix text-* classes with js-
Comment #11
davidhernandezI'm gunna make a first patch with everything in the list just to see how much it is and what might break.
Comment #12
davidhernandezThis was fairly simple and should be easy for use to work on and review. I don't think we need to split it up.
I left out text-format-wrapper.html.twig and book-export-html.html.twig. In the book export template it looks like the div is functional? It creates nesting.
Comment #13
davidhernandezComment #14
LewisNymanWe should run a visual regression test to see if anything changes, should Stark be the active theme?
Comment #15
davidhernandez> should Stark be the active theme?
These should all be templates that had classes removed, thus leaving empty divs, so there should be Classy copies. But, it doesn't hurt to double-check.
If so, Seven and Bartik aren't affected.
Comment #16
davidhernandezGave a quick look and I see a copy of all these templates in Classy, so, yes, check with Stark.
Comment #17
LewisNymanI only found one change to the visuals, it's just where we are using divs for the separate link formatter, they no longer break on to separate lines. I don't think this is a regression we have to worry about.
Comment #18
brahmjeet789 CreditAttribution: brahmjeet789 commentedI have done the intentation of the code
Comment #19
Manjit.Singh@brahmjeet789 I guess you have upload #12 patch again :P
Comment #20
brahmjeet789 CreditAttribution: brahmjeet789 commentedsorry @davidhernandez your patch is uploaded by mistake ,i don't know how to delete this file.
Comment #21
star-szr@brahmjeet789 - thanks for your patch but I'm confused as to what you're trying to do. The patch is 0 bytes and interdiff shows removing blank lines which I think we actually want to keep.
I may be wrong, just looked at the patch on my phone.
Besides that I think we should move the #10 patch to a child issue so that this can remain a meta.
Comment #22
davidhernandezI move the patch in #12 to a child issue so we can leave this as a meta. Other child issues will come later for views and system.
Comment #23
star-szrGoing to active to reflect the official meta-ness, and the markup component doesn't get enough love.
Comment #24
davidhernandezChild issues have been completed.