Page MenuHomePhabricator

Give image <gallery>s fluid width
Closed, ResolvedPublic

Assigned To
None
Authored By
bzimport
Aug 26 2005, 7:13 AM
Referenced Files
F2284: tests.diff
Nov 21 2014, 8:48 PM
F2285: untitled_6.diff
Nov 21 2014, 8:48 PM
F2283: a.diff
Nov 21 2014, 8:48 PM
F2281: bug_3276_results.zip
Nov 21 2014, 8:48 PM
F2282: fix_for_empty_lis.diff
Nov 21 2014, 8:48 PM
F2279: liquid.diff
Nov 21 2014, 8:48 PM
F2280: bug_3276_screenshot.png
Nov 21 2014, 8:48 PM
F2278: 3276a.patch
Nov 21 2014, 8:48 PM

Description

Author: nichalp.wiki

Description:
I use Opera 8.01 in Windows. The '''<gallery>''' tag creates
a very annoying horizontal scrollbar at 800x600 resolution.
Please could you reduce the width?

Mediawiki:Current


Version: unspecified
Severity: enhancement

Details

Reference
bz3276

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 8:48 PM
bzimport added a project: MediaWiki-Parser.
bzimport set Reference to bz3276.
bzimport added a subscriber: Unknown Object (MLST).

ayg wrote:

This could probably be accomplished by replacing the table with floated divs.
I'll put it on my to-look-at list.

ayg wrote:

Why floats won't work

I don't think the floated-div approach will work with captions. If some images
in the gallery have captions and others don't, some boxes will be taller than
others, and the next row break might not go all the way to the left, so it will
look like the attachment. I can't think of any way around this, offhand.

Attached:

Float_trouble_mockup.PNG (204×333 px, 2 KB)

ayg wrote:

My abortive attempt at a patch, in case it's useful to anyone

This partial patch has the flaw depicted in the previous attachment, but
otherwise is more or less working.

Attached:

ayg wrote:

Some more possibilities:

  1. Leave the table markup as-is, but translate using CSS into inline-block and -moz-inline-block.

It will be difficult to do this with CSS alone without mucking up the display for people whose
browsers partially implement the relevant attributes. On the other hand, it will look as it does
now for people with CSS1 or early CSS2 browsers, I suppose. Would require quite a bit of testing.

  1. Translate to divs (or spans to work around an IE bug) and use the inline-block approach. I

suspect this is more likely to work with IE. As a fallback, perhaps display: inline could be used
for the wrapping span, and block for the inside spans. I'm not sure offhand how that works, or if
it would work at all: it might be possible to do this without inline-block at all, if block boxes
inside inline boxes work the way I think they do. I'll look this up.

  1. Use some kind of JavaScript hackery. Icky, and also screws over the most likely beneficiaries -
  2. handheld users -- since many handhelds (I think?) tend to not execute JavaScript, unless perhaps

specifically forced to. Not really a useful option.

  1. Workaround: have a user preference to specify the maximum width (in pixels) to display

galleries, overriding the perrow setting even if that's specified in the content.

ayg wrote:

(In reply to bug 6987 comment #2)

If you take a div as container, and put divs for the images inside of that and
float these to the left, they'll automatically order themselves into a grid (or
vertical strip if you make the container small enough). That way you don't have
to keep browser width in mind.

You do have to make sure the blocks are all the same height though.

Yep, that's what I found out (see comment #2).

For an even
better solution I've tried to make the image divs behave like inline elements so
they would also line up if they aren't all the same height, but I wasn't able to
make that work.

Inline doesn't work, because the image and caption need to be laid out as block elements, and blocks
inside inlines break out of the inline. So you need inline-block. Absolute positioning could be
used, but that has the same problem as floats of knowing the height. Inline-block seems to be the
only realistic way to do this, although there may well be something I've missed.

daniel wrote:

We have a JavaScript solution working for some time now in commons.wikimedia.org.
http://commons.wikimedia.org/wiki/MediaWiki:ResizeGalleries.js

*** Bug 18213 has been marked as a duplicate of this bug. ***

As of now ResizeGalleries.js breaks the manual Gallery's perrow="". If set manually it shouldn't be overridden. http://commons.wikimedia.org/wiki/MediaWiki_talk:ResizeGalleries.js#This_script_breaks_Gallery.27s_perrow.3D.22.22

This is actually very easy:
Use divs with:
{
display:-moz-inline-stack;
display:inline-block;
zoom:1;
*display:inline;
}

add a vertical-align:top;
to cater for inconsistenst heights.

I'll add a patch later

Basic, working patch

here's a quick patch perrow is not yet refactored, best way imo would be to set a fixed width on the container div

