[meta] Update block HTML IDs now that they're plugins

I think this is a situation where we need to probably add default classes to the blocks and transition the css from ids to classes.

Maybe related: #1896098: Add a plugin class to the blocks to identify instances in css

The machine names are very volatile. When you add a block, you can specify it's title, and the machine name changes with the title.

Once the title is localized, the machine name will probably follow, and the css id is broken right from the start.

#1896098: Add a plugin class to the blocks to identify instances in css fixes this issue by adding the block-$pluginid class:

<div id="block-$machinename" class="block block-$module block-$pluginid">

This lets us select the div with div.$module.$pluginid

Capping the acronyms :)

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

<div id="block-$machinename" class="block block-$module block-$pluginid">

This lets us select the div with div.$module.$pluginid

Actually, going by that div you show, the selector would be .block-$module.block-$pluginid.

I think something like this would be clean, but still reliable:

<div id="block-$machinename" class="block $module $pluginid">

Or maybe:
<div class="block $machinename $module $pluginid">

Is there a reason to repeat the "block" string four times for the same element?

tagging for html5/mobile initiative in case they have something to make this go somewhere.

Re-titling.

Agree with Fidelix, if we leave out the block- prefix we gain a lot in readability and verbosity. Any name collisions should be rare and can be dealt with in various ways.

As far as targeting the blocks with JavaScript, decoupling HTML ID and CSS classes from the JS behavior hooks seems to be an emerging best-practice.

CSS classes and HTML IDs should be used for styling, not for hooking in JavaScript functionality, right? I think we've all been stung by jQuery selectors that rely on HTML structure and classes, when the structure and/or classes are changed for visual reasons.

There are different ways to decouple. Attribute data-js or data-hook:

  <section id="login" class="block user pretty-box" data-js="drupal-user-login">
    ...
  </section>
  <script>
    $('[data-js="drupal-user-login"]').methodCall();
  </script>

Feels a little hacky, given the long-winded selector.

Role.js (GitHub repo) extends jQuery/Zepto to add @selectors:

  <section id="login" class="block user pretty-box" role="drupal-user-login">
    ...
  </section>
  <script>
    $('@drupal-user-login').methodCall();
  </script>

Thoughts?

Very few of these blocks are addressed from JS. Most of this is styling.
Also #12 is a larger discussion than this issue.

@12: #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings

I think we should go ahead with below, as suggested by @Fidelix in #8.

<div id="block-$machinename" class="block $module $pluginid">

Initial patch attached to match #15.

I've also attached another patch for testing which removes the if statement around the $block->id() call. Not sure why that's needed so let's see if the tests fail.

OK so it seemed removing the if statement around the $block->id() call didn't break anything.

New patch is based from update-html-ids-1880646-16-test-remove-if-statement.patch and fixes the relevant tests.

Patch makes expected changes and I don't see any syntax or standards issues.

Simplytest.me test:
- Added the Who's Online block to the Highlighted region.
- Added the Who's Online block to the Sidebar first region.
- Got valid HTML

+1 to RTBC

If a contrib module does something fancy like preview a block without an id, then I think that module could pass in a dummy id. I don't think the conditional is necessary anymore either.

New patch attached replaces ":" with -- for plugin_ids as suggested by tim.plunkett.

Updated tests.

Back to NR.

The changes look good.

This used to be titled "HEAD currently contains CSS selector regressions", and I hope this satisfies that need.

