Fix inline list CSS

No patch yet, so think the RTBC here was accidental? :)

Here's a patch cut from the latest changes #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages()

Thanks!

+++ b/core/themes/classy/classy.libraries.yml
@@ -3,6 +3,9 @@ base:
+      # We can not attach this in the item-list.html.twig template since it is
+      # often used during RendererInterface::renderPlain().
+      css/dataset/item-list.css: {}

Let me repeat my concern/comment from the original issue:

#57.3: aahhh! So this is basically a case where a message for drupal_set_message() does need to bubble attachments. That was not at all clear to me on my first reading.

Part of this got added in #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages(), so we need to reroll the patch on top of that.

Both @davidhernandez and @alexpott had a couple concerns in the other issue.

• The CSS to fix up the inline-ness of the item lists should probably be in Classy, so that Bartik, Seven, and any other themes that extend classy have the expected formatting.
• #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() added a fix specifically targeting the visual regression of having an indentation at the beginning of the inline list.
• There was some disagreement about whether an inline list should be rendered as a block level element (i.e. comma-separated, but on its own line), or whether it should be truly inline (so that, for example, it could appear in the same line as the rest of a paragraph of text). I'd lean toward allowing it to be inline, but I lost track of all the specific concerns here.
• There was also some concern about how well the inline lists currently support other item list functionality like titles and descriptions,

Once we have some consensus on this issue and the fixes (whatever they may be) are committed, we should do a couple things:

1. Revisit the fix from #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() (which is a good example of how an inline list could be used) and update it. (Needs followup.)
2. Update the weird "DayMonthHourMinute" tests to ensure complete coverage without testing for words that don't actually exist, testing instead for the presence of the jump links. (Needs followup.)
3. Go back to other places where we used (e.g.) a |safe_join with commas, and convert them to the inline lists too. (Needs followup.)

Filed #2559793: Convert templates and inline templates using |safe_join to inline item list.

Re-rolled not required. Latest patch tried in Drupal 8.0.x branch after rebase It works for me.

@madhavvyas, reroll means exactly rebasing with the patch applied and resubmitting the updated patch on the issue. :)

@joelpittet, honestly I'm not entirely sure; @alexpott and @davidhernandez were the ones with concerns (see the other issue).

Also, we should probably manually test this again once it's rerolled... the screenshots in the summary are not current.

Oh also:

There was also some concern about how well the inline lists currently support other item list functionality like titles and descriptions...

That may be out of scope of this issue but it doesn't remove them it just doesn't explicitly style them different from other item lists.

Let's manually test with those too and add screenshots of what that looks like. I think it should be in scope for this issue to make sure they don't look stupid.

+++ b/core/themes/classy/templates/dataset/item-list.html.twig
@@ -19,10 +20,11 @@
+  {%- set attributes = attributes.addClass('item-list__' ~ context.list_style) %}

I don't think we actually need to do this now. I'll try to take a look later today. A lot of this can be simplified now that we are moving the CSS to Classy.

Thanks @joelpittet.

Can we test with the other features of item lists like title and description and empty to see what it looks like?

I still argue the inlining is the responsibility of messages to set since it is the outer wrapper, not the inline list, but I won't dwell on it.

I was able to refactor some (I had to stop myself from going down the rabbit hole. Is it too late to refactor all of core's CSS?) This allowed some simplifying of Bartik's override and completely removing what was added in Seven. Also, deleting a number of other declarations. Yay!

I'm ok with this. It should be the most flexibly overridable. I checked the form errors (Thanks to stefan.r's awesome little module.) and the lists on the Views list page. I checked all four themes LTR and RTL. Didn't make screenshots though. It's late here.

Here is left to right. I can do right to left later. ...but don't let that stop anyone else from doing it. ;)

something something multiple file upload would be awesome.

right to left

Skitch and Stefan's module have made this so much easier to do.

So what are we going to do about the title support? <h3> tags don't play nicely with inlineness.

I dunno. Do we have a usecase for that yet so it can be thought out?

