Title says it all :) Here's the faulty generated code :

[...]
<ul id="toolbar-menu"><li class="menu-15 path-admin-content first"><a href="/admin/content" id="toolbar-link-admin-content" class="to-overlay"><span class="icon"></span>Content</a></li>
[...]
<code><li class="menu-126 path-admin-content"><a href="/drupal-HEAD/admin/content" id="toolbar-link-admin-content" class="to-overlay"><span class="icon"></span>Find content</a></li>
[...]

I propose to append the mlid to the path based CSS ID, which will make it really unique, here's the patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TheRec’s picture

In my code example, there is a mistake, the path of the second link is "/admin/content" too ;)
And the generated code once you applied the code should be :

[...]
<ul id="toolbar-menu"><li class="menu-15 path-admin-content-15 first"><a href="/admin/content" id="toolbar-link-admin-content-15" class="to-overlay"><span class="icon"></span>Content</a></li>
[...]
<li class="menu-126 path-admin-content-126"><a href="/admin/content" id="toolbar-link-admin-content-126" class="to-overlay"><span class="icon"></span>Find content</a></li>
[...]

mlid could vary depending on your installation, so the value of the CSS ID could not be exactly those for you, but they should be different between those two links.

mr.baileys’s picture

Status: Needs review » Needs work
Issue tags: +Quick fix

Ugh, invalid markup, yuck. Good catch, definitely needs fixing...

Quick review of the patch:

  1. +      $id = str_replace(array('/', '<', '>'), array('-', '', ''), $item['link']['href']).'-'.$item['link']['mlid'];
    

    String concatenation operator should have spaces before and after (http://drupal.org/coding-standards#concat).

  2. mlid could vary depending on your installation...

    I'm wondering if this could create issues for themers. If the mlid is not guarantueed to be the same across sites, the id becomes unreliable to target specific menu items. As an alternative, the current id could become a class value instead, and the id could be shortened to something like toolbar-item-<mlid>?

Setting to CNW for #1, #2 is up for discussion...

TheRec’s picture

About 1., Ok, it will be corrected in the follow-up(s).

About 2. Themeing those with a CSS class does not make more sense (to me at least), for example the menu items that made this "bug" apparent are in two different menus (a submenu of Mangement and Administration Shortcuts), cases where this would be useful are rare in my opinion because you'll change both menu item apparence.
To theme them, you'd usually use their parent CSS class and the HTML element as selector (as it is done in toolbar.css) to theme all the menu items, and in last resort (if you need to theme a particular menu item) use the menu item ID. If you need something more, you can always override theme functions.
I like the idea of path-based CSS ID, it makes it easier to maintain your CSS (toolbar-item-<mlid> would remove this concept), but they must be unique.
I'd like to hear more ideas about how this CSS could be generated so that it would be really unique, one which would not turn into something too big (i.e. not keeping a registry of all the CSS ID already used or similar gas factories) for this small task. Using the text of the link or the menu title is not realistic either, because you could have two items with the same text and path. This left me with the solution of the mlid, until now I always used this trick when theming with Drupal 6, but it ties the theme to one install usually, which is not a valid solution for core, so we should maybe drop the idea of unique ID in core and leave that to themers (or modules like menu_attributes) ?

Gábor Hojtsy’s picture

mlid can totally depend on the order of menu items created, so it is not possible to give predictable icons to items with these IDs. There is <ul id="toolbar-menu"> and <ul id="toolbar-shortcuts"> to match for the CSS, so we can just make this a class instead of an ID and allow for multiple items to have the same class. The CSS can assign icons to items under #toolbar-shortcuts, but not under #toolbar-menu.

TheRec’s picture

Title: "Content" button and "Find content" use the same CSS ID and produce invalid XHTML » Use path based CSS classes instead of ID to style toolbar items individually (for icons)
Assigned: TheRec » Unassigned
Status: Needs work » Active

Ok so mlid are not acceptable. And we require a way to theme shortcut individually to be able to assign them icons, but we cannot give unique CSS ID's reliably, so this is why I repurpose this issue and leave it up for grabs.

We discussed on IRC about it with Gábor Hojtsy and we decided that we should drop the current ID on the <a> and that themers could use the path based class of the <li>. For example to style the "Find content" shortcut, the CSS selector would be #toolbar-shortcuts li.path-admin-content. It removes the ID collisions and gives enough space to theme items individually. It should be noted that if there are two items with the same path in the same menu, the generated class will be the same for both, and it could cause problems for themers, in this edge case there is always the possibility for them to use theme override functions (menu) or contributed modules as mentioned before.

On this clean base of thoughts, the way the $link['href'] is cleaned to make CSS id/class is not reliable when there menu items point to an URL with a querystring or for external URL... so we should put some work in this at the same time.

TheRec’s picture

Status: Active » Needs review
FileSize
2.45 KB

Ok, this path does what I described in my previous message.
No more ID for the links, but they are themable individually with the class of their parent which is based on the path. The way used to clean the path has been changed and now should be exhaustive and always meet the CSS2 W3C recommandation for CSS identifiers. Tested it with :

  • Relative URL (Internal Drupal paths) : admin/content generates path-admin-content
  • Relative URL with querystring : admin/content?test1=val1&test2=val2 generates path-admin-content-test1-val1-test2-val2
  • <front> special case : <front> generates path-front
  • Absolute URL : http://www.example.com/ generates path-http-www-example-com
  • Absolute URL with querystring : http://www.example.com/?test1=val1&test2=val2 generates path-http-www-example-com-test1-val1-test2-val2

Status: Needs review » Needs work

The last submitted patch failed testing.

TheRec’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
TheRec’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

TheRec’s picture

Status: Needs work » Needs review

Patch applies and tests pass locally.

yoroy’s picture

Issue tags: +icons

tag

cosmicdreams’s picture

FileSize
2.94 KB

used patch -p0 < 551172-toolbar-css-class_2.patch to apply the patch. The patch failed to apply to today's latest CVS.

here's the .rej file.

cosmicdreams’s picture

I'll give this another try soon.

cosmicdreams’s picture

Issue tags: -Quick fix, -icons

#8: 551172-toolbar-css-class_2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Quick fix, +icons

The last submitted patch, 551172-toolbar-css-class_2.patch, failed testing.

cosmicdreams’s picture

ah good, then it isn't just me.

eigentor’s picture

Here is the issue that plans to build on this patch: adding icons to "Add content" and "Find content" #735656: Iconize most important Items

amc’s picture

subscribe

tadityar’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.72 KB

Status: Needs review » Needs work

The last submitted patch, 20: 551172-toolbar-css-class_20.patch, failed testing.

tadityar’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
736 bytes

Trying to remove those exceptions.

Status: Needs review » Needs work

The last submitted patch, 22: 551172-toolbar-css-class_22.patch, failed testing.