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,
- Rename classes to better describe how they affect screenreaders
- Reduce the amount of typing if possible.
- Have the classes follow the HTML5 Boilerplate standard.
After #70 was committed,
- 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:
- Decide what prefixes to add.
- Update any code that uses these classes.
- Update #2046301: CSS regression on admin/modules: overlapping "Enabled" table header not properly invisible depending on whether it's been committed at that time.
- 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:
- Rollback or fix any issues that use these classes.
- 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.
- 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.
Related Issues
- Open issues that we know will be affected by this change:
- "Cautionary tale" issue providing motivation for rollback: #1938044: Prefix all toolbar classes to prevent theme clashes
- Meta issues to discuss and head off similar issues: #1921610: [Meta] Architect our CSS, #2029187: [meta] Make sure CSS classes are prefixed properly
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;
}
Comment | File | Size | Author |
---|---|---|---|
#71 | 64-70-diff.txt | 3.38 KB | alexpott |
#70 | drupal-simplify-element-x-1363112-70.patch | 51.46 KB | mparker17 |
#64 | drupal-simplify-element-x-1363112-64.patch | 51.42 KB | mparker17 |
#62 | drupal-simplify-element-x-1363112-62.patch | 0 bytes | mparker17 |
#60 | interdiff-49-56.txt | 20.37 KB | mgifford |
Comments
Comment #1
JacineAnd here's a patch that does that.
Comment #2
JacineTagging. The patch only changes the names of the classes, but the accessibility team should be aware.
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commentedIn 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!
Comment #4
bowersox CreditAttribution: bowersox commentedWhat about allowing both the new short names and the older, longer names?
The patch could be more like this:
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.
Comment #5
JacineSure, 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.
Comment #6
bowersox CreditAttribution: bowersox commented@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.
Comment #7
JacineThanks for the feedback @bowersox! We would definitely document it. It would need a change notification after being committed. :)
Comment #8
mgiffordReally 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:
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
Comment #9
JacineThis 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.
Comment #10
Owen Barton CreditAttribution: Owen Barton commentedI 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).
Comment #11
JacineCan 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.
Comment #12
dcmouyard CreditAttribution: dcmouyard commentedI 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.
Comment #13
JacineMarking this needs work to incorporate the H5BP names into the patch.
Comment #14
mcjim CreditAttribution: mcjim commented+1 for same class names as the HTML5 Boilerplate. Makes perfect sense.
Cleaner, clearer and more elegant.
Comment #15
mgifford#1: 1363112-01-simplify-element-x-classes.patch queued for re-testing.
Comment #17
tlattimore CreditAttribution: tlattimore commentedI 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.
Comment #19
tlattimore CreditAttribution: tlattimore commentedI have resolved the issues that made the patch in #17 fail and re-rolled.
Comment #20
dcmouyard CreditAttribution: dcmouyard commentedThe 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.
Comment #21
LSU_JBob CreditAttribution: LSU_JBob commentedDownloaded and applied the patch, no failures. Since we're just altering class names I'm gonna go ahead and make this RTBC.
Comment #22
catchThis is not using the H5BP names as suggested in #13, and I think that was a good idea, so moving back to needs work.
Comment #23
dcmouyard CreditAttribution: dcmouyard commentedThis patch changes the invisible class to visually-hidden. It also fixes documentation issues and an incorrect jQuery selector in overlay-child.js.
Comment #24
bowersox CreditAttribution: bowersox commented@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!
Comment #25
nod_tag
Comment #26
bowersox CreditAttribution: bowersox commentedRe-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.
Comment #27
tlattimore CreditAttribution: tlattimore commentedPatch 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.
Comment #28
dcmouyard CreditAttribution: dcmouyard commentedWhat'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.
Comment #29
tlattimore CreditAttribution: tlattimore commented#28
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
Comment #30
Everett Zufelt CreditAttribution: Everett Zufelt commentedRegardless 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.
Comment #31
mgifford#26: simplify-element-classes-1363112-26.patch queued for re-testing.
Comment #34
mgiffordWhat 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?
Comment #35
bowersox CreditAttribution: bowersox commented@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.
Comment #36
mgiffordOk, 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.
Comment #37
mparker17Going to test this.
Comment #38
mparker17This does not bode well...
Comment #39
tlattimore CreditAttribution: tlattimore commented@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?
Comment #40
mparker17Attached 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.
Comment #41
mparker17Yay, testbot likes it!
Someone else, please review.
We'll probably have to notify the theming team about this change.
Comment #42
Liam MorlandI read it and it looks good, but now it needs a huge re-roll because of the Twig commit.
Comment #43
mparker17Will fix.
Comment #44
mparker17Once again, every hunk failed to apply.
Re-rolled against head, avoiding an interdiff.
Comment #45
ry5n CreditAttribution: ry5n commented+1, and #44 is looking pretty good. However:
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.
Comment #46
mparker17I forgot the element-focusable -> focusable change... I'll fix it shortly.
In regards to your secondary thoughts:
Comment #47
mgiffordLooks 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:
Comment #47.0
mgiffordmodify according to html5boilerplate.
Comment #47.1
mparker17Updated issue summary.
Comment #48
mparker17Due 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 tosystem.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.
Comment #49
ry5n CreditAttribution: ry5n commentedI’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.
Comment #49.0
ry5n CreditAttribution: ry5n commentedUpdated issue summary.
Comment #50
ry5n CreditAttribution: ry5n commentedUpdated the issue summary with .is-focusable back to .focusable as per #49.
Comment #51
tlattimore CreditAttribution: tlattimore commentedI am with ry5n in #49. I think we should stick with boilerplates standard wherever we can.
Comment #52
mgiffordSimple re-roll to address
Comment #54
mgifford#52: drupal-simplify-element-x-1363112-52.patch queued for re-testing.
Comment #56
ahdiaz CreditAttribution: ahdiaz commentedThis re-roll addresses these errors:
Comment #58
mparker17No reason why CSS element names should affect whether or not the password reset link is correct. Lets try that again.
Comment #59
mparker17#56: drupal-simplify-element-x-1363112-56.patch queued for re-testing.
Comment #60
mgiffordAdding an interdiff, but after reviewing the code and testing it out locally I think this is RTBC.
Comment #61
alexpottAfaics you are also rename element-focusable so this should be
.visually-hidden.focusable
Is this actually used? I've read every line and can't see it in use...
Also the patch needs a re-roll...
Comment #62
mparker17@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
andelement-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.Comment #63
mparker17Whoops, not RTBC; needs review.
Comment #63.0
mparker17Switched .is-focusable back to .focusable
Comment #63.1
mparker17core/modules/system/system.base.css doesn't exist anymore.
Comment #64
mparker17Let's try that again...
Re-rolled on top of
ba810ba
(Fri Jun 14 06:03:28 2013 -0700).Comment #65
penyaskitoComment #66
mgiffordPatch 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.
Comment #67
mgiffordI'm adding this tag back in, though frankly not sure why it was removed or how to ensure that this happens.
Comment #68
alexpottWell it already needs a reroll... due to #1898428: locale.module - Convert theme_ functions to Twig
Comment #69
mparker17*sad trombone*
I'll reroll now.
Comment #70
mparker17Try this...
Comment #71
alexpottHere's the diff between 64 and 70 to help reviewers rtbc... all the changes look good to me.
Comment #72
echoz CreditAttribution: echoz commentedSeems good afaict.
Comment #73
alexpottCommitted da1b134 and pushed to 8.x. Thanks!
Comment #74
mparker17I 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?
Comment #75
mparker17Found documentation on how to write change notices. I'll try to do that this evening.
Comment #76
alexpottGood point @mparker17 - just updating to right priority and following process
Comment #77
mparker17Proposed change notice:
Summary
element-hidden
,element-invisible
andelement-focusable
classes were changed to better describe what they do.invisible
class was added to hide elements visually and from screen-readers, but maintain the (visual) layout.Before
After
Comment #78
mparker17Comments and criticism welcome!
Comment #79
tim.plunkettThe "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?
Comment #80
mparker17Good point.
Try this:
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedThis was committed, so no longer needs the "Avoid commit conflicts" tag.
Comment #82
chrisjlee CreditAttribution: chrisjlee commentedIMHO, Change notice is clear and more effective after #80.
Comment #83
mparker17RTBC?
Comment #84
tim.plunkettYou should actually create the change notice now. The text is fine, once it's in place this can be closed.
Comment #85
mparker17Created change notice: https://drupal.org/node/2022859
Comment #86
tim.plunkettThanks, looks great!
Comment #87
hass CreditAttribution: hass commentedAnd 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.
Comment #88
hass CreditAttribution: hass commented#2029187: [meta] Make sure CSS classes are prefixed properly
Comment #89
echoz CreditAttribution: echoz commentedI 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.
Comment #90
hass CreditAttribution: hass commentedWhat 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.
Comment #91
ry5n CreditAttribution: ry5n commented@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.
Comment #92
echoz CreditAttribution: echoz commentedSetting this back to fixed since #2029187-2: [meta] Make sure CSS classes are prefixed properly
Comment #93
hass CreditAttribution: hass commentedWe 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.
Comment #94
tim.plunkettI 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.
Comment #95
mgifford@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.
Comment #96
xjmAdding 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.
Comment #97
mparker17@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?
Comment #98
bowersox CreditAttribution: bowersox commented@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.
Comment #99
xjmComment #100
mparker17@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:
Comment #100.0
mparker17Using /** now
Comment #101
mparker17Issue summary updated.
Also, removing JavaScript tag because AFAIK it has nothing to do with JavaScript.
Comment #102
mgiffordThis 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.
Comment #102.0
mgiffordUpdated the issue summary as requested by @xjm
Comment #103
hass CreditAttribution: hass commentedI'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
andelement-invisible
. I do not think this is self speaking.Comment #104
echoz CreditAttribution: echoz commentedNote some of the suggestions in #100 are opposite of the original intention to make the class names less verbose.
Comment #105
xjmThanks @mparker17.
Comment #106
Liam MorlandIn 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"?
Comment #107
mparker17I'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.
Comment #108
tstoecklerAs 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.
Comment #109
DamienMcKennaThis 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?
Comment #110
joshk CreditAttribution: joshk commentedDear 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.
Comment #111
DamienMcKennaI 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.
Comment #112
tim.plunkettComment #113
hass CreditAttribution: hass commentedHey, 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.
Comment #114
tstoeckler@hass: Can you provide links to "the case about the toolbar" and the confirmed "troubles"? I for one am not aware of those.
Comment #115
mparker17Hello 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
Comment #116
mparker17@tstoeckler: @hass presented the toolbar difficulties here: #1938044: Prefix all toolbar classes to prevent theme clashes
Comment #117
hass CreditAttribution: hass commented#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
Prefixing the current names is just fine for me. Names as suggested in #100 are good.
Comment #118
JohnAlbinApples 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.
Comment #119
hass CreditAttribution: hass commentedIt'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.Comment #120
mparker17@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.
Comment #121
tim.plunkettThis has nothing to do with H5BP. Just common sense. Someone should close and lock this thread.
Comment #122
hass CreditAttribution: hass commented@tim.plunkett: Is this the way how you understand collaboration? Interesting. I'm highly confused about this and your #94 comment.
Comment #123
tim.plunkettHass, 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.
Comment #124
Liam MorlandI 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.
Comment #125
hass CreditAttribution: hass commented@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.
Comment #126
mgiffordOk, 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.
Comment #127
hass CreditAttribution: hass commentedI 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.
Comment #128
hass CreditAttribution: hass commented#1955912: Conflict with FontAwesome is one more conflict example.
Comment #129
mgiffordA 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.
Comment #130
mgiffordAlso useful to look at other standards for Hidden Content YAML's HTML5 framework includes:
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"
Comment #131
AmyStephen CreditAttribution: AmyStephen commented+1 JohnAlbin #118
Comment #132
JohnAlbinHass requested better explanations of the new classes in #127, so I've updated the change record: https://drupal.org/node/2022859
Comment #133
hass CreditAttribution: hass commentedClass names are very bad and may cause collisions. This is not self speeking, too. Rollback still required.
Comment #134
dcmouyard CreditAttribution: dcmouyard commentedJohnAlbin 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.
Comment #135
hass CreditAttribution: hass commentedI 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.
Comment #136
markhalliwell@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.
Comment #137
markhalliwellPut in a request to #2089955: Lock commenting per #121.
Comment #138
chx CreditAttribution: chx commentedAnd I will lock it as per webmaster request.
Comment #138.0
chx CreditAttribution: chx commentedAdded reference to #2045151
Comment #139
mgifford