Remove title and wrapper div from item-list.html.twig

[edit: nevermind, heard access isn't being given anymore. will submit teeny patch :) ]

Teeny tiny patch attached (I'm assuming this is for stark, yah? :)

looks good to me, but I'm not surre about the indention.

I'm closing this since at this point we just need to get these converted to core, then refactor later. This is a core issue, not a sandbox issue.

Nevermind, I'm just going to move this into the core queue and revise the summary. Less duplication :) Will revise summary.

Related: #1910996: Update theme_item_list to call theme_ol or theme_ul

add templates affected

Just a follow-up for the Twig sandbox folks, I'm removing ol.html.twig and ul.html.twig per #1910996: Update theme_item_list to call theme_ol or theme_ul, we'll follow-up there.

The title support should not be removed. It's there for accessibility: menus, in fact, are lists; and each menu needs a title.

Better summary

Working on this alongside johnnydarkko, who is working on #311011: Replace links.html.twig with item-list--links.html.twig.

code up

Here's a reroll of #13 just to get it applying again.

#17: drupal-clean_up_item_list-1842140-17.patch queued for re-testing.

Tagging for re-roll

Rerolled #17 :)

Updating status

#23: drupal-clean_up_item_list-1842140-22.patch queued for re-testing.

Patch 22 will not apply to current code base.
Can apply at commit 5fccca655e90d9e7a557fda8d16dc2afab93ac08
I'm currently re-rolling patch against current.

Simpletest test failures look related to pager errors.

Re-rolled patch against current code.
conflict was in core/themes/seven/styles.css
Couldn't get my interdiff to work.
I expect patch will fail testing but now we are at least up to date.

needs a reroll, https://drupal.org/contributor-tasks/reroll

Here's a reroll from #28.

I ran the render test that failed on my local dev with the patch applied, and the test passed. So, setting testbot to rerun this.

Errr, sorry, wrong issue.

Here's a reroll.

I'm not fully grokking the preprocess function: it seems to me like this will probably out the item-list class on list elements rather than ul or ol. But maybe I'm wrong.

Hopefully this is a better starting point to finish patching this up.

New attempt to set the item-list class on the list. This is probably wrong: we'll see if it's more or less wrong.

Also, we may want to change the scope of this issue. The header on the list is already optional in the item-list twig file. This patch just focuses on removing the wrapper div now. Probably the way to go.

1. +++ b/core/includes/theme.inc
   @@ -1769,11 +1769,14 @@ function theme_mark($variables) {
   +  $variables['attributes'] = (array) $variables['attributes'];
   

   This line is not needed at all, attributes are magic arrays to start and get converted to Attribute objects during drupal_render.

2. +++ b/core/modules/simpletest/css/simpletest.module.css
   @@ -35,7 +35,7 @@ table#simpletest-form-table tr.simpletest-group label {
   +div.message > ul.item-list {
   

   This would be nicer if it didn't have the element selector on it, can we say that all direct children with that class get the same style?

3. +++ b/core/modules/system/css/system.theme.css
   @@ -149,11 +146,11 @@ abbr.form-required, abbr.tabledrag-changed, abbr.ajax-changed {
   +ul.pager {
   ...
   +ul.pager li {
   

   probably want to do .pager.item-list

4. +++ b/core/modules/views/templates/views-view-summary.html.twig
   @@ -19,14 +19,12 @@
   -  <ul class="views-summary">
   -  {% for row in rows %}
   -    <li><a href="{{ row.url }}"{{ row.attributes }}>{{ row.link }}</a>
   ...
   +<ul class="views-summary item-list">
   +{% for id, row in rows %}
   +  <li><a href="{{ row.url }}"{{ row_classes[id] }}>{{ row.link }}</a>
   

   This row_classes stuff is not right at all... should be row.attributes.

5. +++ b/core/themes/seven/style.css
   @@ -1,15 +1,123 @@
   +body {
   +  color: #000;
   +  background: #fff;
   +  font: normal 81.3%/1.538em "Lucida Grande", "Lucida Sans Unicode", sans-serif;
   +}
   +a,
   +.link {
   +  color: #0074bd;
   +  text-decoration: none;
   +}
   +a:hover,
   +.link:hover {
   +  text-decoration: underline;
   +}
   +hr {
   +  margin: 0;
   +  padding: 0;
   +  border: none;
   +  height: 1px;
   +  background: #cccccc;
   +}
   +summary,
   +legend {
   +  font-weight: bold;
   +  text-transform: uppercase;
   +}
   +h1,
   +h2,
   +h3,
   +h4,
   +h5,
   +h6 {
   +  font-weight: bold;
   +  margin: 10px 0;
   +}
   +h1 {
   +  font-size: 1.538em;
   +}
   +h2 {
   +  font-size: 1.385em;
   +}
   +h3 {
   +  font-size: 1.231em;
   +}
   +h4 {
   +  font-size: 1.154em;
   +}
   +h5,
   +h6 {
   +  font-size: 1.077em;
   +}
   +p {
   +  margin: 1em 0;
   +}
   +dl {
   +  margin: 0 0 20px;
   +}
   +dl dd,
   +dl dl {
   +  margin-left: 20px; /* LTR */
   +  margin-bottom: 10px;
   +}
   +[dir="rtl"] dl dd,
   +[dir="rtl"] dl dl {
   +  margin-right: 20px;
   +}
   +blockquote {
   +  margin: 1em 40px;
   +}
   +address {
   +  font-style: italic;
   +}
   +u,
   +ins {
   +  text-decoration: underline;
   +}
   +s,
   +strike,
   +del {
   +  text-decoration: line-through;
   +}
   +big {
   +  font-size: larger;
   +}
   +small {
   +  font-size: smaller;
   +}
   +sub {
   +  vertical-align: sub;
   +  font-size: smaller;
   +  line-height: normal;
   +}
   +sup {
   +  vertical-align: super;
   +  font-size: smaller;
   +  line-height: normal;
   +}
   +nobr {
   +  white-space: nowrap;
   +}
   +abbr,
   +acronym {
   +  border-bottom: dotted 1px;
   +}
   +.pager li {
   +  padding: 0.5em;
   +}
   

   none of this should be in here.

Here's a patch that addresses the issues in #41.

+++ b/core/modules/views/templates/views-view-summary.html.twig
@@ -19,14 +19,12 @@
-<div class="item-list">
-  <ul class="views-summary">
-  {% for row in rows %}
-    <li><a href="{{ row.url }}"{{ row.attributes }}>{{ row.link }}</a>
-      {% if options.count %}
-        ({{ row.count }})
-      {% endif %}
-    </li>
-  {% endfor %}
-  </ul>
-</div>
+<ul class="views-summary item-list">
+{% for id, row in rows %}
+  <li><a href="{{ row.url }}"{{ row.atributes[id] }}>{{ row.link }}</a>
+    {% if options.count %}
+      ({{ row.count }})
+    {% endif %}
+  </li>
+{% endfor %}
+</ul>

not quite, no need for the [id] in the loop. That code should not change.

Fixing an error with the attributes.

Moving this forward. Someone could take a look for the visual changes

I just review patch 1842140-48.patch. I rendered a test list and it looks the same, with and without the patch.

It looks good to me except, one:
You forgot to include changes to core/themes/seven/css/components/table.css

I created a patch that also includes table.css

I think, the only case, there is a side-effect, is when .item-list has a fixed width and a box-size model, that doesn't count padding to it's size. After applying the patch, .item-list element will be bigger then it's original size.

Since the "ul" tag has already straightforward meaning we actually don't need special ".item-list" class here. For menu purposes we still have ".menu" class there.

Finally we found all the occurrences of ".item-list" in the Drupal core and got rid of them.

Title is still in the templates for compatibility reasons. Anyway it could be easily overridden in templates or even cut in the future.

All of this was done in agreement with mortendk and johnalbin.

Attached patch also fixes views markup.

And let's not forget to add omg-its-maggie in the commit message!

She helped me a lot with testing, support and inspiration!

Fixed issues related to view_list template and tests.

Fixed XPaths in Views Field UI tests.

Guys, please review!

Two things to note:

• The patch was done in agreement with mortendk and johnalbin.
• Don't forget to add omg-its-maggie in the commit message!

The patch looks great to me.

The test changes make total sense and tests pass.

A DivKiller issue, which is a nice cleanup overall.

=> RTBC

Committed 7939567 and pushed to 8.0.x. Thanks!

Getting rid of .item-list is a regression with D8's CSS architecture: https://www.drupal.org/node/1887918.

Now CSS is dependent upon a specific HTML element, ul, which isn't good if an ol is needed. If we were getting rid of the wrapper div, we would have been better off applying .item-list to the ul and using that in the selector rather than a ul element.

1. +++ b/core/modules/system/css/system.theme.css
   @@ -21,24 +21,6 @@ td.active {
   - * Markup generated by theme_item_list().
   - */
   -.item-list .title {
   -  font-weight: bold;
   -}
   -.item-list ul {
   -  margin: 0 0 0.75em 0;
   -  padding: 0;
   -}
   -.item-list ul li {
   -  margin: 0 0 0.25em 1.5em; /* LTR */
   -  padding: 0;
   -}
   -[dir="rtl"] .item-list ul li {
   -  margin: 0 1.5em 0.25em 0;
   -}
   -
   -/**
   

   Nothing replaces these styles that are removed?

2. +++ b/core/themes/seven/css/components/menus-and-lists.css
   @@ -1,15 +1,7 @@
   -.item-list ul {
   -  list-style-type: disc;
   -  list-style-image: none;
   -  margin: 0.25em 0 0.25em 1.5em; /* LTR */
   -}
   -[dir="rtl"] .item-list ul {
   -  margin: 0.25em 1.5em 0.25em 0;
   -}
   -.item-list ul li,
   +li,
   

   We're removing styles without any replacement here. Also, this is now applying styles to every single list item anywhere rather than to list items designated as part of an item list.

3. +++ b/core/themes/seven/css/components/menus-and-lists.css
   @@ -18,13 +10,11 @@ ul.menu li {
   -.item-list ul li.collapsed,
   -ul.menu li.collapsed {
   +li.collapsed {
   

   Again, context gone.

4. +++ b/core/themes/seven/css/components/menus-and-lists.css
   @@ -18,13 +10,11 @@ ul.menu li {
   -.item-list ul li.expanded,
   -ul.menu li.expanded {
   +li.expanded {
   

   Context gone.

5. +++ b/core/themes/seven/css/components/tables.css
   @@ -93,7 +93,7 @@ th.active > a:focus:after,
   -td .item-list ul {
   +td ul {
   

   Again, this is now targeting all unordered lists inside of a table cell.

We should probably do a follow-up issue to make sure that we get this CSS in line with D8's CSS architecture.

Hmm this is a bit of a quick change for having such a large scope. This may need reverting, discussion and mostly screenshots of item lists changed.

I completely agree with the idea behind this, but the rigor in testing is missing. Since it's CSS there is no visual regression tests in core so we need eyeballs.

Well, John Albin agreed, so we should ping him, before rolling back.

We are close to having visual regression testing working in contrib, fwiw.

We can add the ul item-list also back to classy, as core would not have that class anyway.

Ul,ol are lists - always, so that is redundant information.

I think the do not depend on elems is meant in another context, but need John to chime in.

Assigning ...

You are right the class is quite useless but it seems people have given it meaning where there was none before. Using it to distinguish uls used for menus or in the body copy or in pagers vs ones generated by this function.

That "meaning" needs to be undone for this issue to be actually "fixed" which is why the rigor is needed.

Also see #2335003-23: Rename task-list.html.twig to maintenance-task-list.html.twig for a screenshot of the regression.

Are you saying that it is ok to change the classname as long as it's a more sensible semantic name?

Because then I agree.

Do we have to have item-list wrapper? Cant we use it as a class the way I suggested on #48?

You are right the class is quite useless but it seems people have given it meaning where there was none before. Using it to distinguish uls used for menus or in the body copy or in pagers vs ones generated by this function.

That "meaning" needs to be undone for this issue to be actually "fixed" which is why the rigor is needed.

@joelpittet

Right. That's why we executed something like grep -r 'item-list' throughout the whole code to fix everything.

We made a basic tests after and everything looked fine but we used installed Drupal that's why we missed that one on the installation page.

***

Also see #2335003-23 for a screenshot of the regression.

@joelpittet

It's kind of a "bug" in maintenance-page.css actually:

  .task-list {
    ...
    list-style-type: none;
    ...
  }

This rule should be added also there:

  .task-list li {
    ...
    list-style-type: none;
    ...
  }

***

I see only 2 potential problems which should be double-checked while deeper testing:

1. Forgetting OL when moving from DIV.item-list to UL — here we can just clone UL rules for OL, easy.
2. Breaking selectors like ... div > ul since we have no interjacent DIV anymore — this is not so easy to detect, but using such selectors are just wrong initially.

So, I think we do it right way at all: we definitely don't need .item-list class here. What we need — is exploratory testing. And let's wait @johnalbin feedback of course.

It sounds like in the meantime this should be reverted and then we can get a more thoroughly tested version in.

git revert 7939567

Thank you @alexpott!

Adding participants from #2351023: Bullets are showing in installer to the commit credit string in the issue summary, since they jumped in to fix one of the regressions created by this patch.

@tonystar: The site was visually broken also on installed site wherever .item-list was before that change used e.g. messages.

Rerolled and added the regression patch from #2351023: Bullets are showing in installer

Added missing interdiff file.

I think we should start adding URLs/steps to the issue summary where this can be tested. For example the search results uses an item_list theme suggestion.

Fixing tags.

+++ b/core/modules/simpletest/css/simpletest.module.css
@@ -35,7 +35,7 @@ table#simpletest-form-table tr.simpletest-group label {
+div.message > ul {

+++ b/core/themes/bartik/css/style.css
@@ -326,11 +326,11 @@ ul.menu li {
-.item-list ul li {
...
+[dir="rtl"] li {

@@ -494,30 +494,30 @@ h1.site-name {
+.region-header #block-user-login ul,
...
+.region-header #block-user-login ul {

these changes (and a lot on this thread) making the CSS less reusable and harder theming. It just making for the Drupal project, not for all custom Drupal projects. when you adding custom features, you also have to adding many CSS to override the widely pre-applied default style.

I'd suggest to limit the scope. adding re-use .class the ul lists...etc

https://smacss.com/book/type-module

We could try some visual regression testing using SiteEffect.io for this issue.

This CSS definitely needs to be fixed. There are a ton of style definitions that depend solely upon element chains, which doesn't fit with D8's CSS architecture. Defining all the behavior of every li everywhere is far too broad. Much better to have class selectors that are less likely to cause problems elsewhere. Having ID's in the selectors makes the specificity very difficult to override: it looks like that may have been there previously, though, so that may or may not be in scope to fix that for this particular issue.

As suggested by @fabianx I have tested the patch that got reverted, just to see what I would catch with the visual regression testing approach described in #2229187

I wasn't able to figure out yet if it is being considered ok for the patch to introduce visual changes. Meaning I'm not sure if e.g. the spacing between primary tabs is allowed to change or not. As siteeffect isn't public yet I just uploaded gifs to dropbox to give you an idea. The preview mode actually works quiet nicely with gifs:

https://www.dropbox.com/sh/xptxt6skjgi0mjj/AAAfR3MrRxQpE5hZpOEm8UqAa

I can post the urls to these pages if that helps.

Background on which pages have been tested: A fresh D8 install is being done with all modules enabled. Then for every page SiteEffect finds in its build in crawler, the menu router item will be looked up. Only three pages per menu router item will be analyzed. This way only three menu edit forms will be tested and not hundreds. The problem so far is though that no test nodes, comments or terms exist and therefore a lot isn't being covered. Working on it...

The test setup is not fully automated yet, so it always takes me a little while, but I'm getting there. If I can help with running more tests, maybe on the latest patch then let me know.

lets get a beta evaluation for this normal task. added directions to the issue summary.

I added patch, Please check.

I added patch, Please check.

#94 is incomplete, rerolling...

rerolled #83.

$expected wasnt reinitialized... oops!

Argh left me debug calls in the last patch sorry.

Patch applies

@Manuel Garcia Do not worry about the multiple patches. :)

Interdiffs would be nice when making changes after a patch that applies when getting tests to pass though.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff
For example you could have made a interdiff.1842140.97.99.txt and interdiff.1842140.99.100.txt
it helps other people learn from your fixes, and helps reviewers.

Please let us be sure to get the screenshots and issue summary done while this applies and passes tests.

@YesCT true... I keep forgetting we can just use interdiff for this, will tattoo it on me forhead.

Glad to see the rerolling. I think we still need to address the concerns I raised in #90 with using elements in the CSS selectors rather than classes, as that goes against our CSS architecture for D8.

@mdrummong; the patch replaces the logic with ID's as-is. This issue is targeted at removing the wrapper div, which it does. Can we add a follow-up to improve the css selectors?

Core moves fast. Here is a re-roll.

Try again, testbot.

Regarding #106: In my opinion, no. At this stage in the D8 release cycle, we really can't make D8 less shippable. Even ignoring that, we should follow our own standards.

Thanks for keeping on this @BarisW.

I tested the patch in #109 again with siteeffect, looks lot more stable now. Some issues I found:

• /admin/help: The spacing changes a lot, not sure if that is ok
• /admin/reports/updates: Two new bullet points for 'Release notes' and 'Download' that are totally off
• /admin/structure/views/view/frontpage: "Add content" in the preview now has a bullet point that even sticks outside the preview
• The whole admin menu has different spacing
• Primary tabs have a different spacing

Hey @ti2m, it would be nice if you can provide some screenshots from your awesome SiteEffect tool to help to find the issues.

Here are some screenshots, gifs don't work that well either. I think best might be to generate screenshots next to each other and highlight changed areas. Maybe this works for now if you download them and switch between them.

https://www.dropbox.com/sh/000977owkn9uuae/AAD3Pf8M4HqA-S96GzGenJ2ha?dl=0

Rerolling

Stuf taht was removed in core/modules/system/css/system.theme.css was now removed on core/themes/classy/css/components/item-list.css, but there are some styles left in there so someone should take a look.

As a matter of fact, please review everything to do with item-list.css both in bartik and classy.

I'm sorry but I messed up while rerolling so I'm not uploading the reroll. Will try getting to this at another time.

Posting a reroll.

+++ b/core/modules/views_ui/css/views_ui.admin.theme.css
@@ -638,10 +638,15 @@ td.group-title {
+<<<<<<< HEAD
...
+=======
...
+>>>>>>> #109

@@ -752,7 +757,57 @@ td.group-title {
+<<<<<<< HEAD
...
+=======
...
+>>>>>>> #109

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -17,12 +17,12 @@
   list-style-type: circle;

Merge art effects

thanks @jibran! I think you meant "artifacts" here, instead of "art effects" :)

I see what Manuel means in #116, reconciling the changes to Bartik's item-list.css and system.theme.css is making my head hurt!

Yeah @akalata... i messed it up there as well... I'm pinging the fine folks on #drupal-twig in IRC to see if any of them can give us some information to reroll this properly. Will post here any findings.

1. +++ b/core/themes/bartik/css/components/item-list.css
   @@ -3,24 +3,25 @@
   -.item-list ul {
   +ul {
      list-style: none;
      margin: 0 0 0.25em 0;
      padding: 0; /* LTR */
    }
   

   This is going to apply to EVERY ul element. Is that really what we want?

   Similarly the styles in the patch for li are going to affect all of them.

2. +++ b/core/themes/seven/css/components/menus-and-lists.css
   @@ -1,15 +1,15 @@
   +ul {
      list-style-type: disc;
      list-style-image: none;
      margin: 0.25em 0 0.25em 1.5em; /* LTR */
    }
   

   This will also apply to every ul in Seven.

3. I don't really understand why we can't put reasonable semantic class names on these elements and refer to them as such. Targeting elements with CSS seems like a bad idea. I support the idea of removing the wrapper div, but why can't we move the wrapper div's class -- item-list -- to the ul itself? That's what the issue summary says is supposed to happen.

Re-rolled. Probably #126 should be addressed

I don't know if this is directly relevant to this issue, but title_attributes in the item list are lost before they reach the TWIG file.