steps to reproduce:

  1. install a fresh head
  2. click the contextual links trigger in the 'powered-by' block

expected result:
contextual links are visible

actual result:
contextual links get cut off at the end of the block

conclusion:
overflow:hidden; is so cool, it must be applied to everything and everywhere, twice if we have to⸮⸮⸮

CommentFileSizeAuthor
#42 875132-seutje.patch1.73 KBJacine
#32 875132-bartik-overflow-6.patch1.36 KBseutje
#32 875132-bartik-overflow-7.patch1.15 KBseutje
#32 875132-bartik-overflow-8.patch1.17 KBseutje
#27 drupal.block-clearfix.27.patch1.93 KBsun
#21 875132-bartik-overflow-4.patch3.4 KBseutje
#19 875132-bartik-overflow-3.patch751 bytesseutje
#4 contextual-overflow.patch568 byteswatcha
#1 bartik-overflow-hidden-footer.patch462 bytesJeff Burnz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Active » Needs review
FileSize
bartik-overflow-hidden-footer.patch462 bytes

none might be as good as twice to often?

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

We may need to apply this more widely since blocks as inline menus could be added to a number of regions, or other sorts of very thin blocks where the contextual links could get cut off.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

#1: bartik-overflow-hidden-footer.patch queued for re-testing.

watcha’s picture

watcha CreditAttribution: watcha commented
FileSize
contextual-overflow.patch568 bytes

I agree that overflow hidden shouldn't be everywhere, however this patch will make sure this problem is independent of all themes.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Component: contextual.module » Bartik theme

I like the approach off #4

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Component: Bartik theme » contextual.module

Changing component re #4.

watcha’s picture

watcha CreditAttribution: watcha commented
Component: Bartik theme » contextual.module
Status: Needs review » Reviewed & tested by the community

Works as advertised. Achieves the request with minimal amount of code and makes sure that other themes cant break this

bleen’s picture

bleen CreditAttribution: bleen commented

Adding screenshot from #934540: Block Configuration drop down menu hidden in small blocks. which was marked as duplicate of this issue:

bleen’s picture

bleen CreditAttribution: bleen commented

watcha ... you're not really supposed to mark your own patch as RTBC but this looks like a good solution to the issue.

RTBC++

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed #4 to HEAD. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Oh. And this is what it looks like now:

Full title shows up

watcha’s picture

watcha CreditAttribution: watcha commented

@bleen18 sorry forgot to mention that the patch was tested by
the team I work with. Won't be doing it in the future :)

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Fixed » Needs work

wtf, !important on a value that is the default? this is bonkers! o_o
not only is this widely regarded as bad practice, it makes a few very dangerous assumptions:
- ppl couldn't possibly want another overflow value on a block when it has active contextual links (imagine the delight when trying to make ur block into a progressive slider that falls back to a scrollable div, jolly good fun!)
- ppl couldn't possibly want to use D7 and still have some kind of support for IE6 (as it ignore !important altogether)
- ppl couldn't possibly want to put other stuff in a block in that region and offset it (as this creates a specific exception for regions with an active contextual link set)
- ppl enjoy watching the height of the entire document jerk around as they hover over a region (hover/unhover the region and watch that scrollbar go!)

I really wonder how this was tested, the body jerking around happens in pretty much every browser, as the (double) margin collapses without overflow:hidden, whereas it doesn't with the overflow:hidden, and I couldn't manage to reach the contextual menu in that block on IE6/7 in any way

so it "solved" 1 issue, and created 3 others. Add to that the fact that the only way to override this in a browser other than IE<7 is by using the same nasty practice and you can understand my "wtf?" reaction

no offense, but this feels a lot like "let's use a crappy method to attempt to cater for crappy themes instead fixing the crappy themes"
can any1 even justify the presence of the overflow:hidden? Are we so dead-set on endlessly overriding values that shouldn't even be there?

also note that this is being applied to the block wrapper *and* the content wrapper, so I seriously foresee this causing problems

re-opening

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

That was a nice rant.

Now can you suggest an alternative? #8 is a really nasty user-facing problem.

seutje’s picture

seutje CreditAttribution: seutje commented

well, since it's presence can't be justified, just get rid of it? -> #1

the only thing that would change (aside from making the contextual menu visible) is that the combined margin on .block and .content will collapse, but that's how margins are supposed to work, and with the current solution, they still collapse on hover...

my apologies if it seemed like a harsh rant, but I've noticed that it significantly lowers the probability of it being ignored

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

I still think #1 is the best fix, but I did not get a chance to test it really widely and have not kept a close eye on this issue - some good points raised, indeed important is a bit rough and actually goes against the policy of no !important in core (which really should have had an exception granted before being considered).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Ok, then let's get #1 RTBC and we can make the change.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

