Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages()

Might want to use #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() to find examples of marking joined strings as safe.

Postponed on #2505931: Remove SafeMarkup::set in ViewListBuilder. Whatever pattern we find works there can most likely be used here as well.

Per @xjm - this is actionable issue now.

This is wrong one and still postponed.

#2505497: Support render arrays for drupal_set_message() would have helped with this, but that issue is in no danger of being solved and is probably going to be wontfixed.

So one option here is to do a renderPlain() on a render array with the items before returning to the dsm().

This may work once #2505931-170: Remove SafeMarkup::set in ViewListBuilder is in, just needs unit tests.

#2505931: Remove SafeMarkup::set in ViewListBuilder just landed :)

Looks fine for me. Did you checked whether we have a test for that?

Let's manually test?

Also, we need to update the unit test

Marking needs tests as we still need to do the unit tests

Let's fix that fatal error

The 3 spaces after the string "@count errors have been found:" are a bit ugly but let's see if this gets us closer to green, would rather not change the template in this issue.

Looks great. I'd RTBC, but I don't know how to manually test this?

+++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
@@ -24,16 +24,26 @@ class FormErrorHandler implements FormErrorHandlerInterface {
+   * The Renderer service.
...
+   *   The Renderer service.

Nit: s/Renderer/renderer/

Trigger several errors on a form, or alternatively download and install https://github.com/dmsmidt/errorstyle and go to /error-style/form

Tested manually and this looks OK. The only thing is the inline comma list will still need some CSS love but that's a general problem, not sure if that's in scope here?

With patch:

Without patch:

Thanks @stefan.r and @Wim Leers!

1. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
   @@ -589,13 +589,13 @@ function testDatelistWidget() {
   -      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found: Month, Day, Hour, Minute'],
   +      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found:   MonthDayHourMinute'],
   

   Ah, the weird lack-of-spaces in DateTimeFieldTest is a quirk of assertText() and the way the data provider is used.

2. +++ b/core/modules/shortcut/src/Tests/ShortcutSetsTest.php
   @@ -132,7 +132,7 @@ function testShortcutSetSwitchCreate() {
   -    $this->assertRaw('1 error has been found: <a href="#edit-label">Label</a>');
   +    $this->assertText('1 error has been found:   Label');
   
   +++ b/core/modules/system/src/Tests/Form/ValidationTest.php
   @@ -289,10 +289,10 @@ protected function assertErrorMessages($messages) {
   -      $error_links[] = \Drupal::l($message['title'], Url::fromRoute('<none>', [], ['fragment' => 'edit-' . str_replace('_', '-', $message['key']), 'external' => TRUE]));
   +      $error_links[] = $message['title'];
   ...
   -    $this->assertRaw($top_message . ' ' . implode(', ', $error_links));
   +    $this->assertText($top_message . '   ' . implode('', $error_links));
   
   +++ b/core/modules/user/src/Tests/UserBlocksTest.php
   @@ -48,7 +48,7 @@ function testUserLoginBlock() {
   -    $this->assertRaw('1 error has been found: <a href="#edit-name">Username</a>');
   +    $this->assertText('1 error has been found:   Username');
   

   So these hunks are actually removing a bit of the test coverage. In HEAD, they assert both that the error is found and that there is a correct jump link for it (see #1493324: Inline form errors for accessibility and UX).

   So, instead of doing assertRaw() on the whole thing, to avoid unnecessarily testing the template markup, I'd suggest that we split this into separate assertions: one for the error count text, and one for the links to the fragments.

   We could also use that strategy to avoid the weirdness with the spacing in the DateTime test.

3. +++ b/core/tests/Drupal/Tests/Core/Form/FormErrorHandlerTest.php
   @@ -39,9 +40,6 @@ public function testDisplayErrorMessages() {
   -    $form_error_handler->expects($this->at(3))
   -      ->method('drupalSetMessage')
   -      ->with('3 errors have been found: Test 1, Test 2 & a half, Test 3', 'error');
   

   This is also removing test coverage.

Forgot to say, thanks also for the manual testing -- I think the before-and-after screenshots look okay.

Before

After

The CSS can be improved in a follow-up.

We did that loop link checking strategy earlier in this patch, so it just came back around into the patch:)

Follow-up created #2557367: Fix inline list CSS

I think we should address the visual regression in the issue that introduces it - no?

Yes I think that's worth fixing either here, or postponing on #2557367: Fix inline list CSS if it needs more discussion.

The problem here is that a) the div will need a display: inline-block; if it's inside messages__list and b) the margin from this style:

.item-list ul {
    list-style-type: disc;
    list-style-image: none;
    margin: 0.25em 0px 0.25em 1.5em;
}

overrides this one:

ul.item-list__comma-list, ul.item-list__comma-list li, [dir="rtl"] ul.item-list__comma-list, [dir="rtl"] ul.item-list__comma-list li {
    margin: 0px;
    padding: 0px;
}

Should we just fix this with a !important after the 0px in the margin?

The patch attached fixes the issue - it removes the indent and makes comma lists properly inline in all themes.

+++ b/core/modules/system/css/components/item-list.theme.css
@@ -17,17 +17,23 @@
+.item-list ul.item-list__comma-list {

You can't do that. The item-list wrapper doesn't exist in the core template.

We're just looking at the form errors list correct? I'll take a look.

Grr, these things would be easier if we can get that CSS moved to Classy.

I didn't make screenshots for all the RTL but I did look. As usual, someone manually test and doublecheck that it's ok.

p.s. multiple file upload would be really nice to have.

p.p.s I know adding it to messages contradicts my comment in #37, but it's the simplest fix without making out of scope changes. It should be fine if/when we move that file to Classy, which we have an open issue to do.

No margin should be needed LTR or RTL because displaying the comma list inline should get the natural spacing of the text and line height.

Joel, how did you make the "error not related to a real element"? I did not test that. I imagine it would still be ok since the spacing is there in the regression. It should be on the wrapper, not the comma list.

+++ b/core/themes/seven/css/components/menus-and-lists.css
@@ -38,6 +38,7 @@ ul.inline li {
+[dir="rtl"] .item-list .item-list__comma-list{

Needs a space there, but thanks @davidhernandez, that is a bit of a more direct change I had in mind. also +1 to multiple uploads, almost always adding a patch + interdiff, if not crap tone of screenshots;)

A bit of why I opened a follow-up was to avoid bloating the core of this commit/patch with fixing problems that were not part of the IS and weren't a direct cause of this issue.

@davidhernandez used the module mentioned in #23 for the test.

Other than that space mentioned in #42 that can be fixed on commit. I think this is good to go.

Here's a new screenshot set.

Before

After

If this get's kicked back on the CSS one more time, please commit #28 and we can move the CSS to the follow-up.

The fix in #39 is not correct either. The comma list could be used in a success message or any other item list. We need a general fix because of the general styling of lists within lists.

You can't do that. The item-list wrapper doesn't exist in the core template.

I actually think this is wrong. The core styling that is causing problems is

.item-list ul {
  margin: 0 0 0.75em 0;
  padding: 0;
}

so core already has the concept of ul's in item-list classed elements. The answer is to remove this styling and add it to classy where the .item-list wrapper is added and then handle comma lists inside lists there.

The CSS/template changes in #45 look good and 48/49 just did a s/ / / on the error messages to fix tests.

Screenshots from manual testing in #45 look good and did one last manual test on the last patch to confirm (including on the comma-separated item list in the views listing).

The visual regression seems fixed, so this looks ready to me if tests are green now, considering how trivial the last 2 interdiffs are.

The answer is to remove this styling and add it to classy...

Yes, but I agree with Joel that moving all the CSS around is too much to roll into this issue. We can fix that with the plan to move all the CSS.

The comma list could be used in a success message or any other item list.

I targeted the error messages specifically, but that can be changes to just .messages. However, I'm don't think that is correct. All item lists are not inline, this is really only a problem with the comma list, in conjunction with the error message which has the inline label. Other item lists are block displayed uls, so that line .messages--error .item-list has no affect which is fine.

edit: made a typo that hid everything.

@davidhernandez but the comma list needs to be displayed inline regardless of where is it. And I think the changes to the tests in #45 and #47 show that current approach is correct.

+++ b/core/modules/system/css/components/item-list.theme.css
@@ -2,25 +2,12 @@
+ul.item-list__comma-list,
+.item-list ul.item-list__comma-list {
...
+ul.item-list__comma-list li,
+.item-list ul.item-list__comma-list li {

I'm confused, isn't the selector the same twice?

1. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -24,16 +24,26 @@ class FormErrorHandler implements FormErrorHandlerInterface {
   +   *   The Renderer service.
   

   Nit: s/Renderer/renderer/.

2. +++ b/core/tests/Drupal/Tests/Core/Form/FormErrorHandlerTest.php
   @@ -41,7 +42,14 @@ public function testDisplayErrorMessages() {
   +      }));
   +
   

   Nit: Extraneous newline added here.

3. +++ 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: {}
   

   And why is that a problem? Attachments are simply ignored/discarded when using renderPlain().

4. +++ b/core/themes/classy/classy.libraries.yml
   --- /dev/null
   +++ b/core/themes/classy/css/dataset/item.list.css
   

   What's this "dataset" subdirectory?

To clarify #52, this is additional CSS that probably should have been introduced in #2505931: Remove SafeMarkup::set in ViewListBuilder with the original comma-separated list stuff, and we only missed it in that case because the list was on a line by itself. So it makes sense to add it here, though I'd also be okay with a general followup for it. But I agree with @alexpott that we shouldn't add the CSS only for messages.

Nice work on the manual testing!

The comma list is already inline. The issue is the wrapper only because in the context of the error messages that text label is there outside the wrapper. I'm saying I agree there there needs to be a wrapper class for it, that is the root cause (and I new that in the views list builder issue,) but doing so requires making all those changes. Those changes then have to be reviewed outside the context of just the form errors. So if you really want to go that route we have to look at more than form errors.

@dawehner good point! All the .item-list blah belongs in classy.
@Wim Leers
1. Fixed
2. Fixed
3. Yes but we need to add item.list.css when rendering a comma separated list... so doing {{ attach_library('classy/item.list') }} in the item-list.html.twig does not work.
4. The dataset directory is just to match the template's directory... core/themes/classy/templates/dataset/item-list.html.twig

#56 the wrapper is an issue everywhere in classy - it just so happens that so far we have only used comma lists in table columns so we had not noticed.

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

+++ b/core/themes/classy/classy.libraries.yml
--- /dev/null
+++ b/core/themes/classy/css/dataset/item.list.css

Shouldn't this be item-list.css instead of item.list.css?

Rolling a new patch on the assumption the answer to that question is yes as it's consistent with the naming of the other CSS files (seems we use dots for sub-elements and dashes for words).

Given the concerns @davidhernandez, @joelpittet and @xjm (in conversation) are both pointing that the css changes could possibly have a bigger impact.

Uploading #28 again.

So the suggestion I made to @alexpott is that we temporarily let the small visual regression for menus messages in and then fix and test .item-list more broadly in #2557367: Fix inline list CSS since the scope of that is much wider; he'll add the additional work from this issue over there.

Let's fix @Wim Leers feedback in #54 that applied to #28 too.

Also considering that I've reviewed this patch extensively whilst trying to resolve the css issues and did contribute before #28 I feel okay to rtbc this. Even though the changes to FileFieldValidateTest make my eyes bleed.

Let's be done with SafeMarkup::set()

#63++

I fixed those regressions in #39. It's a small change and I don't see why it's an issue if you think it should all be refactored later anyway. I am not confident in any follows getting done, and I don't think we should live with such an obvious visual regression, especially the indent, that we may get stuck with in 8.0.0.

@davidhernandez There is a patch in #2557367: Fix inline list CSS already; hopefully that issue can go forward easily without blocking the security-related critical?

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -589,13 +589,13 @@ function testDatelistWidget() {
-      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found: Month, Day, Hour, Minute'],
+      [['year' => 2012, 'month' => '', 'day' => '', 'hour' => '', 'minute' => ''], '4 errors have been found:   MonthDayHourMinute'],
       // Year and Month selected, validation error on Day, Hour, Minute.
-      [['year' => 2012, 'month' => '12', 'day' => '', 'hour' => '', 'minute' => ''], '3 errors have been found: Day, Hour, Minute'],
+      [['year' => 2012, 'month' => '12', 'day' => '', 'hour' => '', 'minute' => ''], '3 errors have been found:   DayHourMinute'],
       // Year, Month and Day selected, validation error on Hour, Minute.
-      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '', 'minute' => ''], '2 errors have been found: Hour, Minute'],
+      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '', 'minute' => ''], '2 errors have been found:   HourMinute'],
       // Year, Month, Day and Hour selected, validation error on Minute only.
-      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '0', 'minute' => ''], '1 error has been found: Minute'],
+      [['year' => 2012, 'month' => '12', 'day' => '31', 'hour' => '0', 'minute' => ''], '1 error has been found:   Minute'],

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -275,7 +275,7 @@ public function testFieldAdminHandler() {
-    $this->assertText(t('1 error has been found: Test Entity Reference Field'), 'Node save failed when required entity reference field was not correctly filled.');
+    $this->assertText(t('1 error has been found:   Test Entity Reference Field'), 'Node save failed when required entity reference field was not correctly filled.');

@@ -286,7 +286,7 @@ public function testFieldAdminHandler() {
-    $this->assertText(t('1 error has been found: Test Entity Reference Field'));
+    $this->assertText(t('1 error has been found:   Test Entity Reference Field'));

+++ b/core/modules/file/src/Tests/FileFieldValidateTest.php
@@ -35,7 +35,7 @@ function testRequired() {
-    $this->assertText('1 error has been found: ' . $field->label(), 'Node save failed when required file field was empty.');
+    $this->assertText('1 error has been found:   ' . $field->label(), 'Node save failed when required file field was empty.');

@@ -57,7 +57,7 @@ function testRequired() {
-    $this->assertText('1 error has been found: '  . $field->label(), 'Node save failed when required multiple value file field was empty.');
+    $this->assertText('1 error has been found:   '  . $field->label(), 'Node save failed when required multiple value file field was empty.');

@alexpott and I discussed whether to change these assertions in this issue to do multiple assertText(), but the problem with that is that that also reduces the test coverage. Ideally these tests should check for the jump links like the others do, but it makes sense to do that in a followup as well.

Discussed at length with @davidhernandez and @xjm in IRC. @davidhernandez suggested just fixing the indentation in seven. That seems a good compromise.

Missed a bit. Thanks @davidhernandez.

I'm fine with that in #68. I'm not RTBCing because I don't know anything about the rest of the patch.

That works for me for now too.

Thanks everyone and see you in #2557367: Fix inline list CSS! Committed and pushed to 8.0.x.

Should this be marked "Fixed"?

Yes. :)