Updated: Comment #8
Problem/Motivation
Contextual links look messed up in Bartik's footer.
Proposed resolution
Make them work fine by fixing Bartik's inappropriate behavior to override the styling of *every* link in the footer. (And the font size, too.)
- Before
- After
Remaining tasks
None.
User interface changes
Contextual links finally won't look like shit anymore in Bartik's footer!
API changes
None.
Related Issues
None.
Original report by @username
See the attached before and after images. This patch assumes that the !important is in fact necessary, so it adds it for the hover state as well. The alternative would be to remove from both. I am not sure what condition would necessitate the !important anyway unless a themer improperly chose their selectors.
before
https://drupal.org/files/Screen Shot 2013-07-30 at 4.11.49 PM.png
after
https://drupal.org/files/Screen Shot 2013-07-30 at 4.12.05 PM.png
Comment | File | Size | Author |
---|---|---|---|
#21 | bartikfooter7.50.png | 11.33 KB | littlepixiez |
#21 | bartikfooter7.51-dev.png | 15.43 KB | littlepixiez |
#18 | bartik_footer_links_breaks_contextual-2054055-18.patch | 1.13 KB | bnjmnm |
#14 | bartik_footer_links_breaks_contextual-2054055-13.patch | 1.13 KB | bnjmnm |
#8 | after.png | 44.52 KB | Wim Leers |
Comments
Comment #0.0
YesCT CreditAttribution: YesCT commentedembedded images.
Comment #1
markhalliwellLooks good to me.
Comment #2
webchickWould love at least one more eyeball from someone with CSS chops. !important always gives me the willies.
Comment #3
seanrwebchick, I can see why it was done, though, since it is very likely that a contributed (or even core) theme could accidentally override the link color (even just #footer a {color:white;} would do it). In fact, Bartik's style.css at line 955 would do it. There's not a good way to make contextual specific enough to avoid that since we can't use IDs for all of the contextual elements.
Comment #4
markhalliwell^ is a themer and has "CSS chops". I agree with @seanr's statement in #3. If we "need" to override it, all we have to do is do another !important, but I have never styled contextual links.
Comment #5
markhalliwellComment #6
Wim LeersI wish there were a way to solve this without
!important
, but then it probably also wouldn't be in the preceding selector. So, RTBC.(I tested this to verify it's still working.)
Comment #7
alexpottThis feels very wrong to me. I think we should remove all the
!important
and fix what was reported in the original issue #849926-66: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readersComment #7.0
alexpottformatted src correctly with actual spaces
Comment #8
Wim LeersThe problem is Bartik's
#footer-wrapper a { color: something }
statement, which has a higher specificity than.contextual-region .contextual .contextual-links a { color: something }
because of the ID.Conclusion: Bartik should not use an ID, or at the very least, it should not target an extremely generic element such as
a
in a very generic way (i.e.#footer-wrapper
: for everything in the footer).So the solution is to s/
#footer-wrapper
/#footer-wrapper .block .content
/, which is what it's actually targeting anyway.This in fact also solves a problem that has existed for years: Bartik's footer styling messes up all contextual links in the footer, and in many more ways than only on hover. See for yourself:
Assigning to Jesse Beach for review.
Comment #9
markhalliwellThanks @Wim Leers! This is certainly a much more preferable approach. I just didn't take the time to dig too deeply and discover the cause of this.
No !important++
Comment #10
seanrNice, looks much better.
Comment #11
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for backport.
Comment #12
Wim LeersComment #13
bnjmnmI am working on the backport to 7.x
Comment #14
bnjmnmI backported the patch to 7.x
Comment #15
bnjmnmComment #16
seanrWith the exception of the missing space before "{" I think this is good.
Comment #17
Wim LeersI agree with #16: bnjmnm's backport is good to go, besides that missing space. No regressions in manual testing. Marking "needs work" for that reroll :) Thanks, bnjmnm!
Comment #18
bnjmnmCorrected css spacing from patch originally submitted in comment 13 - ready for review + thanks to everyone helping me along as I get accustomed to the core contribution process!
Comment #19
mimes CreditAttribution: mimes commentedI can't seem to see the changes made by this patch.
Comment #20
bnjmnmComment #21
littlepixiez CreditAttribution: littlepixiez commentedCurrently on Bartik 7.50, the footer's contextual links have underlines.
I've tested this patch on Bartik 7.51-dev and it removes the underline on hover.
This appears to produce the desired result and I can see the changes in the CSS from the patch. Can we close this issue?