Posted by derjochenmeyer on April 9, 2010 at 9:22am
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
| Issue tags: | needs accessibility review |
Issue Summary
Invisible elements can "pop up" if height is used uncarefully during theming. For unexperienced users this can become a bug that is not easy to track.
Adding !important to CSS attributes of .element-invisible in system-behavior.css better ensures that invisible elements stay invisible.
.element-invisible {
height:0 !important;
overflow:hidden !important;
position:absolute !important;
}
Comments
#1
Here is a patch.
#2
The last submitted patch, system-behavior.patch, failed testing.
#3
Here we go again. UTF-8 without BOM.
#4
Bot: Test it!
#5
We decided to leave !importants out of core, you can add them in your theme.
#6
Seems it needs UTF-8?
#7
Oh, I didnt know about that decision. However this was one of the first "bugs" i found when i started converting a theme. Do you have a link to the "!important" discussion?
#8
The last submitted patch, system-behavior.patch, failed testing.
#9
It needs unix style endings your patch, if you are working on windows, download notepad++ and choose unix style.
Crosslinking: #730754: CSS coding standards: No browser hacks, no !important in core/modules, ONLY IN THEMES
#10
Thanks for the Crosslink and the notepad++ tipp.
I think that .element-invisible should be better protected against showing up. The strategy to hide those elements with
height,overflowandpositionis accessible and good practice, butheightandpositionare attributes that are usually not used to hide elements and are very prone to be overwritten by accident.Since we don't want to hide .element-invisible with
display: noneortext-indent: -9999pxwe IMO need !important to make this method safer.Anyone who explicitly wants to show these elements can do it by simply adding something like
body .element-invisible {height:auto !important;
overflow:visible !important;
position:static !important;
}
#11
I'll ask sun to jump in, he started the discusion ;)
#12
The last submitted patch, system-behavior.patch, failed testing.
#13
My fault, next try.
#14
Hmmm… I'd almost consider this an exception to the "no !important in core" rule.
I actually think the "position" property is more likely to be accidentally overriden then the height property. But if you override any of the three properties, you're going to get some visible artifacts showing.
We're actually seeing this issue in Bartik if its used during installation. See screenshot in comment #8 of #790556: Make the Maintenance Page Kick Ass
I'm RTBCing this, but I'm going to ping Sun and Jacine to get their input.
#15
Can't find them in IRC. Pinged them in twitter. Setting back to "Needs review" until I hear from them.
#16
Why only for .element-invisible? What about the other generic classes? i.e. at least .element-hidden (but possibly also .js-hide, .js-show)?
I'm equally against !important, but this usage here seems to make sense.
#17
I agree that .element-invisible qualifies as an exception here. I don't think .js-hide, .js-show or .element-hidden fall into this category, because they are only dealing with showing/hiding which isn't likely to break anything.
I also think that since it is such a special case, it would also be worth adding more aggressive "reset" properties because it is far too fragile right now. All you have to do is add a background, border, padding or margins to
.block h2, all of which is very common, and you break it. Normally, I would be very against something like this in core, but in this case, I think it makes perfect sense.#18
Implemented suggestions of #17
/*** Hide elements visually, but keep them available for screen-readers.
*
* Used for information required for screen-reader users to understand and use
* the site where visual display is undesirable. Information provided in this
* manner should be kept concise, to avoid unnecessary burden on the user. Must
* not be used for focusable elements (such as links and form elements) as this
* causes issues for keyboard only or voice recognition users. To prevent that
* these properties get unintentionally overriden by themers they are enforced
* by "!important".
*/
.element-invisible {
height: 0 !important;
overflow: hidden !important;
position: absolute !important;
margin: 0 !important;
padding: 0 !important;
border: none !important;
background: none repeat scroll 0 0 transparent !important;
}
#19
I agree, we need "border: none" and also "padding: 0", but we dont need background and margin.
If "position: absolute" is set margin will not harm an invisible element. And background can be whatever if there is no padding and no height.
#20
I'm slightly confused. The statement in #19 applies to all, margin, padding, border, background, etc., since the element is no longer part of the rendered/aligned DOM tree.
#21
Right now the element is hidden by
.element-invisible {height: 0;
overflow: hidden;
position: absolute;
}
If you add a border to it, the border will be visible.
Also, if a background color or image is set, a padding will make this background visible.
#22
Why aren't we using display: none; ?
#23
see: #473396: Defining System-Wide Approaches to Remove, Make Invisible & Push Content Off-screen with CSS
#24
This means we should add
left: -999em;, so as to actually position the element invisible?#25
It has been considered in this post but was not implemented: http://drupal.org/node/473396#comment-1894592
EDITED
#26
Here is a good article which discusses the posibilities: http://www.webaim.org/techniques/css/invisiblecontent/
Our goal is to make these elements invisible for regular browsers, but NOT for screenreaders. So here are the possibilities mentioned in the article:
visibility: hidden; and/or display:none;No good, will make it invisible for screenreaderswidth:0px;height:0pxNo good! will also make it invisible for screenreaders, so we should rethink the current approach!text-indent: -1000px;No good, would work but could created dotted lines for focused links in some browsersThe article suggests the following:
.element-invisible {position:absolute;
left:-10000px;
top:auto;
width:1px;
height:1px;
overflow:hidden;
}
Lets think about what a themer might do with a typically hidden element like
.block h2to unintentionally reveal it: relative or absolute positioning.The only reason the article above suggests adding "width" and "height" of 1px in addition to "left: -10000px" is that the positioning might be overwritten. In that case it makes sense for us to also add "padding: 0" and "border: 0" as suggested above.
Enforcing all by "!important" would make it almost failsafe.
#27
The left solution gives possible promblems in RTL, consider that while choosing for a solution. The current toolbar skip link uses left:-999px to hide the text but it causes a huge horizontal scrollbar in RTL (in IE6 and IE7). Will this approach fix that?
#28
Taking all this into consideration here is a suggestion which seems best practice:
.element-invisible {position:absolute !important;
width:1px !important;
height:1px !important;
overflow:hidden !important;
padding: 0 !important;
border: none !important;
background-color: transparent !important;
background-image: none !important;
}
Why:
To enforce all this and prevent any unwanted overwrites we add "!important" to all of those.
#29
Counter-recommendation: Just use left: -999em; along with corresponding RTL styles (left: auto; right: -999em;).
#30
Here is a patch that implements suggestions form #28 above.
#31
Patch as suggested in #28 (removed blank line from #30)
#32
Patch as suggested in #29
#33
+++ modules/system/system-behavior.css Wed May 19 18:28:17 2010@@ -296,10 +296,11 @@
- * causes issues for keyboard only or voice recognition users.
+ * causes issues for keyboard only or voice recognition users. To prevent that
+ * these properties get unintentionally overriden by themers they are enforced
+ * by "!important".
"!important" is used to prevent unintentional overrides.
+++ modules/system/system-behavior.css Wed May 19 18:28:17 2010@@ -296,10 +296,11 @@
- position: absolute;
+ position:absolute !important;
Missing space between colon and value.
+++ modules/system/system-behavior-rtl.css Wed May 19 18:26:33 2010@@ -82,3 +82,20 @@
+ *
+ * Used for information required for screen-reader users to understand and use
+ * the site where visual display is undesirable. Information provided in this
+ * manner should be kept concise, to avoid unnecessary burden on the user. Must
+ * not be used for focusable elements (such as links and form elements) as this
+ * causes issues for keyboard only or voice recognition users. To prevent that
+ * these properties get unintentionally overriden by themers they are enforced
+ * by "!important".
We can remove the description (not the summary) in the RTL styles.
+++ modules/system/system-behavior-rtl.css Wed May 19 18:26:33 2010@@ -82,3 +82,20 @@
+.element-invisible {
+ position:absolute !important;
No need to repeat position: absolute; in RTL.
78 critical left. Go review some!
#34
This should get be reviewed by someone with expert knowledge in this area.
#35
#36
Here are 3 articles that support the OFF-LEFT technique.
This article compares several screenreaders:
http://css-discuss.incutio.com/wiki/Screenreader_Visibility
Edge case: The above article points out that the "OFF-LEFT" technique might fail for long instructive texts. (-999em could not be enough).
This article by the "Royal National Institute of Blind People (RNIB)" also supports the OFF-LEFT technique:
http://www.rnib.org.uk/professionals/webaccessibility/wacblog/Lists/Post...
This article from "WebAIM" explains the OFF-LEFT technique in detail
http://www.webaim.org/techniques/css/invisiblecontent/
The additional use of downsizing the element to 1px is a backup for cases where positioning is disabled. >> Do we need that here?
#37
counter-recommendation: Just use left: -999em; along with corresponding RTL styles (left: auto; right: -999em;).@sun
Your suggestion doesn't always work in IE7 and IE6, thats why we are stuck with the rtl toolbar patch.
This blog (about drupal) is saying the same thing: http://www.held.org.il/blog/?p=260
But maybe we just have to css hack rtl if this is the best solution. :(
#38
what about patch #31? Its merely an extension of what we have at the moment, just fixing the holes.
#39
Suggest that the discussion about modifying .element-invisible, other than !important, be moved to #718922: system class: .element-invisible does not work with VoiceOver on OS X Snow Leopard, as a discussion is already going on there regarding how to rework .element-invisible.
1. display: none, does not work, screen-readers respect this and don't provide access to the element. This is expected and appropriate behavior.
2. Pushing the element above the top of the page can cause screen jumping if anything focusable is hidden. I don't personally have a problem with this since nothing focusable should * ever * be hidden with .element-invisible.
3. Hiding to left / right have the problems already discussed here.
4. That is why we went with the current approach, which AFAIK, was working well until VoiceOver on Snow Leopard stopped recognizing it.
Thanks.
#40
This should be added to the theming docs & maybe the upgrade page -> http://drupal.org/node/254940#element-hidden
#41
I think the !important needs to be applied. Without the !important in the core css, everyone is going to have to either override the core .element-invisible rule with one with higher specificity or use another convention; this all defeats the purpose of a shared convention like this.
For example, the element-invisible functionality is broken in "seven" theme. You can add the class=element-invisible attribute in the page preprocess function and it is applied correctly, but seven's css:
#branding h1.page-title {color: #000;
margin: 0;
padding-bottom: 10px;
font-size: 18px;
font-weight: normal;
float: left;
}
has more specificity and breaks the h1 heading of form:
<h1 class="element-invisible page-title">hide this title with element-invisible</h1>And it ends up with the bottom 2/3 of the title hidden; a definate unexpected mix of css rules.
#42
#718922: system class: .element-invisible does not work with VoiceOver on OS X Snow Leopard will fix that one johnbarclay
#43
And ya, it uses !important pretty darn extensively - http://drupal.org/node/718922#comment-2995514
#44
marking as duplicate since http://drupal.org/node/718922 addresses same exact code and would lead to conflict if both are accepted.
718922 is related to accessibility, which seems to trump in importance, plus it is also Reviewed & Tested by the community.
( found as part of #LADrupal code sprint while working on critical UI issues / new Barktik theme )