Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Changing the size of the user picture preset can break the display of comments - the user picture can overflow its containing DIV, because it is hard coded to be 110px wide.
This is extremely fragile design and being a core theme should, at the very least, allow users to take advantage of the standard options in Drupal core - namely being able to change the user picture preset and not have their site blow up. All other core themes support this.
I have crafted a patch to fix this - but only for browsers that can support display: table-cell; which means that for IE6 and IE7 I have retained the original layout (float: left; width: 110px;) and simply added overflow: hidden; to hide the overflow should the user increase the size of their user pictures.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | ie6-before-50x50.png | 88.66 KB | amateescu |
| #17 | ie6-after-50x50.png | 91.21 KB | amateescu |
| #17 | ie6-before-200x200.png | 101.83 KB | amateescu |
| #17 | ie6-after-200x200.png | 99.43 KB | amateescu |
| #17 | ie7-before-50x50.png | 86.89 KB | amateescu |
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedsigh....
Comment #2
Jeff Burnz CreditAttribution: Jeff Burnz commentedQuick roundup screen-shot to show the difference in display:table-cell supporting browsers and IE6/7.
Comment #3
Jeff Burnz CreditAttribution: Jeff Burnz commentedGot a chance to refine this a bit more this evening and fixed the IE issues - this patch allows for resizing the user picture and works in all browsers I tested. The IE RTL stuff probably looks odd in that I am re-declaring position:absolute; on the comment arrow - but this is what I had to do to get it position the arrow correctly.
Tested in IE6, 7, 8, Chrome 5, Firefox 3.6 and Opera 10 with both threading and non-threading.
Seems to an issue with indentation on threaded comments in IE6 and 7, but that looks like an unrelated issue (another issue...).
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commented#3: bartik-comment-user-picture-895898_2.patch queued for re-testing.
Comment #6
markabur CreditAttribution: markabur commentedHere's a reroll. I've done a quick check in Safari, FF, IE7 and IE8, but nothing extensive.
Comment #7
Jeff Burnz CreditAttribution: Jeff Burnz commentedCheers, will test tonight, what do you think of the approach?
Comment #8
markabur CreditAttribution: markabur commentedI think it makes sense. I like seeing a good use for display:table-cell. :-)
And inline/float for old IEs is an ok way to make it work there. Looking at it now, are both of those needed? Doesn't floating sort of automatically force a display:block? In fact I just did a quick test without display:inline in ie.css and it looks fine.
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commenteddisplay: inline is there to mitigate any chance of float margin bugs in IE http://www.positioniseverything.net/explorer/floatIndent.html since this whole thing needs to be pixel perfect. Call it insurance...
Comment #10
markabur CreditAttribution: markabur commentedSounds good, nobody says IE has to be logical!
Comment #11
Jeff Burnz CreditAttribution: Jeff Burnz commented#6: bartik-comment-user-picture-895898_3.patch queued for re-testing.
Comment #12
jensimmons CreditAttribution: jensimmons commentedGood catch!
Can we resize the image using CSS for IE 6 & 7? I'm afraid that someone who uses a modern browser will resize their user pictures, and think it looks great.... and never look at their site in IE 6&7... and so they won't know that the images are getting chopped off. We can shrink / enlarge the IE6/7 versions, and that will look less broken.
And have we tested this to see if it looks good when the user image size is shrunk down, not just made bigger?
Comment #13
Jeff Burnz CreditAttribution: Jeff Burnz commentedPatch in #6 fixes the chopped off overflow hidden hack I originally came up with so that should be no problem, ideally needs a new review as its been a while since I wrote this and can't remember even a single line!
Comment #14
tim.plunketttagging "Needs IE test"
Comment #15
Jeff Burnz CreditAttribution: Jeff Burnz commented#6: bartik-comment-user-picture-895898_3.patch queued for re-testing.
Comment #16
jensimmons CreditAttribution: jensimmons commentedIn keeping track of what's most important to fix for Bartik, I'm going to bump this up to major. Users should be able to resize the user pictures, and have the comments work whether the new size is bigger or smaller than the default size.
Comment #17
amateescu CreditAttribution: amateescu commentedI tested this patch in IE 6/7/8, FF, Chrome (LTR & RTL) and everything looks great. Screenshots for IE attached.
Comment #18
bleen CreditAttribution: bleen commented#6: bartik-comment-user-picture-895898_3.patch queued for re-testing.
Comment #19
bleen CreditAttribution: bleen commentedAs long as #6 turns green again, I'd say that it RTBC given #17 ... I've also looked in FF, Chrome & Safari and life is good
Comment #20
webchickI can't think of a reason a CSS patch could cause testbot to fail (famous last words...), so committed #6 to HEAD.
Thanks for all the thorough testing!!
Comment #22
markabur CreditAttribution: markabur commentedUnintended side effect, when the comment has little text: #1000610: Bartik comment layout was borked recently