Page MenuHomePhabricator

Gallery in 1.17 breaks for audio/video + ogghandler
Closed, ResolvedPublic

Assigned To
Authored By
Feb 11 2011, 8:45 PM
Referenced Files
F7020: Screen_shot_2011-02-16_at_4.52.17_PM.png
Nov 21 2014, 11:12 PM
F7019: new.diff
Nov 21 2014, 11:12 PM
F7018: new.txt
Nov 21 2014, 11:12 PM
F7017: Screen_shot_2011-02-11_at_21.41.49.png
Nov 21 2014, 11:12 PM


The new gallery layout of 1.17 breaks when you have thumbnails for audio and video. The blocks have a fixed height, and this prevents dynamically loading elements like OggPlayer and mwEmbed from adapting the content and benefitting from auto sizing.

Perhaps min-height instead of height ?

Version: 1.17.x
Severity: normal



Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:12 PM
bzimport set Reference to bz27338.
bzimport added a subscriber: Unknown Object (MLST).

Created attachment 8128
screenshot of broken layout


Screen_shot_2011-02-11_at_21.41.49.png (470×664 px, 54 KB)

min-heigth sounds good. I'll make a patch

Min-height might cause the inline error messages to stretch a very larger area.... We should probably verify that.


No, the error messages are generated seperately, see ImageGallery,php L257-L272.
Here's the patch (had to create a new css rule to avoid regressing to a previous bug)


ehm, mind explaining the css rule ? perhaps even with a comment ? Might be helpful if one done someone figure "what the f is this for".

Created attachment 8135
screenshot of rendering with patch

attachment Screen shot 2011-02-12 at 17.55.12.png ignored as obsolete

If the image height is lower than the line-height, the margin-top is applied to the top of the line.
a very short image will not follow for 3-4px laters, thus leading to a bigger distance from the top than it should be.
vertical-align:text-top moves the picture up, so this problem doesn't happen

Trevor said this didn't work on older browsers, that we have to support. Could you test r82215 and see if it works for you?

better patch

Nope, this leads to the first screenshot again.
Here's a better patch:

Remove the css rule again, don't set a fixed height at all, but fix the margin calculation for small images

attachment new.diff ignored as obsolete

Better combined patch, fixes 27458 as well


screenshot of rendering with patch2


Screen_shot_2011-02-16_at_4.52.17_PM.png (682×803 px, 134 KB)

The patch has two problems. First you can't rely on the line-height being 17px, since it's actually 1.5em. Secondly, rounding the margin gives you different div heights for even and odd pixel thumbnails.

I've attempted to fix the first problem with r91427 and the second problem with r91426. Could others test and verify that this works for all cases. Thanks!