Clean up toolbar css

The issues with toolbar.css and toolbar-rtl.css can be found here.

Updated the toolbar.css and toolbar-rtl.css and best as i could thru CSS Lint. Some warning were still left behind. thanks

I think this can be improved, specifically the selector choice (remove over qualified selectors) and the CSS specificity.

We could use:
.toolbar .toggle

Even .toggle alone works but it's a poorly named selector (.toolbar-toggle might be better, but that's another issue)

Instead of:
.toolbar .toolbar-menu a.toggle

Ultimately it will make the CSS more efficient/performant and reduce CSS specificity (making it easier to override in themes) along with reducing the overall amount of CSS (easier to maintain).

This will be addressed in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop.

Css clean up:
Mostly color hex shortened to 3-digit equivalent : ex. #ffffff to #fff;
And long number after the dot, ex: 1.3333 to 1.33 - the difference mostly is 0px, thought can be 1 px for retina displays.

This patch does not apply anymore.

Added tags: toolbar-followup, Spark

I have attached a patch which attempts to clean up some of the toolbar css (which is known not to impact any styles or require any changes outside the CSS files).

The changes introduced are:

1. Cleans up unnecessary longhand hex codes.
2. Cleans up unnecessary granular measurements ie 0.3333 to 0.33. Tested all of these on a retina and non-retina screen and found there to be no difference in the rendering whatsoever.
3. Fixed a bug where a declaration is outside of the relevant CSS block:
   

     border-left: 1px solid #aaaaaa;
   [dir="rtl"] .toolbar .toolbar-tray-vertical {
     border-right: 0 none;
     box-shadow: 1px 0 5px 2px rgba(0, 0, 0, 0.3333);
   }
   

4. Removes unnecessary adjoining classes where they do not impact the selector precedence (not in the case of the icon file or toolbar alignment).
5. Removes overqualified element selectors such as "li.active".
6. Changes the "#toolbar-administration" selectors to ". toolbar" to be more consistent with the other selectors and encourage flexibility.
7. Fixed coding standards with regards to some whitespace.

Let me know if there are any issues with the patch or it doesn't apply and I can fix them up.

First: thank you for working on this! :)

Overall, all your changes look great. I just have a handful of questions and reservations (note that I'm not the maintainer for this module, I'm only trying to help).

1. +++ b/core/modules/toolbar/css/toolbar.icons.css
   @@ -2,13 +2,12 @@
   -
   

   We always leave a newline after the @file docblock. Please restore this :)

2. +++ b/core/modules/toolbar/css/toolbar.icons.css
   @@ -20,7 +19,7 @@
   +  left: 0.67em; /* LTR */
   ...
   -  left: 0.6667em; /* LTR */
   
   @@ -30,19 +29,19 @@
   -  right: 0.6667em;
   +  right: 0.67em;
   
   +++ b/core/modules/toolbar/css/toolbar.theme.css
   @@ -153,7 +154,7 @@
   -  padding: 0.6667em;
   +  padding: 0.67em;
   
   @@ -141,7 +142,7 @@
   -  padding-left: 0.6667em; /* LTR */
   +  padding-left: 0.67em; /* LTR */
   

   Shouldn't this be 0.66 instead of 0.67?