attachment patch.diff ignored as obsolete

ayg wrote:

Could you explain what exactly these CSS rules do, and say what browsers you've tested in? Particularly, what does the "zoom: 1" do; and what browser are you hacking with the * there before "display: inline"? And how does -moz-inline-stack differ from inline-block, for old Firefox that doesn't implement the latter?

Perrow should just be eliminated if the width is fluid. There's no real need for it in that case.

zoom: 1 makes an element get hasLayout in IE.
For some reasons a block element that hasLayout and has display:inline explicitely set, is treated as inline-block in IE6+7.

I'll have to investigate into the differences between -moz-inline-stack in FF2 and inline-block in FF3
Heres a more detailed description of the process: http://blog.mozilla.com/webdev/2009/02/20/cross-browser-inline-block/

Another thing, my patch creates <div class="gallerybox">. Using <li class="gallerybox"> is semantically better though.

ayg wrote:

If you're willing to provide a comprehensive test case and test it in all important browsers, I'd be okay with committing this. The test case should include

  • Small images
  • Large images
  • Different caption lengths
  • Some exercise of various gallery options

If it looks right in at least IE6, IE7, IE8, Firefox 3.x, Opera 10.x, and recent Chrome/Safari, then I'd be willing to try checking it in. I'm a little nervous because I don't know how all the crazy IE stuff works. It would be nice if someone who knew more about old broken IE CSS handling could review this, like Trevor.

I'm not sure we have to worry about Firefox 2.x. According to http://stats.wikimedia.org/archive/squid_reports/2010-10/SquidReportClients.htm, it's around 0.5% market share, so I guess we should make sure it doesn't totally break.

Here's a testpage I build (using <li>'s now, I'll update the patch later..). Looks good for me in Webkit and Firefox and Opera. I'll test IE6-8 tomorrow at work.

ayg wrote:

Looks good to me. Trevor, could you (or someone else who knows how CSS works on old broken browsers) take a quick look at the CSS involved here and see if it seems to make sense?

patch 2

Here's the unified patch.

This time IE wasn't the troublemaker at all: IE6-8 behaved all as expected.
FF2 initally "stacked all images on top of each other", and after this was fixed, I needed to wrap the whole image in a div, because FF2 doesn't seem to accept display:block on a <li>