@seutje is right.

Proper fix is non-trivial, and we faced the very same problem space in various other issues without resolution already, but the patch that went in should be reverted.

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Needs work » Needs review
FileSize
875132-bartik-overflow-3.patch751 bytes

I'm actually a bit confused on what the actual intend is here, as these 3 successive statements all touch the margin of blocks in the footer:

#footer-wrapper .block {
  margin: 20px 0;
  border: 1px solid #444;
  border-color: rgba(255,255,255,0.1);  
  padding: 10px;
}
#footer-columns .block-menu,
#footer .block {
  margin: 0;
  padding: 0;
  border: none;
}
#footer .block,
#footer .block .content {
  overflow: hidden;
  margin: .5em 0;
}

if the point of the overflow is to keep the margins from collapsing, padding would probably be a better fit

attached patch removes the overflow: visible !important; and splits up #footer .block and #footer .block .content, leaves the margin on #footer .block and changes the margin to padding on #footer .block .content. The margin-top: 0; statement is there because a preceding statement (.content { margin-top: 10px; }, bartik's style.css line ~526) needs to be overridden

This causes it to look exactly the same, aside from fixing the contextual links issue
tested in Chrome7, FF3.6 and IE8 (can't rly test in IE6/7 as the functionality is fundamentally broken for that region before & after, slapping a background color on it seems to fix it, but that's a whole other issue altogether)

seutje’s picture

seutje CreditAttribution: seutje commented

do note I've only tested the effects on the powered-by block in the footer region, I am yet to test its effects on multiple blocks in the footer region and the main content region with and without block module enabled

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
875132-bartik-overflow-4.patch3.4 KB

found an unwanted repercussion when putting menu's in the footer
all menu links in the footer are floated, so the block doesn't rly clear anymore. Ideally, we'd want to apply clearfix to the block's .content , not the block wrapper itself
since the content class is hardcoded in the block.tpl.php and not built from the $content_attributes like all the rest, I see 3 options:
- copy the clearix declarations using #footer .block .content as selector (duplicate declarations no guud)
- make a block--footer.tpl.php template and add clearfix to the class attribute (might be confusing to new ppl)
- live with a non-clearing block content for menu blocks in the footer, visually resulting in slightly less space in between the blocks (not as it was designed)

attached patch opts for the second option (lesser of 3 evils imo)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Why don't we add .clearfix to all blocks? (however, on .block, not .block .content)

seutje’s picture

seutje CreditAttribution: seutje commented

that's what I tried at first, but .block .content has padding, so if the menu in there doesn't clear, that padding won't have any effect and the spacing in between blocks would be less
compare:
current, with overflow:hidden
without overflow:hidden, clearfix on .block
without overflow:hidden, clearfix on .content

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: contextual.module » markup

Moving into markup queue.

I only suggested .clearfix on .block to make it consistent with other template elements. Ideally, we'd do that and therefore achieve consistency.

However, if that won't be possible, then I don't really see a reason why we couldn't apply it on .block .content, or perhaps even on both.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Component: markup » contextual.module

Ouch...

As for the issue, yes the second option seems the lessor of the evils, but again, ouch, a whole template? Sorry, I'm so shocked since I haven't looked at core templates in lord knows how long, what a shame that travesty slipped through the cracks (hard coded content class).

I think understandably I am quite resistant to this, but what are the real alternatives?

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

x post

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: contextual.module » markup
FileSize
drupal.block-clearfix.27.patch1.93 KB

Status: Needs review » Needs work

The last submitted patch, drupal.block-clearfix.27.patch, failed testing.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Needs work » Needs review

Theres no real drawback to this that I can see, however for the majority of cases there is no win either - most blocks in most designs do not need clearfix, so in effect we're supporting edge cases here. Sure we need to fix the overflow issue in Bartiks footer blocks, but this seems a rather blanket solution to problems that don't yet exist. IMO core templates should be lean and mean, if we build to the 80% rule then this doesnt cut it, 80% of blocks don't need clearfixing.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Needs review » Needs work

freaken cross posts...

seutje’s picture

seutje CreditAttribution: seutje commented

@sun: wouldn't it be more interesting to add the clearfix (on the block wrapper) to the $classes via preprocess or something? feels a bit silly to print $classes *and* just add clearfix to the end

but like Jeff pointed out, this is sort of an edge-case. modifying the core template for a problem that exists in 1 theme seems a bit backwards, I would much rather fix the theme tbh

I'ma have another quick go at it before I hit the sack

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
875132-bartik-overflow-8.patch1.17 KB
875132-bartik-overflow-7.patch1.15 KB
875132-bartik-overflow-6.patch1.36 KB