+++ b/core/modules/block/block.module
@@ -513,7 +513,10 @@ function template_preprocess_block(&$variables) {
+  $plugin_id_class = str_replace(':', '--', $variables['plugin_id']);
+  $variables['attributes']['class'][] = drupal_html_class($plugin_id_class);

Calling str_replace() in preparation of calling drupal_html_class() looks like a hack. Shouldn't that string replace operation live inside drupal_clean_css_identifier(), which is called by drupal_html_class()?

I'm not against changing the logic of drupal_clean_css_identifier() to replace colons with a double hyphen.

That would however be an API change? I'll put a patch up later and see if it breaks anything.

New patch moves the str replace into drupal_clean_css_identifier(), let's see what the testbot says.

Should we call drupal_clean_css_identifier() directly and use its second argument drupal_clean_css_identifier('this:string', array(':' => '--'));

Or add to the $filter array in drupal_clean_css_identifier()?

I think we should add ":" => "--" to the default filter array in drupal_clean_css_identifier().

New patch attached adding to the default filters.

The proposed solution of:

<div id="block-$machinename" class="block $module $pluginid">

will cause plenty of false-positives when using external JS libraries and possibly CSS libraries.

Both $module and $pluginid are using arbitrary names that may be unique within Drupal, but not universally unique in the frontend space.

Aside from that, adding a built-in ":" → "--" conversion sounds like a very good idea; potentially useful elsewhere.

Is there an argument for keeping $module?

Other than perhaps block menus they look like an edge case, i.e. block-system, block-custom-block do not seem very useful.

Can it be argued that $module can be selected from the $plugin_id using attribute selectors, e.g.:

<div id="block-mytheme-search" class="block block-search-form-block" role="search">
[class*="block-search"]

Are plugin_id's that reliable?

Could something as simple as that work, i.e.:

<div id="block-$block_id" class="block block-$plugin_id">

Great idea, @Jeff Burnz!

I continue to forget that we're able to leverage modern selectors now, so #34 sounds like a very nice and good compromise.

I agree that the use-case for styling all blocks of foo.module is very limited and can definitely be achieved with partial-match attribute selectors.

In fact, this approach resolves yet another hidden issue that existed in the past: Even just the $module and $[plugin_]id sometimes led to ambiguous name clashes, because $[plugin_]id was not namespaced with $module.

The new $plugin_id are always prefixed with $module to my knowledge.

+1 :)

OK I've removed the module name altogether from the block class name. The comment about menu blocks, I guess that's helped somewhat in that they get the extra class block-menu.

#34 suggested:
<div id="block-$block_id" class="block block-$plugin_id">

How about:
<div id="block-$block_id" class="block $plugin_id">

I'm not sure how the block prefix to plugin Id helps us. plugin_id's seem to have the word block in anyway if it was clarity we were after? eg system-menu-block--tools and search-form-block.

If we were adding the word block so we could use attribute selectors for module provide blocks without clashes eg to match block-search than i'd rather just keep the module name as it's own class.

Personally I think it's more readable to have .block.system over [class*="block-system"] in your CSS.

Patch shows my first suggestion without the module name at all and without the block prefix.

On a side note, it's really annoying that you can't preview your comments anymore because you're editing the issue.

I'm not sure how the block prefix to plugin Id helps us. plugin_id's seem to have the word block in anyway

I'm not totally sure how plugin_id works, if block is a required suffix etc, we can follow #1879496: Do we need to worry about plugin ID collisions?, I know I have prepended module name and appended block in a test module I wrote, unless this in enforced programatically the we can't really guarantee it will alway be there. Someone who knows more about this needs to comment.

if block is a required suffix etc

That's a coincidence:

"aggregator_category_block"
"aggregator_feed_block"
"custom_block"
"test_block_instantiation"
"test_cache"
"test_html_id"
"test_xss_title"
"book_navigation"
"recent_comments"
"forum_active_block"
"forum_new_block"
"language_block"
"node_recent_block"
"node_syndicate_block"
"search_form_block"
"shortcuts"
"statistics_popular_block"
"system_breadcrumb_block"
"system_help_block"
"system_main_block"
"system_menu_block"
"system_powered_by_block"
"user_login_block"
"user_new_block"
"user_online_block"
"views_block"
"views_exposed_filter_block"

Thanks tim.plunkett, so yeah, not reliable enough to ensure uniqueness. Even if core sorts this inconsistency out contrib will not, that's a given.

Shifting focus to the id, and wondering if we might be able to remove it, found #1989568: Remove block config ID from being used as an HTML ID or template suggestion.

The only reason I can think of to keep the id is as a fragment identifier?

$variables['elements']['#block']->id() could be a class? EclipseGc actually hints at this #1. I tend to think this fits our idea of BEM better, a unique class per block instance rather than an id. No need for id or chained selectors in CSS.

The fragment identifier is kind of a big deal imo, I wonder if there are any contrib modules that currently rely on this?

