Updated: Comment #100

Note the patch in #70 was committed to core as da1b134.

Problem/Motivation

Originally,

  • Old classes don't describe how they affect screenreaders.
  • Old classes are overly verbose.
  • Old classes don't follow a standard familiar to people outside the Drupal community.

After #70 was committed, the following problem was raised:

  • CSS classes without prefixes can sometimes interfere with classes in a theme.

Proposed resolution

Originally,

  1. Rename classes to better describe how they affect screenreaders
  2. Reduce the amount of typing if possible.
  3. Have the classes follow the HTML5 Boilerplate standard.

After #70 was committed,

  1. Add the prefixes back or rollback the patch.

Remaining tasks

  • Decide whether to add back the prefixes or rollback the patch
  • If we decide to add the prefixes back, we'll have to:
    1. Decide what prefixes to add.
    2. Update any code that uses these classes.
    3. Update #2046301: CSS regression on admin/modules: overlapping "Enabled" table header not properly invisible depending on whether it's been committed at that time.
    4. Update the change notice ([#2022859: Simplified names of "element-x" helper classes]) and documentation pages [#2023177: Upgrading 7.x themes to 8.x], [#388372: Standard Drupal core styles and classes] and [#2023297: D7 to D8 Upgrade: Generated HTML].
  • If we decide to rollback, we'll have to:
    1. Rollback or fix any issues that use these classes.
    2. Rollback or fix #2046301: CSS regression on admin/modules: overlapping "Enabled" table header not properly invisible depending on whether it's been committed at that time.
    3. Delete the change notice ([#2022859: Simplified names of "element-x" helper classes]) and update documentation pages [#2023177: Upgrading 7.x themes to 8.x], [#388372: Standard Drupal core styles and classes] and [#2023297: D7 to D8 Upgrade: Generated HTML].

User interface changes

This issue makes no changes to the user interface.

API changes

This issue makes a change to the CSS "API" used by core, modules, and themes.

Original report by Jacine

Something that has really bothered me since these classes were originally introduced is how overly verbose they are. I see no reason these classes need to be so verbose and would like to see them follow the HTML5 Boilerplate (CSS in Github from v4.3.0).

Before After
element-hidden hidden
element-invisible visually-hidden
element-focusable visually-hidden.focusable
invisible

Copying from Boilerplate, the CSS for invisible should go in core/modules/system/css/system.module.css and look like:

/**
 * Hide visually and from screen-readers, but maintain layout.
 */
.invisible {
  visibility: hidden;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jacine’s picture

Status: Active » Needs review
FileSize
28.41 KB

And here's a patch that does that.

Jacine’s picture

Issue tags: +Accessibility

Tagging. The patch only changes the names of the classes, but the accessibility team should be aware.

Everett Zufelt’s picture

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!

bowersox’s picture

What about allowing both the new short names and the older, longer names?

The patch could be more like this:

-.element-hidden {
+.hidden, .element-hidden {
   display: none;
 }

We've spent lots of energy teaching themers to use "element-invisible" rather than "display:none". I would suggest we keep those old class names working. That would be less upgrade effort when themers move from D7 to D8.

Jacine’s picture

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.

bowersox’s picture

@Jacine, that's fine with me. As long as we document this change for D8, I agree with you that it is a search-and-replace solution.

Jacine’s picture

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

mgifford’s picture

Really have to echo this "You really wouldn't believe how much time and effort was spent on building consensus around the current names :)" - the name change would certainly make it easier to remember. We need to get back to why we went with element-x in the first place though. This was brought in by here:
http://drupal.org/node/473396#comment-1828830

To quote @sun:

"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."

I do think we need to give some thought about how we look at this for D8. I've got a thread here with issues to discuss:
http://groups.drupal.org/node/120224

Jacine’s picture

"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):

/* Hide from both screenreaders and browsers: h5bp.com/u */
.hidden { display: none !important; visibility: hidden; }

/* Hide only visually, but have it available for screenreaders: h5bp.com/v */
.visuallyhidden { border: 0; clip: rect(0 0 0 0); height: 1px; margin: -1px; overflow: hidden; padding: 0; position: absolute; width: 1px; }

/* Extends the .visuallyhidden class to allow the element to be focusable when navigated to via the keyboard: h5bp.com/p */
.visuallyhidden.focusable:active, .visuallyhidden.focusable:focus { clip: auto; height: auto; margin: 0; overflow: visible; position: static; width: auto; }

/* Hide visually and from screenreaders, but maintain layout */
.invisible { visibility: hidden; }

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.

Owen Barton’s picture

I feel that there is too high a probability of collision here if we move to shorter names. Given the large number of libraries and other things that are pulled into Drupal, there is a good chance that these would collide with something, especially since JS stuff is often hiding/showing things and hence quite likely to hit on these terms (and traditionally often bad at namespacing, especially in css).

Jacine’s picture

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.

dcmouyard’s picture

I agree with Jacine’s suggestion on using the same class names as the HTML5 Boilerplate.

@Owen I'm not too worried about collisions—most 3rd-party widgets and libraries namespace they're css selectors.

Jacine’s picture

Status: Needs review » Needs work

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

mcjim’s picture

+1 for same class names as the HTML5 Boilerplate. Makes perfect sense.
Cleaner, clearer and more elegant.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility, -html5

Status: Needs review » Needs work
Issue tags: +Accessibility, +html5

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

tlattimore’s picture

Status: Needs work » Needs review
FileSize
28.76 KB

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.

Status: Needs review » Needs work

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

tlattimore’s picture

Status: Needs work » Needs review
FileSize
27.86 KB

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

dcmouyard’s picture

The patch in #19 looks good to me using Stark, Bartik, and Seven.

I tested in Linux and Mac on Chrome, Safari, and Firefox. I haven't tested it in Windows or IE.

LSU_JBob’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Category: feature » task
Status: Reviewed & tested by the community » Needs work

This is not using the H5BP names as suggested in #13, and I think that was a good idea, so moving back to needs work.

dcmouyard’s picture

Status: Needs work » Needs review
FileSize
30.65 KB

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

bowersox’s picture

@Everett Zufelt pinged @Jacine about this, and they both feel we should exactly match the HTML5 Boilerplate naming convention. So the name should be "visuallyhidden" without a hyphen. The whole idea is that we match HTML5 Boilerplate exactly because so much existing code is written against that. I am also comfortable with this.

@dcmouyard, thanks for moving this forward!

nod_’s picture

Issue tags: +JavaScript

tag

bowersox’s picture

Re-roll of 25. The only difference is the name is now "visuallyhidden" rather than "visually-hidden", in order to match HTML5 Boilerplate exactly. Also a few whitespace changes removed.

Please review and test. Make sure to clear all caches after applying the patch. Verify that the invisible stuff still works right -- like the Skip to main content link and the visually hidden text on active vertical tabs, for example. It works for me in Firefox 12 and Safari 5.

tlattimore’s picture

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.

dcmouyard’s picture

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 Drupal coding standard states words should be separated by dashes in css classes.

tlattimore’s picture

#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

Everett Zufelt’s picture

Regardless if there may be a Drupal standard stating that ashes are preferred / required, we have buyin from Core markup / accessibility maintainers to special case for this limited use-case. The benefit is not only for existing code, but existing understanding and future training.

mgifford’s picture

Issue tags: +JavaScript, +Accessibility, +html5

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

Status: Needs review » Needs work

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

mgifford’s picture

Status: Needs work » Needs review

What needs to be done to mark this RTBC?

1) Apply patch, clear cache & look for instances where element-invisible was previously used. (menu, slogan, toolbar, overlay, forum, block titles, etc)

2) Seek approval from someone about limited use-case variation from a Drupal standard.

Anything else?

bowersox’s picture

@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.

mgifford’s picture

Ok, I'm going a bit further. If we're going to standardize to the boilerplate, let's standardize to the boilerplate.

EDIT: In #9 above we've @Jacine pulled in the H5BP. If we're going to use their lingo, we might as well use their CSS. Otherwise it's just a bit confusing. We're going to need to do some testing on this with AT, but seems to work with a brief review. Also, I added the invisible class even though it isn't used.

Hopefully this meets the bots requirements.

PS. I also edited the description at the top to fit what was being done in the patch.

mparker17’s picture

Assigned: Unassigned » mparker17

Going to test this.

mparker17’s picture

This does not bode well...

mattparker@yavinIV:htdocs/drupal8 (1363112 ✓) % git apply ~/Desktop/simplify-element-classes-1363112-36.patch 
/Users/mattparker/Desktop/simplify-element-classes-1363112-36.patch:418: trailing whitespace.
.hidden { 
/Users/mattparker/Desktop/simplify-element-classes-1363112-36.patch:438: trailing whitespace.
  position: absolute; 
/Users/mattparker/Desktop/simplify-element-classes-1363112-36.patch:457: trailing whitespace.
  width: auto; 
/Users/mattparker/Desktop/simplify-element-classes-1363112-36.patch:464: trailing whitespace.
  visibility: hidden; 
error: patch failed: core/includes/form.inc:3790
error: core/includes/form.inc: patch does not apply
error: patch failed: core/includes/pager.inc:259
error: core/includes/pager.inc: patch does not apply
error: patch failed: core/includes/theme.inc:1542
error: core/includes/theme.inc: patch does not apply
error: patch failed: core/misc/collapse.js:78
error: core/misc/collapse.js: patch does not apply
error: patch failed: core/modules/block/block.admin.inc:394
error: core/modules/block/block.admin.inc: patch does not apply
error: core/modules/book/book-navigation.tpl.php: No such file or directory
error: core/modules/forum/forum-icon.tpl.php: No such file or directory
error: core/modules/forum/forum-list.tpl.php: No such file or directory
error: patch failed: core/modules/forum/forum.module:1161
error: core/modules/forum/forum.module: patch does not apply
error: patch failed: core/modules/node/lib/Drupal/node/Tests/PageEditTest.php:52
error: core/modules/node/lib/Drupal/node/Tests/PageEditTest.php: patch does not apply
error: patch failed: core/modules/node/node.module:1298
error: core/modules/node/node.module: patch does not apply
error: patch failed: core/modules/overlay/overlay-parent.js:332
error: core/modules/overlay/overlay-parent.js: patch does not apply
error: core/modules/overlay/overlay.tpl.php: No such file or directory
error: core/modules/system/html.tpl.php: No such file or directory
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Form/ElementsLabelsTest.php:65
error: core/modules/system/lib/Drupal/system/Tests/Form/ElementsLabelsTest.php: patch does not apply
error: core/modules/system/page.tpl.php: No such file or directory
error: patch failed: core/modules/system/system.base.css:232
error: core/modules/system/system.base.css: patch does not apply
error: patch failed: core/modules/toolbar/toolbar.module:212
error: core/modules/toolbar/toolbar.module: patch does not apply
error: core/themes/bartik/template.php: No such file or directory
error: patch failed: core/themes/bartik/templates/maintenance-page.tpl.php:25
error: core/themes/bartik/templates/maintenance-page.tpl.php: patch does not apply
error: patch failed: core/themes/bartik/templates/page.tpl.php:101
error: core/themes/bartik/templates/page.tpl.php: patch does not apply
error: core/themes/seven/page.tpl.php: No such file or directory
tlattimore’s picture

@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?

mparker17’s picture

Attached is an updated patch.

Since every hunk in the patch in comment 36 failed to apply, I don't know how to create an interdiff.

mparker17’s picture

Assigned: mparker17 » Unassigned

Yay, testbot likes it!

Someone else, please review.

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

Liam Morland’s picture

Status: Needs review » Needs work

I read it and it looks good, but now it needs a huge re-roll because of the Twig commit.

mparker17’s picture

Assigned: Unassigned » mparker17

Will fix.

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
FileSize
49 KB

Once again, every hunk failed to apply.
Re-rolled against head, avoiding an interdiff.

ry5n’s picture

+1, and #44 is looking pretty good. However:

+++ b/core/modules/contextual/contextual.jsundefined
@@ -411,7 +411,7 @@ Drupal.contextual.collection = new Backbone.Collection([], { model: Drupal.conte
-  return '<button class="trigger element-invisible element-focusable" type="button"></button>';
+  return '<button class="trigger visuallyhidden element-focusable" type="button"></button>';

We need to change .element-focusable too, not just .element-invisible. Jacine’s proposal is to use '.focusable'.

The main thing is to make sure we do all the name changes, not just some. However, I have a few secondary thoughts:
- We should consider the new CSS standards. If we agree this is an element state, it should be '.is-focusable'.
- I know it’s a deviation from html5bp, but Drupal usually dasherizes multi-word class names. What do you think about using '.visually-hidden'?
- If we are using the same (or almost the same) classes as html5bp, we should consider using the same CSS rules. That could be a follow-up.

mparker17’s picture

Assigned: Unassigned » mparker17

I forgot the element-focusable -> focusable change... I'll fix it shortly.

In regards to your secondary thoughts:

  • Overall, I'd like to see this issue get committed before feature freeze (June 30, 2013) because, at the very least, "visually hidden" better describes what the class does than "element-invisible".
  • I don't really mind whether we hyphenate or prefix "is-" to class names, but I value conforming to standards / norms. I don't know the numbers on HTML5 Boilerplate usage, but it has significant market adoption, let's go with it's class names. If consistency with other Drupal classes will solve a lot of problems, let's do that.
mgifford’s picture

Assigned: mparker17 » Unassigned

Looks like the CSS rules for html5bp are still the same:
https://github.com/h5bp/html5-boilerplate/blob/master/css/main.css

Consistency is a good thing, can't see a problem at this point with '.is-focusable'

Also, I'd prefer to stick with Drupal's approach of using "-"'s so '.visually-hidden' would be my preference

I'd rather just stick with the names for now. They also use both hidden & invisible. So we're looking at the following I think:

.hidden
.visually-hidden 
.visually-hidden.is-focusable:active, .visuallyhidden.is-focusable:focus 
.invisible 
mgifford’s picture

Issue summary: View changes

modify according to html5boilerplate.

mparker17’s picture

Issue summary: View changes

Updated issue summary.

mparker17’s picture

Due to the recent twig work, none of the patch hunks applied successfully, so I can't generate an interdiff. But here's the updated patch, with the focusable class fixed.

Also, in some of my previous patches, I had forgotten to add the .invisible class to system.base.css — this is now corrected.

Further, I updated the issue summary to reflect the most-recently-agreed-upon specs as well as the code for the .invisible class.

Marking this as API change and Needs change notification since this does make a change to the "API" used by themers.

ry5n’s picture

I’ve reviewed this and thought about it quite a bit. I think I was wrong to consider the .focusable class a state class; it’s really no more of a state than visually-hidden is. So for the moment I think it’s best to stick to the html5 boilerplate class and deviate as little as possible. I’ve rerolled this with .is-focusable changed back to .focusable, and some improved docblocks.

ry5n’s picture

Issue summary: View changes

Updated issue summary.

ry5n’s picture

Updated the issue summary with .is-focusable back to .focusable as per #49.

tlattimore’s picture

I am with ry5n in #49. I think we should stick with boilerplates standard wherever we can.

mgifford’s picture

Simple re-roll to address

error: patch failed: core/modules/edit/js/edit.js:237

Status: Needs review » Needs work
Issue tags: -JavaScript, -Accessibility, -html5, -API change, -Needs change record

The last submitted patch, drupal-simplify-element-x-1363112-52.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +JavaScript, +Accessibility, +html5, +API change, +Needs change record

The last submitted patch, drupal-simplify-element-x-1363112-52.patch, failed testing.

ahdiaz’s picture

Status: Needs work » Needs review
FileSize
50.79 KB

This re-roll addresses these errors:

error: core/modules/contextual/contextual.base.css: No such file or directory
error: core/modules/contextual/contextual.theme.css: No such file or directory
error: core/modules/contextual/contextual.toolbar.css: No such file or directory
error: core/modules/overlay/overlay-parent.css: No such file or directory
error: core/modules/system/system.base.css: No such file or directory
error: core/modules/tour/css/tour.css: No such file or directory

Status: Needs review » Needs work

The last submitted patch, drupal-simplify-element-x-1363112-56.patch, failed testing.

mparker17’s picture

Status: Needs work » Needs review

No reason why CSS element names should affect whether or not the password reset link is correct. Lets try that again.

mparker17’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
20.37 KB

Adding an interdiff, but after reviewing the code and testing it out locally I think this is RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/edit/js/edit.jsundefined
@@ -279,7 +279,7 @@ function loadMissingEditors (callback) {
-  var $el = $('<div id="' + id + '" class="element-hidden"></div>').appendTo('body');

+++ b/core/modules/node/lib/Drupal/node/NodeRenderController.phpundefined
@@ -52,7 +52,7 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
-          'title' => t('Read more<span class="element-invisible"> about @title</span>', array(

+++ b/core/modules/overlay/overlay-child.jsundefined
@@ -69,11 +69,11 @@ Drupal.behaviors.overlayChild = {
-          .find('a.element-focusable').removeClass('element-invisible');

+++ b/core/modules/system/css/system.module.cssundefined
@@ -269,11 +269,11 @@ input {
-.element-invisible.element-focusable:active,
-.element-invisible.element-focusable:focus {
+.visually-hidden.element-focusable:active,
+.visually-hidden.element-focusable:focus {

Afaics you are also rename element-focusable so this should be .visually-hidden.focusable

+++ b/core/modules/system/css/system.module.cssundefined
@@ -281,6 +281,14 @@ input {
+/*
+ * Hide visually and from screen-readers, but maintain layout.
+ */
+.invisible {
+  visibility: hidden;
+}

Is this actually used? I've read every line and can't see it in use...

Also the patch needs a re-roll...

curl https://drupal.org/files/drupal-simplify-element-x-1363112-56.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 52013  100 52013    0     0  33980      0  0:00:01  0:00:01 --:--:-- 39225
error: patch failed: core/includes/theme.inc:1650
error: core/includes/theme.inc: patch does not apply
error: patch failed: core/modules/overlay/overlay.module:391
error: core/modules/overlay/overlay.module: patch does not apply
mparker17’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
0 bytes

@alexpott

I've re-rolled the patch; it's correct and complete as of 7d7f337 (Fri Jun 14 05:55:36 2013 -0700).

Apparently those uncaught element-hidden and element-invisible were on lines that had been added to core since this patch's last reroll; I've verified they have been changed in this patch.

Note: this patch is really easy to re-create (it's a simple find-replace) but it has to constantly chase HEAD. IMHO, the sooner we can commit this to core, the better — the further we are from the commit where this patch was last re-rolled, the more likely someone else will have introduced a new usage of the old class names.

The element-focusable problem was apparently introduced in #56 and has been corrected.

The .invisible class is not used in Drupal core, but was added because of comments #9 and #36.

mparker17’s picture

Status: Reviewed & tested by the community » Needs review

Whoops, not RTBC; needs review.

mparker17’s picture

Issue summary: View changes

Switched .is-focusable back to .focusable

mparker17’s picture

Issue summary: View changes

core/modules/system/system.base.css doesn't exist anymore.

mparker17’s picture

Let's try that again...

Re-rolled on top of ba810ba (Fri Jun 14 06:03:28 2013 -0700).

penyaskito’s picture

Issue tags: +Avoid commit conflicts
mgifford’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Avoid commit conflicts

Patch applies nicely, just not in SimplyTest.me. Not sure why.

Applied it locally and it worked nicely. Navigated through Bartik & Seven and all looks fine.

.invisible is still included, which I think is fine if we're aiming to build on the HTML5 Boiler Plate.

I also grepped for 'element-focusable' to see if any other instances were in Core. We're good.

mgifford’s picture

Issue tags: +Avoid commit conflicts

I'm adding this tag back in, though frankly not sure why it was removed or how to ensure that this happens.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Well it already needs a reroll... due to #1898428: locale.module - Convert theme_ functions to Twig

curl https://drupal.org/files/drupal-simplify-element-x-1363112-64.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 52656  100 52656    0     0  18616      0  0:00:02  0:00:02 --:--:-- 28821
error: patch failed: core/modules/locale/locale.pages.inc:781
error: core/modules/locale/locale.pages.inc: patch does not apply
mparker17’s picture

*sad trombone*

I'll reroll now.

mparker17’s picture

Status: Needs work » Needs review
FileSize
51.46 KB

Try this...

alexpott’s picture

FileSize
3.38 KB

Here's the diff between 64 and 70 to help reviewers rtbc... all the changes look good to me.

echoz’s picture

Status: Needs review » Reviewed & tested by the community

Seems good afaict.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed da1b134 and pushed to 8.x. Thanks!

mparker17’s picture

I feel that this needs a change notification, since the new class names will come as a surprise to Drupal themers. How do I go about making one?

mparker17’s picture

Assigned: Unassigned » mparker17
Status: Fixed » Active

Found documentation on how to write change notices. I'll try to do that this evening.

alexpott’s picture

Title: Simplify names of "element-x" helper classes » Change notice: Simplify names of "element-x" helper classes
Priority: Normal » Critical

Good point @mparker17 - just updating to right priority and following process

mparker17’s picture

Proposed change notice:

Title
Project
Drupal core [nid:3060]
Introduced in branch
8.x
Introduced in version
8.0-alpha2
Issues
#1363112
Description

Summary

  • The element-hidden, element-invisible and element-focusable classes were changed to better describe what they do.
  • The new names were chosen for consistency with HTML5 Boilerplate, a popular and well-known starting point for HTML 5 themes.
  • An invisible class was added to hide elements visually and from screen-readers, but maintain the (visual) layout.

Before

<div class="element-hidden">...</div>
<div class="element-invisible">...</div>
<div class="element-invisible element-focusable">...</div>

After

<div class="hidden">...</div>
<div class="visually-hidden">...</div>
<div class="visually-hidden focusable">...</div>
<div class="invisible">...</div>
Impacts
Themers
Updates done
Theming guide (Details: TBD)
mparker17’s picture

Status: Active » Needs review

Comments and criticism welcome!

tim.plunkett’s picture

The "Before" has 3 examples, the "After" has 4. That doesn't help explain what the difference is.
What's the D7 equivalent of the 4th example?

mparker17’s picture

Good point.

Try this:

Before

<div class="element-hidden">...</div>
<div class="element-invisible">...</div>
<div class="element-invisible element-focusable">...</div>
<div style="visibility: hidden;">...</div>

After

<div class="hidden">...</div>
<div class="visually-hidden">...</div>
<div class="visually-hidden focusable">...</div>
<div class="invisible">...</div>
effulgentsia’s picture

Issue tags: -Avoid commit conflicts

This was committed, so no longer needs the "Avoid commit conflicts" tag.

chrisjlee’s picture

IMHO, Change notice is clear and more effective after #80.

mparker17’s picture

RTBC?

tim.plunkett’s picture

You should actually create the change notice now. The text is fine, once it's in place this can be closed.

mparker17’s picture

Created change notice: https://drupal.org/node/2022859

tim.plunkett’s picture

Title: Change notice: Simplify names of "element-x" helper classes » Simplify names of "element-x" helper classes
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Thanks, looks great!

hass’s picture

Status: Fixed » Needs work

And now rollback this patch, please. If there are more of these type of patches around, also rollback them. element- was a good prefix.

We already leaned that classes should never written this simple way. See #1938044: Prefix all toolbar classes to prevent theme clashes for the reasons that causes a lot of theme clashes.

hass’s picture

echoz’s picture

I think .hidden or .invisible are a different case than for example, .box, .icon or .menu. The former are established classes (html5boilerplate) that consistantly have the same property/value, rather than used for different theme specific styling as would be put on the latter classes in this example.

hass’s picture

What boilerplate does - doesn't mean that this is correct for a very large CMS like Drupal where we have rules to prefix classes with it's module name - just as one example. If a theme use this class name, core need to change. We've done this for ages for good reasons.

Theme classes always win and this asks for collisions only - nothing else.

ry5n’s picture

@hass Your point about name collisions is well taken. Namespacing and avoiding generic class names is a great idea (see the CSS coding standards). For this particular set of classes, I think there are a couple of things that argue against namespacing them:

1) they are intended to be global;
2) their names and functions are de facto standards thanks to html5 boilerplate; in my experience they are useful for sites of every shape and size and don’t vary from site to site;
3) they used everywhere in Drupal’s markup (which argues for keeping them short).

Given that, can you come up with a plausible scenario in which these classes cause problems for a theme developer?

BTW, I think wider discussion in #2029187: [meta] Make sure CSS classes are prefixed properly is very much worth having. I think we should do that first though before rethinking this issue though.

echoz’s picture

Status: Needs work » Fixed
hass’s picture

Status: Fixed » Needs work

We need to fix the naming first. Boilerplate is not a defacto standard. I never used it nor do I have an idea what boilerplate is. This means boilerplate could be something that a few people know and like, but in the rest of the world it's unknown and not used at all.

tim.plunkett’s picture

I hate to do it, but I agree with hass here, if only because "Boilerplate is not a defacto standard" is very true. I don't know who proposed that it was, but that's just silly to say.

I don't see any semantic gain by this rename, and only potential problems.

mgifford’s picture

@tim.plunkett - there is no accessibility improvement gained by changing the namespace.

I don't really care, except that the Boilerplate names are easier to remember.

I don't know if it's a matter of Boilerplate being the standard. However, it's a big, well known framework that many themers will know about that has a similar structure. I'm sure there are other HTML5 standards out there, but I'd be surprised to find that they include these accessibility elements.

xjm’s picture

Title: Simplify names of "element-x" helper classes » [Rollback under discussion] Simplify names of "element-x" helper classes

Adding a title prefix. Please update the summary to indicate the patch has already been committed and to summarize the pros and cons of the rename.

Also, if the community comes to a consensus that this should be rolled back, #2046301: CSS regression on admin/modules: overlapping "Enabled" table header not properly invisible will also need to be reverted (if it goes in before then).

It looks like #1921610: [Meta] Architect our CSS is the bucket this falls under. We do not have a component maintainer for CSS listed in MAINTAINERS.txt.

mparker17’s picture

@hass, @tim.plunkett, @xjm: my sincere apologies for any inconvenience caused. I'm a new contributor (and themer) and my goal in helping out with this issue was to work through accessibility problems from oldest to newest.

I believe that the new class names are better at explaining how these classes affect screenreaders and provide better insight into when someone might want to use one over another.

However, I was not involved in the discussion around why HTML5 boilerplate was chosen and/or why the element prefixes were removed: I assumed that the people involved in choosing them did their research and had good reasons to do so; I just rerolled the patch and took care of any follow-up issues.

***

I will be remotely participating in the Twin Cities DrupalCamp A11y Sprint tomorrow. I'd definitely be interested in working with anyone / everyone interested or concerned with this issue and the changes it makes to ensure it gets resolved expediently.

I'd like to do what I can to ensure that these classes have clear names which explain how they affect screenreaders while also making sure they do not clash or collide with anything or cause anyone unnecessary problems.

***

Going forward, I think the best compromise in this situation would be to restore the element- prefix, while keeping the new "visually-hidden", "focusable" and "invisible" descriptors. This would allow the classes to remain understandable and prevent them from clashing with other things and also allow #2029187: [meta] Make sure CSS classes are prefixed properly to move forward independently.

Does anyone have any thoughts about this proposed solution, or any better solutions?

bowersox’s picture

@mparker17, no need to apologize. It appeared that there was consensus around the change, and it was committed into D8. Now it appears there is discontent. Not your fault.

My personal opinion is that the choice of name does not matter. What really matters more than the name is that we document and teach the community how to use these classes properly for accessibility. I don't think it's worth pouring a lot of effort into un-changing/re-changing these class names.

Having both sets of class names seems like a recipe for clutter and confusion.

To get a final decision, it seems like we need input from Dries or a decision-maker. Someone who is passionate about this could take the time to write up an objective summary of the pros and cons and the options for moving ahead.

xjm’s picture

mparker17’s picture

@hass, thanks for the link to #2029187: [meta] Make sure CSS classes are prefixed properly... I'll definitely keep an eye on that issue.

What does everyone think of classes like:

Before Currently Proposed
element-hidden hidden element-hidden
element-invisible visually-hidden element-visually-hidden
element-focusable visually-hidden.focusable element-visually-hidden.element-focusable
invisible element-invisible
mparker17’s picture

Issue summary: View changes

Using /** now

mparker17’s picture

Assigned: mparker17 » Unassigned
Issue tags: -JavaScript, -Needs issue summary update

Issue summary updated.

Also, removing JavaScript tag because AFAIK it has nothing to do with JavaScript.

mgifford’s picture

This resolution would make it more understandable for people as it is closer to a broader naming convention.

It's not the simplification, but should address the issue of namespace problems.

mgifford’s picture

Issue summary: View changes

Updated the issue summary as requested by @xjm

hass’s picture

I'm fine with this prefixing. Something I'm really missing is a description - how and for what/where this classes should be used. These details are missing in the change notice, too. I have mainly difficulties to understand element-visually-hidden.element-focusable and element-invisible. I do not think this is self speaking.

echoz’s picture

Note some of the suggestions in #100 are opposite of the original intention to make the class names less verbose.

xjm’s picture

Thanks @mparker17.

Liam Morland’s picture

In general, I think having a prefix makes sense to avoid collisions. Why "element"? Elements are the only thing that a class can be applied to. Why not "drupal"?

mparker17’s picture

I'm cool with "drupal".

I also considered "system" to denote that it was coming from the system module.

I mostly proposed "element" to get the discussion started; although it is more-familiar to people and only one letter difference.

tstoeckler’s picture

As far as I'm aware, a "drupal" prefix is against our new coding standards. I thought we prefix our classes with component names, but "drupal" does not make sense in that case. "drupal" is not a component.

DamienMcKenna’s picture

Status: Needs work » Postponed

This is intending to roll back a change that was agreed to by many with little solid reason other than "it might conflict".

Given this falls under the purview of Drupal's CSS architecture, how about stopping the onesie-twosie architectural changes until a solid plan (#1921610: [Meta] Architect our CSS) has been decided upon?

joshk’s picture

Dear Drupal developers,

For the love of all that is holy, keep the classes short and simple. Collisions don't cause a WSOD like in PHP, and in fact are part of what the "Cascading" part of CSS is designed to handle. You can work it all out in the theme if there are issues.

On the other hand, adding prefixes to everything creates all kinds of barriers: it makes it difficult to re-use existing front-end code, and more laborious to integrate existing designs. It isolates Drupal themes on Drupal island. This isn't where we want to be.

Nobody who does front end all day long shares our love of verbosity in naming, or baroque 12th-degree nesting of DOM objects. We should use simple semantic names wherever possible in the front-end. The clear preference in that discipline has been to err on the side of simplicity and ease of use (c.f. every javascript tool using "$" for it's shorthand, HTML boilerplate which is a thing a lot of people use, even if you haven't heard of it), and resolve naming conflicts between various libraries using the many existing mechanisms available as part of the CSS/JS/DOM model.

TL;DR what Jacine said.

DamienMcKenna’s picture

Status: Postponed » Fixed

I think this deserves to go back to fixed, and lets leave it at that until such time that both a) the CSS architecture group decide there needs to be a specific change, b) it has sign-off from the designers & themers of the community, kinda like the original feature request had.

tim.plunkett’s picture

Title: [Rollback under discussion] Simplify names of "element-x" helper classes » Simplify names of "element-x" helper classes
hass’s picture

Status: Fixed » Needs work

Hey, don't close this, please.

Backbone is not a standard. Read the case about the toolbar again and you may get an idea. We need to prefix and i was not aware of this bad change and case here before it happened. Only for the reason that a very few think backbone is the standard, don't make it a standard. Waiting for the css meta case is not required and aside they confirmed the troubles already. I'm a themer.

Committing this patch was a clear fault.

tstoeckler’s picture

@hass: Can you provide links to "the case about the toolbar" and the confirmed "troubles"? I for one am not aware of those.

mparker17’s picture

Hello everyone,

The Drupal Code of Conduct suggests that we should Be Collaborative and Consult Others in the approach we take to resolving this issue. Let's keep an open mind and focus on how we can improve Drupal for everyone's benefit in this situation.

@hass, @Owen Barton, you have made a good case for name prefixing. Would you prefer a complete rollback or would adding prefixes to the new names suffice? If adding prefixes to the new names is acceptable, do you have a preferred / suggested prefix?

@joshk, @Jacine, @catch, @Everett Zufelt, @dcmouyard, @mcjim, @tlattimore, @mgifford, all of you have expressed support for using HTML5 Boilerplate as a standard. Could I trouble one of you take a few minutes to provide some numbers on the adoption of HTML5 Boilerplate inside and outside the Drupal community?

Thanks

mparker17’s picture

@tstoeckler: @hass presented the toolbar difficulties here: #1938044: Prefix all toolbar classes to prevent theme clashes

hass’s picture

Assigned: JohnAlbin » Unassigned
Status: Fixed » Needs work

#2029187: [meta] Make sure CSS classes are prefixed properly
#1938044: Prefix all toolbar classes to prevent theme clashes
#1757466: Prefix all navbar classes to prevent theme clashes

Would you prefer a complete rollback or would adding prefixes to the new names suffice?

Prefixing the current names is just fine for me. Names as suggested in #100 are good.

JohnAlbin’s picture

Assigned: Unassigned » JohnAlbin
Status: Needs work » Fixed

Apples and oranges. The issue hass points out is one where the Toolbar module was using the .box class. That's way too generic. I approve of the change to make that .box class more specific.

However, .visually-hidden and .focusable are goldilocks-level of just-right specificity. Not ridiculously specific like the .element-* and .drupal-* prefixes would make them, nor too generic.

I see no evidence that the names that are used in core right now would ever cause a conflict. If you create a theme and use .invisible styling for anything other than making the element visibility: hidden;, I seriously question your class naming skills.

Thank you to everyone who worked on the original patch! I worked on the Drupal 7 patch when we created it and named it element-invisible, etc. and I like these new names much better. The HTML5 Boilerplate team did a better job of naming it then we did. I'm glad they were able to iterate on our Drupal 7 work. And now we get to use their (naming convention) work.

hass’s picture

Assigned: Unassigned » JohnAlbin

It's possible that I really have a lot more attributes defined in a generic class named invisible. Maybe I clear floats inside, set a height:0... and much more... just some ideas what could happen. The names are not good and prove to troubles. Again, HTML5 Boilerplate is not an accepted standard.

mparker17’s picture

@hass, thanks for clarifying what you want changed.

@JohnAlbin, thanks for your input!

***

I'd personally like to hear some more information about how many people use HTML5 Boilerplate inside and outside the Drupal community before I'd be comfortable with changing the issue status.

tim.plunkett’s picture

This has nothing to do with H5BP. Just common sense. Someone should close and lock this thread.

hass’s picture

@tim.plunkett: Is this the way how you understand collaboration? Interesting. I'm highly confused about this and your #94 comment.

tim.plunkett’s picture

Hass, look at the names listed in #115. We have consensus.
While my teo comments may seem at odds, they're actually the same: Drupal should not follow H5BP just because some people like it. Boilerplate as an argument, for or against, is flawed.

JohnAlbin's point is clear and well put.

Liam Morland’s picture

I think even if HTML5 Boilerplate is not widely used we are still better off following an existing standard than coming up with yet another standard. If it works for us, we should use it; if it does not work, we should not use it.

hass’s picture

@tim: #115 is not a consensus. it's just a listing of some people and their point of view and some may think there is no collision and some other have them or at least see them coming up. There have been a lot of bad class name changes in D8 and not many people are testing it with contrib modules and themes.

The adoption of boilerplate is near zero in Drupal community. https://drupal.org/project/boilerplate, 567 sites. Compare to Zen 107480 sites. By these numbers we can safely say boilerplate is not a standard for sure and their decisions on naming could be wrong for sure. I don't know their adoption on the internet, but I guess it's in other numbers the same. no countable numbers.

mgifford’s picture

Ok, so what are the alternatives to http://html5boilerplate.com :
http://www.initializr.com/
http://www.getskeleton.com/
http://twitter.github.io/bootstrap/
http://www.yaml.de/
http://firezenk.github.io/zimit/index.html
http://foundation.zurb.com/

And Bootstrap is catching up according to Google Trends http://goo.gl/94gfa7

But mostly it's a matter of having something that is easier to understand that what is in D7 now. Having something that is easily understandable is the goal of this transition and I think D8's naming structure for this will be easier to remember than it was for D7.

hass’s picture

I asked to explain the meaning of the current naming. Can you explain, please? I do not understand the current names and when I should use one or the other. No joke.

hass’s picture

#1955912: Conflict with FontAwesome is one more conflict example.

mgifford’s picture

A lot of the comparison boils down to the comment in #100.

The difference between element-hidden & element-invisible isn't clear. It was trying to rely on jQuery conventions as I recall. What's the difference between hidden & invisible in D7? I helped author it, but usually end up looking it up to ensure I got it right.

While hidden & visually-hidden is a bit more clear.

If you want to hide the page from everyone you want to use hidden. If you want to hide the element just visually (especially if there is semantic meaning provided for screen readers) then specify visually-hidden.

mgifford’s picture

Also useful to look at other standards for Hidden Content YAML's HTML5 framework includes:

.ym-skip The use of the unordered list .ym-skiplinks (see above) is optional, but the preferred solution. Alternatively, you can integrate the skip links directly into the layout. To ensure the functionality, the CSS class .ym-skip is required for skip anchors.
.ym-hideme Elements with the CSS class .ym-hideme are visually hidden in browsers, but still accessible for screen readers.
.ym-print Elements with the CSS class .ym-print are visually hidden in media screen, but still accessible for screen readers and visible on media print.
.ym-noprint Elements with the CSS class .ym-noprint will not be printed, but are visible on any other media type (e.g. screen).

Interesting for comparison, but when looking at other frameworks for what others are doing, it's good to find someone who is doing something close to what Drupal is doing.

EDIT: I think I've been clear, but to echo @AmyStephen below "+1 JohnAlbin #118"

AmyStephen’s picture

+1 JohnAlbin #118

JohnAlbin’s picture

Status: Needs work » Fixed

Hass requested better explanations of the new classes in #127, so I've updated the change record: https://drupal.org/node/2022859

hass’s picture

Status: Fixed » Needs work

Class names are very bad and may cause collisions. This is not self speeking, too. Rollback still required.

dcmouyard’s picture

Status: Needs work » Fixed

JohnAlbin did an excellent job explaining the new class names and what they're meant for in the change record.

@hass: please be more specific about why these class names are bad and cause collisions. Class name collision with 3rd party code that is often combined with Drupal is a legitimate concern. Class name collision with your personal naming conventions is not.

hass’s picture

Status: Fixed » Needs work

I already provided a link or more for the type of issues above. It's a rule for ages to prefix all classes with it's module name, too.

markhalliwell’s picture

Status: Needs work » Fixed

@hass, please... just stop. I completely agree with #110. The "consensus" in #115 is not just developers, but actual themers, who have all agreed (myself included) that this is fine. There is no "work" that needs to be done here.

markhalliwell’s picture

Put in a request to #2089955: Lock commenting per #121.

chx’s picture

Status: Fixed » Closed (fixed)

And I will lock it as per webmaster request.

chx’s picture

Issue summary: View changes

Added reference to #2045151

mgifford’s picture

Issue summary: View changes