Problem/Motivation

When implementing hook_toolbar() without #title (or empty) the rendered icon (look for the ? in the main menu) is misaligned, see attached screenshot.

In think the problem is in toolbar.icons.css and specific in the height declaration, because there isn't content. 

.toolbar .toolbar-icon:before {
  height: 100%;
}
CommentFileSizeAuthor
#19 toolbar-css-2267037-19.patch532 bytesvollepeer
#18 regression.png7.66 KBwim leers
#15 toolbar-css-2267037-15.patch382 bytesvollepeer
#11 toolbar-css-2267037-9.patch852 bytesvollepeer
#9 toolbar-css-707484-9.patch852 bytesvollepeer
#8 icons_misaligned.png38.2 KBvollepeer
Schermafbeelding 2014-05-14 om 9.25.02 AM.png82.64 KBmichaelmol

Comments

michaelmol’s picture

michaelmol commented
Issue summary: View changes
michaelmol’s picture

michaelmol commented
Issue summary: View changes
petednz’s picture

petednz commented
Issue summary: View changes
petednz’s picture

petednz commented

can you add your code that is adding the title-less menu entry so others can quickly replicate and try to progress this?

michaelmol’s picture

michaelmol commented

Here the implementation of hook_toolbar, with an empty title element.
I have used the class toolbar-icon-user class so the user icon is rendered. 

/**
 * Implements hook_toolbar().
 */
function MYMODULE_toolbar() {
  $items['MYMODULE'] = array(
    '#type' => 'toolbar_item',
    'tab' => array(
      '#type' => 'link',
     // Empty title element.
      '#title' => '', 
      '#attributes' => array(
        'title' => t('Coffee'),
        'class' => array('toolbar-icon', 'toolbar-icon-user'),
      ),
    ),
  );

  return $items;
}
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Novice

Can you explain why this is not a problem for the Contextual module's toolbar tab, then?

vollepeer’s picture

vollepeer commented
Assigned: Unassigned » vollepeer
vollepeer’s picture

vollepeer commented
StatusFileSize
new icons_misaligned.png38.2 KB

This problem also affects the icons in the Contextual module's toolbar (see attached screenshot). Working on a patch now.

vollepeer’s picture

vollepeer commented
StatusFileSize
new toolbar-css-707484-9.patch852 bytes

Alignment issue can be solved by adding a vertical align for the Contextual module's toolbar icons, and a min-height for all toolbar icons in general. Needs to be tested for cross-browser compatibility.

vollepeer’s picture

vollepeer commented
vollepeer’s picture

vollepeer commented
StatusFileSize
new toolbar-css-2267037-9.patch852 bytes
vollepeer’s picture

vollepeer commented
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/contextual/css/contextual.toolbar.css
@@ -12,6 +12,7 @@
+  vertical-align: top;

    The contextual toolbar tab does have (fallback) text, and is aligned correctly, so I don't see why this is necessary.

    If this is necessary to solve the generic case, then this doesn't belong in contextual.toolbar.css, but in the appropriate Toolbar CSS. 

  2. +++ b/core/modules/toolbar/css/toolbar.icons.css
@@ -20,6 +20,7 @@
+  min-height: 40px;

    Everything is em-based in this CSS file AFAICT, so we should probably stick to that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Also, in principle you must define a title: it's necessary for accessibility purposes — how else are screen reader users supposed to know what a toolbar tab is for? :)

vollepeer’s picture

vollepeer commented
StatusFileSize
new toolbar-css-2267037-15.patch382 bytes

vertical-align: top;
Targets the problem that multiple icons in the Contextual module's toolbar are defined without a title text. I leave this one out as it is an edge case.

min-height: 40px;
Converted to em's.

vollepeer’s picture

vollepeer commented
Status: Needs work » Needs review

The last submitted patch, 11: toolbar-css-2267037-9.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
StatusFileSize
new regression.png7.66 KB

Lovely — nice & simple! :)

I'd love to have RTBC'd this, but unfortunately I found a regression… (this is in Chrome).

vollepeer’s picture

vollepeer commented
Status: Needs work » Needs review
StatusFileSize
new toolbar-css-2267037-19.patch532 bytes

Thanks for reviewing. Fixed regression problem.

Status: Needs review » Needs work

The last submitted patch, 19: toolbar-css-2267037-19.patch, failed testing.

vollepeer’s picture

vollepeer commented
Status: Needs work » Needs review

19: toolbar-css-2267037-19.patch queued for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Perfect, thanks :)

(Manually tested and confirmed, also with different font sizes.)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d82a0ff and pushed to 8.x. Thanks!

  • Commit d82a0ff on 8.x by alexpott: 
    Issue #2267037 by vollepeer | michaelmol: Fixed Toolbar tab icons...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dobe’s picture

dobe commented
Status: Closed (fixed) » Needs work

I am not sure whether to open a different issue or reopen this one. I am dealing with the same regression as #18 in latest dev.

MattA’s picture

MattA commented
Status: Needs work » Closed (fixed)

@dobe Then you will likely want to see #2546196: Toolbar's orientation-toggling arrows broken by #2173527 instead.