Contextual link colors easily overwritten

It is usually not recommended and bad practice to use !important. It's also not implemented in IE7. A way around it is to add more selectors for the overriding property, or remove some not needed selectors on the original CSS definition.

Here's an attached patch that removes some selectors on the original property that we don't really need so that the overriding property take precedence. This means that themes don't need to drill down as deep as four selectors to override the property.

Would be great if Gabor could test Rob's patch.

I think the original approach from Gábor was correct. We want the contextual link styles to not be overridden in a way that makes the interface unusable in situations as common as adding a style such as #footer a { color: #fff; }.

While I agree that !importantis generally evil, I think this is a valid use case. The !important declaration in the original patch should also work as expected in IE7 (and even IE6, its bugs with !importantdon't stop the rule taking effect in simple cases like this). It's also still possible to override an !important style declaration from a theme by providing your own !importantstyle declaration with a higher specificity (for those who want to opt in to overriding these styles).

Okay, I have a better understanding now. The problem is that it is too easily over-ridable by the theme. It was the opposite direction that I thought it was going. If we add another selector to the contextual.css definition, then that one will take precedence over other definitions that have three or less selectors in it and we can avoid !important.

Thanks all!

Whoops, created the patch incorrectly. Haven't had coffee yet.

Hi Rob,

Unfortunately the last patch still doesn't fix the given problem with a selector such as #footer a { color: #fff; } (which is a common example). The specificity of a CSS selector is calculated as: (number of elements) * 1 + (number of classes) * 10 + (number of ids) * 100.

So, .contextual-links-wrapper ul.contextual-links li a has a specificity of 23, while #footer a has a specificity of 101.

We'd need to add 10 extra classes to the selector to equal the specificity provided by a single id. For this reason, and because it is undesirable to accidentally restyle these links I think !important is justified in this case. It can still be overridden easily by theme's if they want to, but protects against accidentally overriding the style and forcing theme developers to redeclare the contextual links styles throughout their themes.

A short comment on the reason we're using !important in this place would be good as it can be a bit of a developer WTF. But other than that I think this is RTBC.

That's a shame. I didn't know identifiers had such importance in the cascade. I thought it was just based on the number of selectors, but it seems I was wrong. Do you have any more information, or a link on how the weighting is made? Thanks!

Works with me! It's unfortunate that we have to have !important CSS in Drupal core, but if it helps the Drupal 7 Themer Experience, then I'm alright with that.... #D7TX? ;-)

Committed to CVS HEAD. Thanks.

It's unfortunate that we have to have !important CSS in Drupal core...

Oh my, this is the first instance I see where we really need to set !important either in Drupal core or any theme I do. :-)

Adding an ID to each "contextual-links-wrapper", like we do for blocks, would make things harder.

oups

#730754: CSS coding standards: No !important in core/contrib modules, ONLY IN THEMES

So we've put an !important into core, just because someone wrote a buggy theme, which doesn't support the modules being installed?

Yes this definitely looks like it's the responsibility of the theme, not the module. Set it back, the theme author should just have a #footer ul.contextual-links li a {} selector in his css file, or better yet a #page ul.contextual-links li a {} selector that will overwrite ALL of these links.

#page ul.contextual-links li a has a specificity of 1,1,3
vs
#footer a of 1,0,1

So the first will win for ALL contextual-links in the entire system at ALL times unless someone does something more specific (which is unlikely).

Eclipse

Yeah, EclipseGc is right. There are a few ways to resolve this, and it belongs in the theme.

@EclipseGc: please don't. You can't rely that #page will be there in the first place, the theme can override that (although they could re-implement this declaration, but what if we apply this approach elsewhere?). We can't use #identifiers for the links since they wouldn't be unique, so I guess we are doomed.

Sanity check: why are we using #footer id in the first place? I know it is unique within the scope of the document, but it would make more sense to preserve ids for things like #node-1234, where I might use it to override the default .node properties.

So I guess this would be: #page .selector for D7, rethink the usage of ids for D8. Also, RobLoach approach is right: if the selector is not needed, remove it. In the end, they should look like:

.contextual-links a {
...
.contextual-links a:hover {

I have just read the CSS Code Standards http://drupal.org/node/302199 and there's nothing about "removing unnecessary weight". So I don't know whether I should patch for that, even though the CSS file uses .contextual-links or ul.contextual-links randomly.

typo.

Adding DTX to avoid remember this bug even if it lingers until D8.

This issue will be resolved in: #849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers