Simplify names of "element-x" helper classes

And here's a patch that does that.

In theory I am in support of the change. You really wouldn't believe how much time and effort was spent on building consensus around the current names :) So, as long as all supporting documentation can be cleaned up for D8, and if the patch doesn't miss anything, then +1 from me!

Sure, that's possible (and if it was the last choice, I'd settle for it) but I'd really prefer we that didn't. We are changing all sorts of things about Drupal 8, from markup to CSS to API's, etc... We don't really do this sort of thing - babysitting old/bad code - for upgrade purposes.

In reality it would literally be a quick find and replace fix that would be listed in the upgrade guide. This patch took no more than 10 minutes for me to roll for all of core. It's pretty rare that themes are upgradable for major versions and the worst case scenario here is that elements that are supposed to be invisible will appear and most module developers will notice that immediately and be able to reference it in the upgrade guide.

I hated these classes since I first laid eyes on them, but didn't want to bikeshed and prevent the issues from going forward because they had been going on for a very long time before I got involved. AFAIC, It's completely asinine to have a "element-" prefix on a class, because classes are FOR elements. LOL. It reminds me of the whole "clear-block" vs. "clearfix" thing all over again.

Also... This is sorta related on the documentation/blog post front - The current code for the element-invisible class is totally buggy. I'm not using this code in my themes because it has too many visual issues. I'm currently using this version. It needs to be fixed and so will any documentation or blog posts that reference it. Basically, it's not exactly a model example that is set in stone right now.

Thanks for the feedback @bowersox! We would definitely document it. It would need a change notification after being committed. :)

"The only possible issue I can see with those two generic CSS class names is that an arbitrary module might want to integrate a third-party library (such as an wysiwyg editor) or even simply imports content from another system, where one of those classes had a completely different meaning. In both cases, user interface or content elements will vanish mysteriously. And, we cannot alter these classes later on, because many themes/modules/sites will depend on them. However, using a prefix for them might as well be strange."

This is an edge case IMO. And using a prefix for any plugin/widget is a best practice. Trying to protect against a situation like that is the definition of babysitting bad code, and it's probably the reason why so many of our classes are excessive and convoluted.

Thinking about this more, I think we should just do the same thing that H5BP does, except use .visually-hidden (with a dash):

There is no way to confuse those class names. And visually-hidden is a MUCH better descriptive class name than invisible anyway. As you can see invisible does have a different meaning. There are a slew of people using this code right now. If there were complaints about class name clashes they would have addressed them by now.

I really do think this is an edge case issue and that we need to stop bikeshedding and think about what we want to see here for D8. I want Drupal to be more inline with what is being done in the web standards community. Certain things need to be intuitive and make more sense to developers, and this is one of them.

People literally gave up because there was so much bikeshedding going on. That's why we have this prefix IMO, and I don't think we should keep it, or have that whole bikeshed discussion all over again. We should just think about what the best class is for each, and IMO it's what is pasted above.

Can someone please go find some examples of actual collisions?

Maybe we should just prefix all of our classes with drupal-. Then we will be safe. Sigh.

Marking this needs work to incorporate the H5BP names into the patch.

#1: 1363112-01-simplify-element-x-classes.patch queued for re-testing.

I firmly agree with dropping the element- prefix, this makes things so much cleaner and just *makes sense*. I re-rolled the patch from #1 to cleanly apply to current 8.x HEAD.

I have resolved the issues that made the patch in #17 fail and re-rolled.

Downloaded and applied the patch, no failures. Since we're just altering class names I'm gonna go ahead and make this RTBC.

This patch changes the invisible class to visually-hidden. It also fixes documentation issues and an incorrect jQuery selector in overlay-child.js.

tag

Patch from #26 looks great! Applies cleanly and makes exact changes as described. I fully support this class the idea to adopt boilerplate's naming convention, the less 'unique' instances for stuff like this we have in core, the easier it becomes for new comer's to Drupal to get understand become familar.

Would be nice to get some other eyes on this and mark RTBC.

#28

What's the benefit of matching the exact class name HTML5 Boilerplate uses? How often does 3rd-party code uses the HTML Boilerplate naming convention?

The benefit is following a standard that is already laid out in one of the webs most popular (and forward thinking) front-end frameworks. The less drupalisms we have, the easier it is for people who are already familiar with other frameworks to understand whats being done and how to extend it. Plus - I don't see anything in the coding standards that talks about words having to be separated by dashes. I only remember hearing talk of using dashes over underscores in class names.

CSS Standard: http://drupal.org/node/302199
Markup Standard: http://groups.drupal.org/node/6355

#26: simplify-element-classes-1363112-26.patch queued for re-testing.

The last submitted patch, simplify-element-classes-1363112-26.patch, failed testing.

@mgifford, I consider step (2) to be already finished. We got lots of positive feedback here for moving ahead with this exception to the normal naming convention. The HTML5 and Accessibility teams agree, as do many others who have weighed in.

It looks like the patch needs a re-roll. After that, if one more person reviews it positively it is ready for RTBC.

Going to test this.

@mparker: If you look at the date on the patch in #37 you'll see it was last rolled almost 8 months ago. This is a long time in Drupal core development and therefore A LOT has changed, so this patch does not apply cleanly anymore. Care to try and re-roll?

Yay, testbot likes it!

Someone else, please review.

We'll probably have to notify the theming team about this change.

Will fix.