3. +++ b/core/modules/toolbar/css/toolbar.module.css
   @@ -5,14 +5,14 @@
   -#toolbar-administration {
   +.toolbar {
   

   I understand the rationale ("ID selectors are bad!" & "this is the selector you use elsewhere!"), but I think this might be written using an ID selector because .toolbar is a class that some other module might use too, think a complex admin UI, a WYSIWYG editor, and so on.

   I am in favor of this, but I think we might want jessebeach to confirm that this is okay; she might have had good reasons to do it this way.

4. +++ b/core/modules/toolbar/css/toolbar.theme.css
   @@ -164,7 +165,7 @@
   +  padding: 0.17em;
   ...
   -  padding: 0.1667em;
   

   This looks like it might need to be 0.166?

Hi Wim, thanks for the review!

I believe 0.167 is closer to 0.166.. than 0.166. Same goes for 0.67 being closer to 0.66.. than 0.66. I believe the recurring digits round the trailing decimal up. That being said in the case of "0.17", I have added the additional "6" for clarity, "0.167".

Thanks for the @file docblock pickup. I have changed the other 3 blocks to be consistent and include the new line.

With regards to the ID/class discussion, there are lots of examples that would mess up contrib modules who used the "toolbar" class. It should probably either be changed across the board (I'm happy to open up another issue for it), or be consistent across all of the other toolbar selectors.

.toolbar a {
  cursor: pointer;
  padding: 1em 1.33em;
  text-decoration: none;
}

With regards to the ID/class discussion, there are lots of examples that would mess up contrib modules who used the "toolbar" class. It should probably either be changed across the board (I'm happy to open up another issue for it), or be consistent across all of the other toolbar selectors.

I personally think that's fair.

I'll let jessebeach address the remaining feedback.

I'm more of a foster maintainer until we find someone permanent =D

PerthSam152, thanks for taking this work on. Here are my comments.

1. +++ b/core/modules/toolbar/css/toolbar.icons.css
   @@ -30,19 +30,19 @@
    }
   -.toolbar button.toolbar-icon {
   +.toolbar .toolbar-icon {
      background-color: transparent;
      border: 0;
      font-size: 1em;
    }
   

   I, like you, dislike needless over-qualification of selectors. However, specificity is not in and of itself a bad thing; it's simply a power not to be abused.

   In this particular case, I do mean to target just button elements. These elements have background-color, border and a wonky font-size that need to be removed in order to make this element appear like a link.

2. +++ b/core/modules/toolbar/css/toolbar.menu.css
   @@ -18,7 +19,7 @@
    }
   -.toolbar .toolbar-tray-vertical li.open > ul {
   +.toolbar .toolbar-tray-vertical .open > ul {
      display: block; /* Show the sub-menus */
    }
   

   Again, this specification is intended. I want li.open > ul relationships, not div.open > ul relationships.

3. +++ b/core/modules/toolbar/css/toolbar.menu.css
   @@ -29,7 +30,7 @@
    .toolbar .toolbar-tray .active-trail > .toolbar-box a,
   -.toolbar .toolbar-tray a.active {
   +.toolbar .toolbar-tray .active {
      color: #000;
      font-weight: bold;
    }
   
   +++ b/core/modules/toolbar/css/toolbar.module.css
   @@ -5,14 +5,15 @@
     */
   

   We can take out the specificity here. Even better would be to namespace the .active class. That's not something we need do in this issue.

4. +++ b/core/modules/toolbar/css/toolbar.module.css
   @@ -5,14 +5,15 @@
     */
   -#toolbar-administration,
   -#toolbar-administration * {
   +
   +.toolbar,
   +.toolbar * {
      -moz-box-sizing: border-box;
      -o-box-sizing: border-box;
      -webkit-box-sizing: border-box;
      box-sizing: border-box;
    }
   

   I suggest not addressing these declarations in this patch. We do in fact want the specificity of an ID here, not a class, because the Toolbar is not like other components in the sense that we want it to be bomb-proof against outside styling influences.

   We've had a lot of experience using the Navbar module on live sites (the D7 backport of Toolbar) and feedback has been that the CSS of Toolbar/Navbar is not specific enough! I know, my initial reaction is probably like yours: "well, your theme CSS is too generic and not well formed." But we can't take that approach here. We really need to make the Toolbar a styling-isolated component. Here are the navbar issues for reference. I would love love love to have your expert help porting these Navbar changes forward to Toolbar. For reference:

   #2158771: CSS hardening
   #2120029: Navbar does not override core "ul.menu li" (system.menus.css)
   #1757466: Prefix all navbar classes to prevent theme clashes
   #1938742: Navbar should override margin in system.css (ul.menu li)

5. +++ b/core/modules/toolbar/css/toolbar.module.css
   @@ -27,7 +28,7 @@
    .toolbar .item-list li,
   -.toolbar .menu li.expanded {
   +.toolbar .menu .expanded {
      list-style-type: none;
   

   This qualification is intended and necessary to override system styling (IIRC, I might be proven wrong here).

6. +++ b/core/modules/toolbar/css/toolbar.theme.css
   @@ -64,21 +65,22 @@
   +  box-shadow: -1px 0 5px 2px rgba(0, 0, 0, 0.33); /* LTR */
    }
   -  border-left: 1px solid #aaaaaa;
   +
    [dir="rtl"] .toolbar .toolbar-tray-vertical {
   

   I posted a separate issue for this. Whichever of these gets committed first will determine where it gets fixed.

4 decimal points

I like to take repeating decimals out to 4 digits because of rounding inconsistencies in browsers:

http://ejohn.org/blog/sub-pixel-problems-in-css/

It doesn't put any appreciable strain on the browser and it might help avoid sizing inconsistencies. A thousandths place rounding inconsistency takes a thousand blocks lined up to become apparent. Personally I don't think we need to change these. If you're confident that reducing the accuracy of the number won't result in broken layouts, then I'm fine to lop off digits.

In sum, the hex color value changes go in, we should reverse a couple of the de-specification changes and leave out the removal of the ID selector (pending downstream changes to harden the CSS).

Again, PerthSam152, so glad to have you working on this code. I hope my comments help fill in some of the folk knowledge for this module.

Hi jessebeach,

Thanks for the review! I will take a look at each of the points you raised in a new patch.

I will admit, shortening the decimal places was mostly because of #6. To me it really depends on whether reducing them makes the CSS easier to work with and has zero side effects. I'll leave those particular changes out of the next patch.

- Sam

1. Withdrawn.
2. Withdrawn.
3. All good.
4. Annoying, but I can understand why we need to delay this.
5. The core selectors that adds "disc" and "menu-collapsed.png" is ".menu .expanded". This should be fine if you're okay with it.
6. All good.
7. re: decimals. I don't have IE spooled up in a VM right now otherwise I probably would have tested it and left them in there, but for now they can remain out of this patch for now.

Attached patch against HEAD.

Re-rolled because of #2163231: RTL styling was jumbled while fixing the disappearing Toolbar tray box-shadow issue.

18: 1663198-Clean-up-toolbar-css-18.patch queued for re-testing.

18: 1663198-Clean-up-toolbar-css-18.patch queued for re-testing.

18: 1663198-Clean-up-toolbar-css-18.patch queued for re-testing.

We had a broken test in core, #2142989: Test for BundleConstraintValidator which has now been reverted.

Very minor fix of a rule that wasn't deleted.

Tagging from the global sprint.

Patch no longer applies. Rerolled.

+++ b/core/modules/toolbar/css/toolbar.icons.css
@@ -20,14 +19,14 @@
-  left: 0.6667em; /* LTR */
+  left: 0.67em; /* LTR */

I was wondering about these lines and the kind of effect they would have. It still seems like these em values are still converted into sub pixels. I think we have to modify them so they round to whole pixels, right?

Forgot to upload the image

#31: I pinged Jesse Beach, asking if she could explain why she did it that way.

I was wondering about these lines and the kind of effect they would have. It still seems like these em values are still converted into sub pixels. I think we have to modify them so they round to whole pixels, right?

I tend to want more significant digits in my values, rather than fewer so that rounding errors are avoided. That being said, if the change works visually, there's nothing inherently wrong with it.

I'd rather keep the units more specific as well. It feels like we are reducing the digits just so it looks cleaner? It would be nice if the em values calculated to whole pixels, that feels cleaner to me.

OMG OMG! Excited to see what you will do, Emma! :)

I have rounded up or down the px values computed by the browser from the current em values found in the patch and files that the patch affects.

I then converted these into em values based on the base font size of toolbar which is 13px.

Here are the values 'calculated' to save others time (original patch value -> rounded to px -> converted to ems)

1.33em -> 17px -> 1.30769230769em
0.67em -> 8px -> 0.69230769em
0.17em -> 2px -> 0.15384615em
2.75em -> 36px -> 2.769230769em
0.25em -> 3px -> 0.23076923em
3.75em -> 49px -> 3.7692307692em

I also noticed that the toolbar was getting it's line height from the admin theme. So I added a line-height value in rems in the same place as where the base font size in rems is defined.

   font-size: 0.8125rem;
+  line-height: 1.25rem;

I have also noticed a bug in the patch from at least #30. When the toolbar is in vertical mode and you have many menu levels expanded you can't scroll down to the very bottom of them. You can on core right now. So in the example below I tried to scroll to see the toolbar toggle orientation button at the bottom but the bottom part is cut off see screenshot...
 

 
So someone needs to fix that as I am struggling to work it out. But otherwise I hope I made some improvements to this issue.

This is weird, because it looks like it's being hidden using Javascript. I don't see why this patch would affect this, as it's CSS only.

@Wim Leers, any ideas?

Some @Wim Leers magic would be great! I think as some of the selectors have changed there is now definitely weird behaviour happening with the vertical toolbar.

Now your doing work on the css & cleaning it up, its important that we remember to name things the right way.
i cretead an issue that maybe should be merged into this?
#2419135: Change the used CSS classes to follow the coding standards . in short:
* use oocss naming
* js- prefix on classes thats used by javascript
* seperate classes out them so a themer can without breaking functionality redesign a toolbar ;)

#1944572: Remove "ul.menu" dependency to prevent theme clashes

There seems to be a lot going on with different issues that all aim to fix the styles / markup of the toolbar. Using lewisnyman's tool for linting the css for errors only refer to two known errors. They are:

Line	Column	Message
11	3	Unknown property 'tap-highlight-color'.
15	3	Unknown property 'touch-callout'.

These are addressable issues. As it turns out, both of these properties only exist if you append "-webkit-" in front of them. It's likely that they were included through some sass-based mechanism.

Furthermore the one instance of the -webkit-tap-highlight-color that is used is also present in the normalize.css file

-webkit-tap-highlight-color: rgba(0,0,0,0);

I think it's OK to keep what toolbar.css is doing to the toolbar as it's reasonable to assume that a developer would want to exclude normalize.css from their theme.

The following patch shows what I think we can do to improve our linting score for toolbar.css

ah there's a previous patch in #40 but it doesn't apply.

Found this issue in RTL

I would like to point out this issue too:

https://www.drupal.org/node/2494235

Check toolbar toggle button visibility patch please.

Look is good for me!
+1 for rtbc

Just checked, looks good.
+1 for rtbc

screenshots after patch apply

Looks nice to me, +1 to RTBC

Please, correct vertical aligning in button on horizontal toolbar.

Pseudo selector set to 1em because of all icons are 16x16.
Screenshots provided:
http://prntscr.com/8dt5t6
http://prntscr.com/8dt5q1

It looks like all patches after #40 are missing a lot of the changes?

Related #2546196: Toolbar's orientation-toggling arrows broken by #2173527

By now we have stylelint so if we make any kind of changes like this the stylelint config needs to be updated.

Patch #59 is failed to apply to 9.1x.
Here is the fixed review patch attached screenshot for the reference.

@komalkolekar looks good.
Probably a code review is needed before it can be moved to rtbc

Testing on 9.5.x I'm no longer able to replicate. Going to close as cannot reproduce. if you are still seeing the issue please reopen and provide an issue update.

I installed the Drupal version 9.5.0-dev in my local. After that I installed the latest Drupal contrib module admin_toolbar with version 3.1 using composer: $ composer require 'drupal/admin_toolbar:^3.1'
It is working fine for me. I didn't find the issue. If you have an older version of the module then you can update your module. If there is no issue then please close that issue. If you still facing the issue then reopen and please reupdate the issue.

@ZenDoodles
Please check this issue once again. I have tested and verified

This is working fine without applying any patch