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.
#18 interdiff-meta_remove-2483319-18.txt4.18 KBbrahmjeet789
#18 meta_remove-2483319-18.patch0 bytesbrahmjeet789
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,768 pass(es). View
#18 meta_remove-2483319-12.patch10.03 KBbrahmjeet789
#17 2483319-BackstopJSReport.pdf551.14 KBLewisNyman
#17 2483319.backstop.json_.txt1.07 KBLewisNyman
#12 meta_remove-2483319-12.patch10.03 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,485 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


Cottser’s picture

Issue summary: View changes
Issue tags: +divitis

Thanks @davidhernandez :)

Cottser’s picture

Don'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.

  • aggregator-feed.html.twig
  • block--system-branding-block.html.twig
  • book-export-html.html.twig
  • datetime-wrapper.html.twig
  • details.html.twig
  • filter-tips.html.twig
  • forums.html.twig
  • image-widget.html.twig
  • item-list.html.twig
  • link-formatter-link-separate.html.twig
  • maintenance-page.html.twig
  • mark.html.twig
  • node-edit-form.html.twig
  • search-result.html.twig
  • taxonomy-term.html.twig
  • text-format-wrapper.html.twig

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.

davidhernandez’s picture

Issue tags: +Novice

I took a look at the remaining banana phase 2 issues #2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy 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?

davidhernandez’s picture

double post

davidhernandez’s picture

that was weird

Cottser’s picture

Yeah I think we should keep h* tags even if they have no attributes, they at least have semantic meaning.

LewisNyman’s picture

Yeah, 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 up

Cottser’s picture

Agreed! This is just to remove the cruft and should be fairly quick and painless. This is not #dreammarkup.

LewisNyman’s picture

If 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.

Cottser’s picture

We 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-

davidhernandez’s picture

Assigned: Unassigned » davidhernandez
Issue tags: +LosAngeles2015

I'm gunna make a first patch with everything in the list just to see how much it is and what might break.

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
10.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,485 pass(es). View

This 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.

davidhernandez’s picture

Status: Active » Needs review
LewisNyman’s picture

We should run a visual regression test to see if anything changes, should Stark be the active theme?

davidhernandez’s picture

> 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.

davidhernandez’s picture

Gave a quick look and I see a copy of all these templates in Classy, so, yes, check with Stark.

LewisNyman’s picture

I 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.

brahmjeet789’s picture

10.03 KB
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,768 pass(es). View
4.18 KB

I have done the intentation of the code

Manjit.Singh’s picture

@brahmjeet789 I guess you have upload #12 patch again :P

brahmjeet789’s picture

sorry @davidhernandez your patch is uploaded by mistake ,i don't know how to delete this file.

Cottser’s picture

@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.

davidhernandez’s picture

I 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.

Cottser’s picture

Component: theme system » markup
Status: Needs review » Active

Going to active to reflect the official meta-ness, and the markup component doesn't get enough love.

davidhernandez’s picture

Status: Active » Fixed

Child issues have been completed.

Status: Fixed » Closed (fixed)

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