The new $plugin_id are always prefixed with $module to my knowledge.

If that's the case I think we should go with:
<div id="block-$block_id" class="block $plugin_id">

The new $plugin_id are always prefixed with $module to my knowledge.

"recent_comments"
That is not prefixed. There is no requirement that they be prefixed.

Yeah I agree with the fragment identifier that's a big one.

What about selecting the block in JavaScript. Id's are still quite a bit faster to select than classes?

Recap:

We need a way to apply CSS:

1. to all blocks
2. to all instances of a particular block
3. to just one instance of a block, preferabley without chaining

Targeting blocks by module "sounds" like a good idea, but in practice it's rarely done. However, if we have the module in the plugin_id this is entirely doable using attribute selectors. If you really need module as a separate class then you can do it in your theme.

This issue could be highly dependant on #1879496: Do we need to worry about plugin ID collisions? , where we can clean things up by adding the module prefix to all plugin id's that don't have it, and remove -block suffix, hopefully. We will then we set a de facto standard for contrib and get nice clean, usable classes. I read into tim.plunketts posts above that there is no technical reason why this cannot happen.

Un-prefixed block classes are prone to "false positives" because contrib and the wider front end world is unknowable. For example users could install a module with a library and it could break their site/design. It's also way more readable in CSS when you see "block-search" as opposed to just "search" and it negates the chained selectors.

So, if #1879496 lands we can do this:

<div id="block-$block_id" class="block block-$plugin_id">

Example selectors:

/* select all blocks */
.block {}


/* select all search module blocks */
[class*="block-search"] {}


/* select all instances of search form block (block-$plugin_id),  existing -block suffix removed */
.block-search-form {}


/* select this single instance of search form block (block-$block_id) */
#block-searchform {}

@benjy afaict, not really much faster, but the fragment and being able to avoid chained selectors are pretty compelling reasons to keep the id I think.

Sorry but I don't agree that avoiding false positives is a good enough reason to apply the "block-" prefix to all blocks.

I believe these clashes would be quite rare and should be dealt with individually.

@Fidelix, I hear your passion, but may I ask what do we gain from not having prefixes? That's the bit I don't really understand in your argument.

IMHO, we get a cleaner, easier to understand HTML.
Overall, it's one of many small steps for improving the default markup, but I believe it helps.

Few things to consider: (some of which is addressed above by a few people)

The IDs can be useful in certain use-cases but in general don't help styles as most front-end devs are moving to class selectors for specificity and ability to override.

I strongly believe if needed it should be easier to define the ID, ala a field that sets the ID of that block instance, or some automatic way. But doing the automatic work for something that may not be needed and just adds bulk the markup may be counter productive. @benjy mentioned a possibility of "Off, Auto, Manual" and I'd suggest "Off" by default.

Auto-generated ones will need to care about duplicate blocks which adds a bit tricky in being consistent and semantic for theming and moving code between dev/stage/prod environments.

I'd like to recommend, turn them "Off" by default the HTML IDs. Allow people who need them to easily manually set in the UI, prepare/preprocess hooks, or in the template and optionally auto-generate as before.

This is part to keep features we have already for those who depend on them and part to set sane and clean defaults.

@EclipseGc and @Jeff Burnz does that fall in line what you mentioned in #1 and #41?

And to a lesser note, I'd prefer to avoid name conflicts especially on generated classes, so prefixing. It's a bit cleaner without prefixing I agree but without it there will need to be more nested selectors to avoid the conflicts which will be annoying to debug and mildly slower to parse by the browser.

#48, OK, so you're talking about HTML, I am more thinking about selectors and how easy they are to understand in the stylesheets.

For example when reviewing a co-workers stylesheet and I see .recent-comments {...} I don't know if it's selecting a block, field, view or some other entity type, i.e. it has no context.

Remember, I'm saying we should get rid of the silly -block suffix that most core blocks have insisted on. I'll even write the freaking patch if I have to. I hate that thing!

I understand you, Jeff, and I agree that it would be more difficult to understand context if the css was like .recent-comments {...}

However, it is the job of your co-workers to apply context where it's needed:
.block.recent-comments {...} and we should say that in the CSS coding best practices docs. IMHO there is nothing wrong with chaining selectors like that, and as you know it's a standard practice of most modern css frameworks and libraries.

