r82181 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r82180‎ | r82181 | r82182 >
Date:19:39, 15 February 2011
Author:mah
Status:reverted (Comments)
Tags:
Comment:
* (bug 27338) Gallery in 1.17 breaks for audio/video + ogghandler

Patch by DieBuche, who explains the CSS:

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

I've not tested this, but DieBuche and Derk-Jan Hartman tested and
provided screenshots in the bug.
Modified paths:
  • /trunk/phase3/includes/ImageGallery.php (modified) (history)
  • /trunk/phase3/skins/common/shared.css (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/shared.css
@@ -780,6 +780,10 @@
781781 margin: 2px;
782782 }
783783
 784+li.gallerybox div.thumb a.image img {
 785+ vertical-align: text-top;
 786+}
 787+
784788 div.gallerytext {
785789 overflow: hidden;
786790 font-size: 94%;
Index: trunk/phase3/includes/ImageGallery.php
@@ -156,11 +156,11 @@
157157 }
158158
159159 /**
160 - * Add an image at the beginning of the gallery.
161 - *
162 - * @param $title Title object of the image that is added to the gallery
163 - * @param $html String: Additional HTML text to be shown. The name and size of the image are always shown.
164 - */
 160+ * Add an image at the beginning of the gallery.
 161+ *
 162+ * @param $title Title object of the image that is added to the gallery
 163+ * @param $html String: Additional HTML text to be shown. The name and size of the image are always shown.
 164+ */
165165 function insert( $title, $html='' ) {
166166 if ( $title instanceof File ) {
167167 // Old calling convention
@@ -286,14 +286,13 @@
287287 $imageParameters['alt'] = $nt->getText();
288288 }
289289
290 - # Set both fixed width and height. Otherwise we might have problems
291 - # with the vertical centering of images where height<line-size
 290+ # Set both fixed width and min-height.
292291 $thumbhtml = "\n\t\t\t".
293 - '<div class="thumb" style="width: ' .($this->mWidths+30).'px; height: ' .($this->mHeights+30).'px;">'
 292+ '<div class="thumb" style="width: ' .($this->mWidths+30).'px; min-height: ' .($this->mHeights+30).'px;">'
294293 # Auto-margin centering for block-level elements. Needed now that we have video
295294 # handlers since they may emit block-level elements as opposed to simple <img> tags.
296295 # ref http://css-discuss.incutio.com/?page=CenteringBlockElement
297 - . '<div style="margin:'.$vpad.'px auto;">'
 296+ . '<div style="margin:'.$vpad.'px auto 0;">'
298297 . $thumb->toHtml( $imageParameters ) . '</div></div>';
299298
300299 // Call parser transform hook

Follow-up revisions

RevisionCommit summaryAuthorDate
r82215re r82181 remove min-height that doesn't work with IE6mah03:38, 16 February 2011
r82309followup r82181 and r82215 to fix the FIXME and botched fix for FIXME....mah23:21, 16 February 2011
r85151MFT: r82000, r82004, r82020, r82025, r82038, r82039, r82048, r82070, r82081, ...demon20:39, 1 April 2011
r91427better fix for bug 27338 - doesnt rely on line-heightkaldari21:30, 4 July 2011
r91573refixing bug 27338 with all parser test fixes, reverts r91557kaldari18:07, 6 July 2011

Comments

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:43, 15 February 2011

min-height doesn't work in IE6. This change caused the gallery to no longer render correctly on older browsers.

#Comment by Kaldari (talk | contribs)   19:46, 4 July 2011

min-height change reverted in r82215. 0 margin addition reverted in r82309.

#Comment by Kaldari (talk | contribs)   19:57, 4 July 2011

Forgot to mention that the css was also reverted in r82309.

#Comment by Reedy (talk | contribs)   18:02, 15 July 2011

Why is this marked 1.17wmf1? Does it need merging, or is this from previous when it was unreverted?

Status & tagging log