k, here goes...
6 has a small discrepancy, but allows us to simply not care if the menu clears or not, puts clearfix on .block and tries to adjust the margins to accommodate
7 makes it look exactly the same, but slaps clearfix on all menu trees, not sure if this is desired
8 copies the clearfix css and applies it to #footer .menu (the first of the 3 evils stated in #21)

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

re: @sun in #24:

I only suggested .clearfix on .block to make it consistent with other template elements. Ideally, we'd do that and therefore achieve consistency.

I think this makes sense. I don't get why the clearfix class is applied to node templates and not block templates. I think most themes would end up adding the clearfix class anyway, so let's fix this and not make people override this template for such a stupid reason.

re: @seutje in #31:

@sun: wouldn't it be more interesting to add the clearfix (on the block wrapper) to the $classes via preprocess or something? feels a bit silly to print $classes *and* just add clearfix to the end

Totally agree with this. I assume @sun did it this way, because that's the way the node template is setup. IMO both this should be done in preprocess with the rest of the classes for consistency.

As for the Bartik issue, I like this patch: 875132-bartik-overflow-7.patch. I think applying a .clearfix on the menu tree is perfectly fine (it might even make sense in theme_menu_tree() itself). I don't think it would have a negative effect amywhere and the code is much cleaner.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented

@sun: wouldn't it be more interesting to add the clearfix (on the block wrapper) to the $classes via preprocess or something? feels a bit silly to print $classes *and* just add clearfix to the end

Totally agree with this. I assume @sun did it this way, because that's the way the node template is setup. IMO both this should be done in preprocess with the rest of the classes for consistency.

If you really must add clearfix to block template then no, I do not agree with this and how sun has it is better - because then its very easy to remove if you don't want it. Ramming this into the $classes variable means its much harder to get rid of and would be making big assumption about what themers/designers actually want. This is wrong - while certain assumptions must be made, its a very big assumption that a design demands float clearing, indeed that the designer wants or needs to use the clearfix method, which is just one clearing method of many. If its hard code we can remove it very easily (edit HTML), if its in the $classes variable then we're saying "learn PHP".

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

#34 I disagree. All of the default classes drupal provides are added via preprocess. The reason they are in this $classes variable and not in $attributes is to make "adding" classes easy. I am tired of these inconsistencies. What makes "that" class any more special than the others? Anyway, that's my 2 cents.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Needs review » Needs work

The default classes added to templates via $classes typcially do not carry any declarations, they're mere tokens without style - styling hooks the themer can use if they want to, otherwise they do nothing - clearfix is not the same, it carries a lot of style, and implications - that is the key difference here - I think whoever wrote this put a lot of thought into it and got it right, I believe it was JohnAlbin and EclipseGC and co but I can't recall, we're going back a bit now.

Edit: clarifying why I don't really like the idea of placing clearfix in the $classes variable and would prefer it stayed hard coded as per suns patch.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

Jeff - So then say you do not support adding clearfix class, if that's your gripe. I am aware of how/when these were added. I don't agree with it, and I am entitled to that opinion. We can disagree on that, it's fine. Just saying...

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs work » Needs review

Patches in #32 need more reviews. As I said in #33, I'm good with http://drupal.org/files/issues/875132-bartik-overflow-7.patch.

It doesn't touch core templates, and there haven't been any other objections, so I'm changing this to needs review, because it's not clear what actually needs work, if anything.

Jeff Burnz’s picture

Jeff Burnz CreditAttribution: Jeff Burnz commented
Status: Needs work » Needs review

Sure Jacine, I know you understand all this stuff, I was just clarifying my objection to those who may not understand my position or why I am bitching about this, no disrespect intended, my apologies if it came across that way.

I'm good with #33, http://drupal.org/files/issues/875132-bartik-overflow-7.patch also and fwiw this is what I do in my own themes.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs review » Reviewed & tested by the community

Jeff, no worries. You are entitled to your opinion too, of course. I'm not offended, just trying to keep the "bitching" and the number of comments to fix an issue at a minimum these days. Otherwise, it's hard to focus on fixing things, for me at least. ;)

So, it looks like we agree on the fix for this! YAY :D

Marking http://drupal.org/files/issues/875132-bartik-overflow-7.patch RTBC!

Thanks @seutje ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

That new function needs some PHPDoc.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs work » Needs review
FileSize
875132-seutje.patch1.73 KB

Oops. Here you go @webchick.

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Needs review » Reviewed & tested by the community

haven't been able to find any unwanted repercussions, but this feels a lot like RTBCing my own patch :x

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

All I did was add a comment, and I think this is ready, so RTBC +1. :)

tom_o_t’s picture

tom_o_t CreditAttribution: tom_o_t commented

#42: 875132-seutje.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.