I just realized there is css for .title. Why is that there? Did we remove that accidentally? We don't actually need it with the h3, but I don't know if that is just legacy.

I don't think there is a use-case for it with a comma list but an inline div having h3 inside doesn't seem to make much sense - and it is possible to do with the current template.

The item list title is an H3. I don't think it should be inline, it is not a label.

I hard-coded it real quick to see how it looks. It seems fine to me. I wouldn't expect it to, out-of-the-box, be treated any different. It's an H3,

+++ b/core/includes/theme.inc
@@ -999,6 +999,7 @@ function template_preprocess_tablesort_indicator(&$variables) {
+  $variables['wrapper_attributes'] = new Attribute($variables['wrapper_attributes']);

This line of code makes my brain bendy.

Also, a few lines down, we have:

  foreach ($variables ['items'] as &$item) {
    $attributes = array();
    // If the item value is an array, then it is a render array.
    if (is_array($item)) {
      // List items support attributes via the '#wrapper_attributes' property.
      if (isset($item ['#wrapper_attributes'])) {
        $attributes = $item ['#wrapper_attributes'];
      }

Hash or no hash?! I confused. There may be a perfectly logical explanation for this, but that's what made me hesitate before committing.

Forgot to say, I'm okay with the title thing if others are. If you put an item list with a title in a sentence, well that is your silly. Thanks for the screenshots!

I rerolled the patch.

Patch rerolled #40

+++ b/core/modules/system/system.libraries.yml
@@ -19,6 +20,27 @@ base:
+    theme:
+      css/components/action-links.theme.css: { weight: -10 }
+      css/components/breadcrumb.theme.css: { weight: -10 }
+      css/components/button.theme.css: { weight: -10 }
+      css/components/collapse-processed.theme.css: { weight: -10 }
+      css/components/container-inline.theme.css: { weight: -10 }
+      css/components/details.theme.css: { weight: -10 }
+      css/components/field.theme.css: { weight: -10 }
+      css/components/form.theme.css: { weight: -10 }
+      css/components/icons.theme.css: { weight: -10 }
+      css/components/inline-form.theme.css: { weight: -10 }
+      css/components/link.theme.css: { weight: -10 }
+      css/components/links.theme.css: { weight: -10 }
+      css/components/menu.theme.css: { weight: -10 }
+      css/components/messages.theme.css: { weight: -10 }
+      css/components/progress.theme.css: { weight: -10 }
+      css/components/tableselect.theme.css: { weight: -10 }
+      css/components/tabledrag.theme.css: { weight: -10 }
+      css/components/tablesort.theme.css: { weight: -10 }
+      css/components/tabs.theme.css: { weight: -10 }
+      css/components/textarea.theme.css: { weight: -10 }

@madhavvyas, can you check your reroll? This shouldn't be there. This code above was committed the other day so it should already be in head.

New Rerolled patch!

Rerolled!

@davidhernandez When I check in HEAD, I do not found theme code as you specified

/core/modules/system/system.libraries.yml

Did you rebase your branch with 8.0.x? Those CSS file moves, and the change to system.libraries.yml, were done on Sunday. And if you look at the patch in #40 and all the ones before, you'll see the code is not there.

I have rechecked. I have rebased my 8.0.x branch using git pull --rebase
I do not found changes specified in comment #48
@davidhernandez Please advice if you referring any other branch.

@madhavvyas the patch you uploaded in #44 had that code. It should not have been there. Please reroll from #26 or #40, not the patches after.

Let's see if this one works.

Just finished a line by line comparison of the last RTBC patch with the newest patch. The only difference is that the old patch had to take care of moving the item list CSS to Classy and make a few edits in that file, while the new patch only needs to make those edits in the file that has already been moved to Classy in HEAD.

So as far as code goes, this is good to go. We could redo the manual testing, but I'm comfortable with moving this to RTBC since the code is the same line by line.

@davidhernandez thanks for the screenshots. Committed 0e6abff and pushed to 8.0.x. Thanks!