Anyway, it works now. Perrow is retained (It's not set by default though, but can be specified on individual gallerys)

It also happens to fix Bug 3770

attachment liquidGalleries.diff ignored as obsolete

ayg wrote:

Your patch is broken, the line numbers are missing:

@@ -%ld,%ld +%ld,%ld @@

Could you generate a correct patch using "svn diff"? (Or is this broken one generated by that and it's a bug in SVN?)

ayg wrote:

(Sorry for the slow response, by the way. I'll try to be more prompt for future iterations if I can manage it.)

patch (with line numbers)

This was weird. I used the normal svn diff. Too lazy to investigate though, a new one is attached.

Attached:

ayg wrote:

Screenshot with patch applied, incorrect results

Testing the patch on a simple gallery,

<gallery>
File:Test.gif|Foo
File:Test.gif|Bar
File:Test.gif
File:Test.gif
File:Test.gif
File:Test.gif
File:Test.gif
</gallery>

it doesn't seem to work as expected. I did action=purge and Shift-refresh to ensure that everything was as it should be, but the boxes were stacked on top of each other in both Firefox 4b7 and Chrome dev, as illustrated in the attached screenshot.

Attached:

bug_3276_screenshot.png (751×196 px, 131 KB)

Weird, I can't reproduce that (in neither chrome nor ff4). You applied it to the trunk, right?
I just tried again with a clean install from svn & it behaves as expected.
(I updated the dropbox link with a page generated by that patch, a perrow example is added as well)

Do you maybe have something weird in Mediawiki:common.js/.css (etc.)?

Otherwise, could you attach a zip of the whole page (firefox's save as..)

ayg wrote:

Before and after HTML output with patch applied

I'm running trunk, and the patches applied cleanly. It's not a clean trunk install, but if it breaks my dev install it will probably break other people's too, and I certainly can't commit it if I can't verify that it works. I don't have anything in {Common,Monobook}.{css,js} as far as I can tell, and I don't see anything in LocalSettings.php that seems relevant.

before.html in the attached zip is what I get on regular trunk for the markup I gave in my last comment. After applying your patch and doing action=purge and Shift-refresh, I get after.html. I briefly tried to debug it, but I don't have time to look closely and didn't properly understand all of your changes in the first place. Plus I get a sad tab in Chrome when trying to use Web Inspector here and Firebug doesn't work in Firefox 4.0 yet, so that makes it harder.

Attached:

weird, something in your install is causing the generation of empty <li> before each image <li> (<li style="list-style: none"></li>) These are by default display:block, so they add a newline before each image. I could fix that with css easily, but I'd still be interested where they come from. I'll have a look at that this evening.

Fix empty list elements

I greped through the trunk & all extensions, but couldn't find any code that could be responsible for those <li>s. Anyway, a new patch is attached, should work with those now.

Attached:

ayg wrote:

It's caused by $wgUseTidy = true;. It seems like a Tidy bug -- I don't know what objection it had to your original markup, it validates just fine. A minimal test-case exhibiting the problem is

<ol>
<li>x</li>
</ol>

(That's a tab for indentation, not spaces.) It inserts a bogus <li> there, so the number displays as "2.", not "1.". I worked around it by just removing all the tabs from your patch, so now Tidy doesn't mess it up so badly. I should probably report the bug to Tidy's maintainers, but I don't really have time.

With the patch modified thus, it looks okay, but the display is changed a bit. The old markup had a bit less space between the images, and a border around the whole thing. It's not a big deal, but were you aiming for pixel-perfect identical rendering to the previous code here? If so, I'm not seeing it.

Other than that, it looks fine now. I'd prefer if you could tweak it to look just the same as before, but if you don't want to, I'm okay with committing it as-is, given how long it's taken me to get back to you all this time.

Ah, ok, I'll file a bug for that later on.
Regarding the space between images: one margin got duplicated. remove "margin: 2px;" in shared.css, line 794 & it should look the same.
I removed the border attribute, because it tends to look weird if you have an odd number of images in the rows (4-4-2 etc.). Also, due to the box model, the border would always be on the very right side of the screen, even if the images have a linebreak before that.

(In reply to comment #26)

It's caused by $wgUseTidy = true;. It seems like a Tidy bug -- I don't know
what objection it had to your original markup, it validates just fine. A
minimal test-case exhibiting the problem is

<ol>

<li>x</li>

</ol>

(That's a tab for indentation, not spaces.) It inserts a bogus <li> there, so
the number displays as "2.", not "1.". I worked around it by just removing all
the tabs from your patch, so now Tidy doesn't mess it up so badly. I should
probably report the bug to Tidy's maintainers, but I don't really have time.

I'd think it's caused by the "tab-to-&#9;"-hack introduced in r42257. At least that was the problem with the similar bug 16108.

ayg wrote:

Reverted r42257 in r77410, and committed the patch (with style fixes, parser test fixes, fix from comment 27) in r77411.

Followup

Here's a follow-up. It does three things:

  1. Adds a line css to actually fix Bug 3770 (Sorry, I had forgotten to paste it there as well)
  2. Fixes a problem, where the box size would be slighty higher if the image thumb has a height of less than 8px
  3. Normalize the calculation of the vertical padding to be exactly the same one as the horizontal padding. (This is only relevant for unusually large galleries, eg. previously it returned a not-exactly-square box if you specified 500px as both width and height)

Attached:

(In reply to comment #30)

Followup

Here's a follow-up. It does three things:

thx. committed.

r77411 has a problem on Firefox, when there is a floating object. It looks like a clear:both. The old table-based gallery hasn't this problem.

<div style="float:right; width:10em; height:10em; border: 1px solid red">Box</div>
<gallery>
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
Image:Köln Panorama.jpg
</gallery>

r77411 defines
ul.gallery { display: inline-block }

It should define
ul.gallery { display: block }

@DieBuche, can you fix the parser tests please ? I looked at them, but I can't figure out what the desired behavior should be.

@Fomafix this behavior is expected. A gallery is now full width (and will thus clear floating objects), unless a perrow is specified. I note btw that this will cause some serious harm in articles most likely.... Perhaps we should only be fullwidth in the "file list" mode, and use a perrow=4 default in normal <gallery> tag mode, unless perrow="auto"

ayg wrote:

I think it should be full-width by default. If it interferes with floats in particular cases, those can be fixed manually. Surely it's pretty rare to have floats next to galleries? Making it not flexible width by default more or less defeats the point.

full-width as default is good, but a clear for floating objects is not good. With

ul.gallery { display: block }

its possible to have full-width without a clear for floating objects. At least for Firefox.

ayg wrote:

Yeah, actually, floats should play nice with inline-block, right? The line boxes should get shortened, just like with regular inline stuff. Done in r78232.

Tests

I can't seem to remember why I made the ul inline-block, seems to work this way as well. Anyway parser test patch is attached

Attached:

Cite

cite test patch as well

Attached: