Misaligned Icons in Drupal 8.x

Attached is a patch for the same. Though, need to check whether this intended.

This patch only seems to affect Seven, so moving to that component.

This patch only seems to modify the horizontal alignment but the report suggests we need to modify the vertical alignment?

Oh, I overlooked vertical alignment part, my bad. I'll upload a patch for the same shortly.

Actually this issue also happens when Bartik is the main admin theme, this patch should address the problem in both Bartik and Seven. =)

Oh, formatting error, sorry about that, this one should be fine.

The patch works fine and solves all the listed issues.

why the new line on line 156?

I probably accidently added that line while editing the file, this following patch doesn't have this issue. Thanks for noticing.

Please add a space between .messages and {

#10 Yeah I missed the extra line in the patch. But this patch looks good.I can't find any errors.

+++ b/core/modules/system/css/system.theme.css
@@ -540,7 +540,9 @@ ul.tabs {
+.progress .messages {
+  background-position: 10px 25px;
+}

Don't know why you should add this. Doesn't seem like this is part of the solution? I removed these lined and created a new patch.

This is looking better! Thanks.

Would love an "after" screenshot here. Also, tentatively tagging "Needs accessibility review," because I thought that the reason we put the icon right up next to the *top* of a very long error message was for accessibility, though I'm not totally sure about that.

just tested the patch and added screenshots as per webchick's request in
#21.

The reason why this piece of code is there is because without it, the last example given in the description will continue wrong.

+++ b/core/modules/system/css/system.theme.css
@@ -540,7 +540,9 @@ ul.tabs {
+.progress .messages {
+  background-position: 10px 25px;
+}

The following screenshots are the result of the patch 14 that I previously submitted.

Patch number 19 and new drupal version that implemented new icons fixed this issue.

No, it does not fix.

To reproduce this, just run the update script locally with no internet connection. Tested on the latest Drupal 8 dev version.

The reason why it's not working with the AJAX error is because the message is wrapped in <pre>, which has it's own padding and margin depending on the theme (Bartik has a different background colour for it, too).

There doesn't appear to be specific styling for .messages pre: maybe that's what we need here?

As an aside, I created a sandbox module to deliberately create an AJAX error: https://drupal.org/sandbox/mcjim/2227053
Going to /error tries to run a batch which fails, giving a nice error to work with :-)

Added separated style for .messages pre

Patch in #31 is looking good, though there's still mis-alignment until #2220905: Misaligned messages status is resolved.

The only issue is the AJAX error pages, which output the messages to the content rather than to #console, so don't have margin on the left.

Do you have any example where the left margin issue could be tested?

@lauriii I pushed a sandbox project just for this: https://drupal.org/sandbox/mcjim/2227053
You'll need to clone it: https://drupal.org/project/2227053/git-instructions

It's just a module: you can put it in /modules then enable it. Then you can visit http://your-site/error in your browser to trigger an AJAX error.

Thanks for the module. Whats the problem we are discussing? I think I didn't quite get that.

mcjim++

Thanks for the module!

And the patch..

Thanks, @lauriii, patch is almost there!
A couple of suggestions:

Icon position (possibly controversial!):

In system.theme.css, change line 517 from background: no-repeat 10px 18px; to background: no-repeat 10px 17px;. I think the icons need to go up a teeny tiny bit, and this looks right to me.

#console margin on node/add pages

Change margin: 0 2em; to margin: 0 0 0 8px; on line 1503.
This fixes the extra, unnecessary margin on the node/add pages introduced in #2095275: Space above add content form with no overlay

Remove the extra 8px margin from messages--status

Perhaps we could do that in this patch?

43: misalligned_icons_in_drupal_8-2213583-43.patch queued for re-testing.

Error message icon looks ok on Firefox/Ubuntu.

But is the check mark for the status message too low?

Disabling a CSS line fixes it:

This is what it looks like with the CSS line disabled:

@mathieso I agree, I think the check mark is better higher up.
@lauriii Otherwise, I think the patch is good! :-)

+++ b/core/modules/system/css/system.admin.css
@@ -206,6 +206,7 @@ table.system-status-report td.status-icon div {
+  margin-top: 2px;

+++ b/core/modules/system/css/system.theme.css
@@ -514,7 +514,7 @@ ul.tabs {
+  background: no-repeat 10px 17px;  /* LTR */

It seems only 2 lines related to this issue. Please focus on it only.

OK. We can restrict this issue to the misaligned icons only and create a new issue for the misaligned messages themselves or just update the issue summary to reflect what's in the patch. Happy to do either, as long as we end up with nicely aligned messages :-)

I think we can remove separate position for status messages

+++ b/core/modules/system/css/system.theme.css
@@ -545,15 +545,12 @@ ul.tabs {
-  background-position: 12px 19px;  /* LTR */

remaining things looks good.

I can't see an accessibility problem with this patch. What needs to be done to RTBC it?

I think we're going to need before/after screenshots in Seven/Bartik/Stark to get this committed. The changes are very subtle.

51: drupal8-misalligned_icons_in_drupal_8-2213583-51.patch queued for re-testing.

Please review revised patch as patch in #51 no longer applies in the current code.

Patch 59 successfully resolved the problem on the following browsers: Chrome (mac), Safari (mac), Opera (mac), Firefox (mac) and Internet Explorer 8 (Crossover on Mac).

+++ b/core/themes/seven/style.css
@@ -140,7 +140,17 @@ ul.menu li.expanded {
 #console {
-  margin: 9px 0 10px;
+  margin: 9px 0 10px 8px;
+}
...
+.content .messages {
+  margin-left: 8px;
 }

What about rtl?

Added RTL support. Some pictures here:

Rerolled the patch. Here are before and after screen shots.

Before:

After:

Looks like the spacing is off for:

+.messages pre {
+ margin: 0;
+}

I'm also curious about the changes to [dir="rtl"] .messages--status {

I'd just like some comment on that as it's a change from #70 and I don't know why.

The reason for the changes to [dir="rtl"] .messages--status { is this:

Before:

After:

Thanks for the detailed screenshots. The code looks good, I have one question:

+++ b/core/themes/seven/css/style.css
@@ -127,7 +127,23 @@ ul.menu li.expanded {
+.content .messages {
+  margin: 0 0 0 8px; /* LTR */
+}
+[dir="rtl"] .content .messages {
+  margin: 0 8px 0 0;
 }

Is there a good reason to include the .content class in the selector?

Fixed indentation according to #75.

Tested latest patch without the .content class in the selector according to #77
Before:

After:

FYI I created a related issue: #2321121: Remove #console

Thanks, really close with this I think.

I found a little bit of weirdness on the status messssages when testing ltr->rtl

The background position styling for messages--status doesn't kick in but it looks fine

But for RTL it does and it looks wrong.

Maybe we just need to delete the background position styling for messages--status?

I've removed the background position - looks good to me.

I did notice however that the arrows on the expandable detail elements does get quite close to the text - screenshot attached.

Swapped #console to .messages. As said in #2321121, this appears to not cause any further issues.

#2321121: Remove #console was closed as duplicate.

@jamesquinton, @Gábor Hojtsy - closing #2321121: Remove #console means that removing #console from the twig file should be also done here.

1. +++ b/core/themes/seven/css/style.css
   @@ -124,10 +124,23 @@ ul.menu li.expanded {
   +[dir="rtl"] .messages {
   +    margin: 9px 8px 10px 0;
   +}
   +
   +.messages pre {
   +  margin: 0;
   +}
   

   We don't need a blank line here.

2. +++ b/core/themes/seven/css/style.css
   @@ -124,10 +124,23 @@ ul.menu li.expanded {
   +.content .messages {
   +  margin: 0 0 0 8px; /* LTR */
   +}
   +[dir="rtl"] .content .messages {
   +  margin: 0 8px 0 0;
   

   What is this code for? Why do we override .messages when inside .content? Is it for the node creation page?

I didn't find any case where .content .messages would happen. I also removed #console in this patch.

After apply the misalligned_icons_in-2213583-83.patch it looks nice, here I attached an screenshot.
Cheers!

I applied patch from #88 and everything looks ok. See screenshots before and after. Good work!

Is the if statement redundant now?

ie.

      {% if messages %}
        {{ messages }}
      {% endif %}

could become just

      {{ messages }}

Good question.

Hm. The if is used to print a markup container if there is a value to be printed inside it (from the Twig coding standards).

+++ b/core/themes/seven/css/style.css
@@ -124,10 +124,16 @@ ul.menu li.expanded {
+.messages {
+  margin: 9px 0 10px 8px; /* LTR */
+}

+++ b/core/themes/seven/templates/page.html.twig
@@ -86,7 +86,7 @@
-        <div id="console" class="clearfix">{{ messages }}</div>
+        {{ messages }}

Why is the div removed? Should this be something like:
<div class="messages">?

So - here's a quick update.

Right, forget about #94. The {messages} doesn't need a div around it, because it's added in a preprocess. The patch from #88 seems to be fine.

Comment #91 is correct. We can remove the if. I've attached a patch, and tested this patch with @Laurii at Frontend United in Copenhagen. :)

Tested this manually and it works. Fixes the issue pointed by webchick in comment #92

Minor nitpick:

+++ b/core/themes/seven/templates/page.html.twig
@@ -85,9 +85,7 @@
+        {{ messages }}

Extra indentation.

Just removed the extra space as mentioned by @herom in #97. Again pushing to Need Review to ensure get passed through Test Bot.

ok. back to RTBC.

+++ b/core/modules/system/css/system.theme.css
@@ -523,7 +523,7 @@ ul.tabs {
+  background: no-repeat 10px 17px;  /* LTR */

@@ -598,8 +594,9 @@ table tr.warning {
+  background-position: 99.3% 19px;

default background position is '10px 17px'. RTL is diff.

Really don't like use percentage offset value. It has bad result on smaller screen width. Why not use background-origin?? ( http://caniuse.com/#search=background-origin )

Or apply the CSS rule to inner DIV + padding. eg:

[dir="rtl"] .messages--error > div {
  background-image: url(http://s7b6de091c3666dc.s2.simplytest.me/core/misc/icons/ea2800/error.svg);
  background-position: right 2px;
  background-repeat: no-repeat;
  padding-right: 24px;
}

Anybody interested to move this forward? I came from #2318381: Message not styled properly on interface translation page which was postponed on #2321121: Remove #console which was marked as duplicate of this one. So for the sake of these things happening, now that it is all bundled up in this bigger issue, how do we ensure it happens? :)

Here we go.
Needed a reroll after #2321505: Split Seven's style.css into SMACSS categories, which deleted the seven style.css, and moved the #console styling to a separate file. So, there are two interdiffs: one for the reroll, and one to address #101.

Also attached a screenshot that the RTL still looks fine.

EDIT: Oops, posted the interdiffs with .patch. Sorry for the below noise.

It seems like we can use background-position: right 10px top 17px; instead of the percentage so that works great. I don't think I was even aware of that. Well done!

I manually tested the patch just to double check and I think it looks good. Onwards!

Committed 877b9c6 and pushed to 8.0.x. Thanks!

Yay, thanks. Finally unpostponed #2318381: Message not styled properly on interface translation page :) Progress!

Nice work. This got in! Woop woop! :)