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;
}
Comment | File | Size | Author |
---|---|---|---|
#35 | system-behavior-alternative.patch | 1.07 KB | derjochenmeyer |
#31 | system-behavior.patch | 1006 bytes | derjochenmeyer |
#32 | system-behavior-alternative.patch | 1.65 KB | derjochenmeyer |
#30 | system-behavior.patch | 1008 bytes | derjochenmeyer |
#18 | system-behavior.patch | 956 bytes | derjochenmeyer |
Comments
Comment #1
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere is a patch.
Comment #3
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere we go again. UTF-8 without BOM.
Comment #4
derjochenmeyer CreditAttribution: derjochenmeyer commentedBot: Test it!
Comment #5
aspilicious CreditAttribution: aspilicious commentedWe decided to leave !importants out of core, you can add them in your theme.
Comment #6
derjochenmeyer CreditAttribution: derjochenmeyer commentedSeems it needs UTF-8?
Comment #7
derjochenmeyer CreditAttribution: derjochenmeyer commentedOh, 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?
Comment #9
aspilicious CreditAttribution: aspilicious commentedIt needs unix style endings your patch, if you are working on windows, download notepad++ and choose unix style.
Crosslinking: #730754: CSS coding standards: No !important in core/contrib modules, ONLY IN THEMES
Comment #10
derjochenmeyer CreditAttribution: derjochenmeyer commentedThanks 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
,overflow
andposition
is accessible and good practice, butheight
andposition
are 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: none
ortext-indent: -9999px
we IMO need !important to make this method safer.Anyone who explicitly wants to show these elements can do it by simply adding something like
Comment #11
aspilicious CreditAttribution: aspilicious commentedI'll ask sun to jump in, he started the discusion ;)
Comment #13
derjochenmeyer CreditAttribution: derjochenmeyer commentedMy fault, next try.
Comment #14
JohnAlbinHmmm… 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.
Comment #15
JohnAlbinCan't find them in IRC. Pinged them in twitter. Setting back to "Needs review" until I hear from them.
Comment #16
sunWhy 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.
Comment #17
JacineI 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.Comment #18
derjochenmeyer CreditAttribution: derjochenmeyer commentedImplemented suggestions of #17
Comment #19
derjochenmeyer CreditAttribution: derjochenmeyer commentedI 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.
Comment #20
sunI'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.
Comment #21
derjochenmeyer CreditAttribution: derjochenmeyer commentedRight now the element is hidden by
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.
Comment #22
Frando CreditAttribution: Frando commentedWhy aren't we using display: none; ?
Comment #23
derjochenmeyer CreditAttribution: derjochenmeyer commentedsee: #473396: Defining System-Wide Approaches to Remove, Make Invisible & Push Content Off-screen with CSS
Comment #24
sunThis means we should add
left: -999em;
, so as to actually position the element invisible?Comment #25
derjochenmeyer CreditAttribution: derjochenmeyer commentedIt has been considered in this post but was not implemented: http://drupal.org/node/473396#comment-1894592
EDITED
Comment #26
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere 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:0px
No 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:
Lets think about what a themer might do with a typically hidden element like
.block h2
to 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.
Comment #27
aspilicious CreditAttribution: aspilicious commentedThe 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?
Comment #28
derjochenmeyer CreditAttribution: derjochenmeyer commentedTaking all this into consideration here is a suggestion which seems best practice:
Why:
To enforce all this and prevent any unwanted overwrites we add "!important" to all of those.
Comment #29
sunCounter-recommendation: Just use left: -999em; along with corresponding RTL styles (left: auto; right: -999em;).
Comment #30
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere is a patch that implements suggestions form #28 above.
Comment #31
derjochenmeyer CreditAttribution: derjochenmeyer commentedPatch as suggested in #28 (removed blank line from #30)
Comment #32
derjochenmeyer CreditAttribution: derjochenmeyer commentedPatch as suggested in #29
Comment #33
sun"!important" is used to prevent unintentional overrides.
Missing space between colon and value.
We can remove the description (not the summary) in the RTL styles.
No need to repeat position: absolute; in RTL.
78 critical left. Go review some!
Comment #34
tstoecklerThis should get be reviewed by someone with expert knowledge in this area.
Comment #35
derjochenmeyer CreditAttribution: derjochenmeyer commentedComment #36
derjochenmeyer CreditAttribution: derjochenmeyer commentedHere 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?
Comment #37
aspilicious CreditAttribution: aspilicious commented@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. :(
Comment #38
derjochenmeyer CreditAttribution: derjochenmeyer commentedwhat about patch #31? Its merely an extension of what we have at the moment, just fixing the holes.
Comment #39
Everett Zufelt CreditAttribution: Everett Zufelt commentedSuggest 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.
Comment #40
mgiffordThis should be added to the theming docs & maybe the upgrade page -> http://drupal.org/node/254940#element-hidden
Comment #41
johnbarclay CreditAttribution: johnbarclay commentedI 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:
has more specificity and breaks the h1 heading of form:
And it ends up with the bottom 2/3 of the title hidden; a definate unexpected mix of css rules.
Comment #42
aspilicious CreditAttribution: aspilicious commented#718922: system class: .element-invisible does not work with VoiceOver on OS X Snow Leopard will fix that one johnbarclay
Comment #43
mgiffordAnd ya, it uses !important pretty darn extensively - http://drupal.org/node/718922#comment-2995514
Comment #44
mike stewart CreditAttribution: mike stewart commentedmarking 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 )