@joelpittet:

And to a lesser note, I'd prefer to avoid name conflicts especially on generated classes, so prefixing. It's a bit cleaner without prefixing I agree but without it there will need to be more nested selectors to avoid the conflicts which will be annoying to debug and mildly slower to parse by the browser.

You meant chained selectors? That's what they're for, what you call "conflict" is actually sharing attributes and like I said that's a good thing; and every frontend developer should know how to take advantage of that.

It's not harder to debug in 2014, a time we have excellent CSS/JS inspectors/debuggers in every major browser.

About parse performance... the impact is literally negligible. Even on low-end devices.

@joelpittet

About turning on/off IDs...
I think having that in core would be very confusing.
But there's nothing preventing us from giving contrib the ability to preprocess that. This honestly belongs to a module like https://drupal.org/project/clean_markup

It's a great initiative and it needs help.

Are we not duplicating discussion and code from: #1896098: Add a plugin class to the blocks to identify instances in css A lot seems to be covered there.

What is still relevant from the OP and is this still critical? Isn't this just about the ID and other issues in the OP?

Time to rewrite the issue summary and refocus?

@Fidelix thanks, feel old, chaining never used to work well cross-browser so it's on my internal "ignore this feature till it gets sorted" list.

Updated the issue summary as suggested by Jeff Burnz in #53.

Removed Needs issue summary update tag as I just updated the issue summary.

Per #53 I'm marking duplicate of #1896098: Add a plugin class to the blocks to identify instances in css.

Hm. While #1896098: Add a plugin class to the blocks to identify instances in css does indeed contain a patch, this issue holds a much more in-depth discussion...

Given the block plugin ID list in #40, I think we have these concrete tasks:

1. Remove the leading module name from all block plugin IDs.

   → Code and CSS classes should rely on $plugin->getProvider().

2. Remove the trailing "_block" suffix from all block plugin IDs.

   → Needless duplication, both in plugin IDs as well as generated CSS classes.

3. Make the primary block CSS class "block-$provider-$pluginid".

   → Clean class names that cover all use-cases.

   Note: Double-check plugin IDs of derivative plugins (e.g., menu blocks). To my knowledge, they are compatible and would work like this:

   Plugin ID:  base_plugin_id:derivative_id           →  menu:main
   CSS class:  block-$provider-$baseid:$derivativeid  →  block-menu-menu--main
   

4. Remove the "block-$module" CSS class.

   → Rare use-case. If necessary, this can be achieved with a CSS3 [class*='block-module-'] selector.

If that's the final set of tasks (and if we agree, of course :)), then let's create issues for those :)

(3) would be the existing #1896098: Add a plugin class to the blocks to identify instances in css, of course.

I still disagree with this:

CSS class:  block-$provider-$baseid:$derivativeid  →  block-menu-menu--main

I believe this "block-" prefix is unnecessary.
Otherwise, I'd say we have a clear path forward, thanks Daniel.

Re-opening and turning into a meta issue, since #59 keeps on getting referenced elsewhere (now in issue summary).

I disagree with #58. Unless we somehow change the plugin discovery to be namespaced by provider, we have to prefix them by module, or we risk clashes. Looking at the list in #40, I can only see one example that was not prefixed by module name and that was changed to a view in the meantime.

Removing _block suffix seems like a useful thing, but I'm not sure if that's worth an API/storage change at this point in time?

So, as it was asked a year ago, what is still really necessary and doable here?

I think this is the right priority/status at the moment, I also am not at all clear what needs doing, and this isn't critically breaking anything.

Seems like, we should generate ids by the machine name (since that has to be unique) and classes by the plugin id (since that can be placed multiple times). What is this issue about again?

Eclipse

Not actively part of the Blocks-Layouts work.

3years “block-” still exist...

It is fine to complain but better to update the issue summary, providing justification to reopen the issue. In #63 a committer questioned the issue an no one has updated it.

This is still a thing, and in the latest releases more things are breaking, specifically views utilizing IDs which is a lot of modules, especially when related to javascript enabled layouts.

Been 9 years since this moved to PNMI. Should it be reopened or closed?