Last modified: 2008-10-06 17:22:56 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 9126 - Image maps fail to scroll in IE7
Image maps fail to scroll in IE7
Product: MediaWiki extensions
Classification: Unclassified
ImageMap (Other open bugs)
All All
: Normal minor (vote)
: ---
Assigned To: Tim Starling
: patch, patch-need-review
Depends on:
Blocks: 640
  Show dependency treegraph
Reported: 2007-02-28 06:54 UTC by lordtbt
Modified: 2008-10-06 17:22 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---

Minimal test-case illustrating issue with IE7 (488 bytes, text/html)
2007-02-28 17:37 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
Diff file for ImageMap.php (942 bytes, patch)
2007-06-05 02:36 UTC, Michael Daly
Diff file for ImageMap.i18n.php (1.66 KB, patch)
2007-06-05 02:38 UTC, Michael Daly
Diff file for ImageMap_body.php (15.60 KB, patch)
2007-06-05 02:39 UTC, Michael Daly
New source file for ImageMap - ImageMapArea.php (9.66 KB, text/x-wiki)
2007-06-05 02:40 UTC, Michael Daly
New source file for ImageMap - ImageMapAreaSet.php (3.45 KB, text/x-wiki)
2007-06-05 02:40 UTC, Michael Daly
New source file for ImageMap - ImageMapPres.php (17.34 KB, text/x-wiki)
2007-06-05 02:41 UTC, Michael Daly
Combined patch (49.19 KB, patch)
2007-06-05 18:19 UTC, Brion Vibber
Combined diff (49.86 KB, patch)
2007-06-05 20:06 UTC, Michael Daly
Combined patch (59.63 KB, patch)
2007-06-07 01:57 UTC, Michael Daly
Combined patch (59.50 KB, patch)
2007-06-15 20:09 UTC, Michael Daly
Combined patch (59.52 KB, patch)
2007-06-27 03:10 UTC, Michael Daly
latest patch (59.90 KB, patch)
2007-08-10 22:00 UTC, Michael Daly
Combined patch (137.88 KB, patch)
2008-08-24 17:20 UTC, Michael Daly
Combined patch (137.79 KB, patch)
2008-08-24 18:09 UTC, Michael Daly
patch file (140.59 KB, patch)
2008-09-13 19:43 UTC, Michael Daly
Combined patch (55.82 KB, patch)
2008-09-21 16:42 UTC, Michael Daly

Description lordtbt 2007-02-28 06:54:00 UTC
There is a bug (probably in IE7, if it is unfixable on Wikimedia please WONTFIX) relating 
to the imagemap extension:  <-- an imagemap in an overflow:auto 
div does not scroll with the content in IE7, it remains static as if the position 
were "absolute"

It does scroll in Firefox though.

This has also been reproduced at Wikia:
Comment 1 Rob Church 2007-02-28 15:31:06 UTC
It sounds like an IE bug with respect to the overflowing div.
Comment 2 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-02-28 17:37:13 UTC
Created attachment 3281 [details]
Minimal test-case illustrating issue with IE7

This appears to be due to position: relative on the image div.	In standards
compliance mode (ironically, not in quirks mode: try removing the doctype
declaration), IE7 appears to treat that as "fixed relative to the containing
block" instead of "offset relative to normal flow" for some godforsaken reason.

The position: relative is used so that the little "click me to get to the image
page" icon can be positioned absolutely relative to the image div.  To avoid
this it would seemingly have to be rewritten somehow to use some uglier method,
like using static positioning for the image div and relative positioning for
the icon. That should be perfectly possible, I suppose, since we know the
dimensions of the image in pixels, unless the user scales it somehow without
scaling the whole page (there's a FF extension for that, for instance).  The
current solution is cleaner because it doesn't have to refer to the dimensions,
but I guess we may have to drop that.  Or maybe we could overlay it with
ImageMagick instead of the client browser, or something.  I dunno.
Comment 3 Splarka 2007-03-01 07:17:06 UTC
A partial fix would be to remove the style="position:relative" from the div if
"desc none" were used, as it would be uneeded. But, the default behavior (of a
description showing) would still be broken in IE7.

A more complete partial fix (that would remove some functionality,
unfortunately) would be to give the div a class="imagemap", and then remove the
description link and positioning in IE70Fixes.css: "div.imagemap a {
display:none }" and "div.imagemap { position: static !important }". Slightly
alternate to this would be to have the descriptive (i) link sitting just below
the image in IE7, or a plain text link (ugly).

Neither are very suitable. Adding the (i) to the image with ImageMagick and the
link as part of the <map> seems best.
Comment 4 Michael Daly 2007-05-06 18:28:05 UTC
If the information icon is moved off the image, this can be fixed by using
nested <div>s in table mode.  That eliminates the need for any absolute/relative
positioning and the thing works for Internet Explorer.

I made this fix (and several others) in my code and it's demonstrated on my wiki:

I don't like the info icon overlapping the image at all, so this kind of fix
looks good to me.  I'm sure there are others who would disagree, but I offer the
solution for those who are interested.

This fix also solves the problems for bugs 8718 and 8788 as well = three for the
price of one.
Comment 5 Raimond Spekking 2007-05-06 18:36:02 UTC
It would be kind of you provide a unified diff against extension code in SVN.
Then we are able to review, test and apply. Thanks a lot.
Comment 6 Michael Daly 2007-05-06 19:21:27 UTC
I made the changes to my Imagemap (Alternate) extension, so, while it is based
in part on ImageMap, a diff will be very different.

At this point, I'm just inquiring whether there would be a strong objection to
the use of this approach and the movement of the info icon off the image.  My
web page lets you see the results and comment on that bit at least.

If it is considered generally acceptable, I could apply the changes to Imagemap
and then make that available for you.  That could take a day or so, depending on
my schedule.
Comment 7 Raimond Spekking 2007-05-06 19:31:03 UTC
(In reply to comment #6)
> At this point, I'm just inquiring whether there would be a strong objection to
> the use of this approach and the movement of the info icon off the image.  My
> web page lets you see the results and comment on that bit at least.

It will break all existing usages/designs if the icon will be per default off
the image. I suggest an extra parameter in the "desc" line if you think it would
be usefull. Could be nice in some circumstances.

> If it is considered generally acceptable, I could apply the changes to Imagemap
> and then make that available for you.

Yes, please file a diff against the actual imagemap code and we can test it. Thanks.

Comment 8 lordtbt 2007-05-06 19:41:02 UTC
What you are recommending for the fix for 9126 is what I believe Splarka said, 
which I've been doing for some time now here:

Comment 9 Michael Daly 2007-05-06 21:10:25 UTC
> It will break all existing usages/designs if the icon will be per default off
> the image. I suggest an extra parameter in the "desc" line if you think it 
> would be usefull. Could be nice in some circumstances.

It will move the image 26 pixels one way or the other.  For centred images
(using centre as a parameter in the image line) that makes no difference as that
is always done with float:none (assuming the image is not so large as to cause
horizontal scrolling by adding the 26 pixels - but that would be resolution
dependant anyway).  In other cases with float on the left or right, there will
be a slight shift.

Adding a parameter would mean, to me, two different versions - the old way with
its intendant problems or the new way with the icon on the side.  This is
straightforward, but not trivial, in the code, but might be confusing for some
who wonder why moving the icon illicits very different behavior with, say,
scrolling in IE.  (the old way could still mean fixing the basic problem of not
having a narrow <div> wrapped around the image to avoid the full-width
interference).  Give me several days and I can scratch together the ImageMap
version with this option enabled on the desc line for you to check out.

Yes, lordtbt, this is essentially the same as one of Splarka's suggestions, but
with the icon to the side rather than the bottom.  I think it looks nicer on the
side, but that's just one opinion :).

Moving the icon to the side could also minimize the need for an alternate icon
that contrasts with the background image (as per 9772).

Another alternative that would work sometimes would be to force a default of
"About this image" on the background wherever there is no clickable area
specified and no user-supplied default.  This is potentially ugly and inconsistent.
Comment 10 Michael Daly 2007-05-14 15:37:24 UTC
It took a while longer than I wanted, but I have made changes for this and bugs
8718 and 8788 to the current ImageMap code.  Due to the nature of the suggestion
to provide an extra parameter on the desc line, the program had to be
reorganized to parse all the input (to extract the desc parameter) prior to
outputting the image HTML.

I have provided a description of the changes, as well as links to a sample page
and the modifications to the input parameters on this page:
Please note as well the talk page there for a few unresolved issues.

In addition, a link on that page is provided to my SVN page where the code can
be downloaded.
Comment 11 Michael Daly 2007-05-19 20:41:17 UTC
I've just tested my changes in MW version 1.10 - works fine. has a link to my SVN page for the changes.
Comment 12 Michael Daly 2007-06-05 02:36:58 UTC
Created attachment 3713 [details]
Diff file for ImageMap.php
Comment 13 Michael Daly 2007-06-05 02:38:34 UTC
Created attachment 3714 [details]
Diff file for ImageMap.i18n.php

Requires updates for all languages except English to conform with the new or changed English messages
Comment 14 Michael Daly 2007-06-05 02:39:08 UTC
Created attachment 3715 [details]
Diff file for ImageMap_body.php
Comment 15 Michael Daly 2007-06-05 02:40:03 UTC
Created attachment 3716 [details]
New source file for ImageMap - ImageMapArea.php
Comment 16 Michael Daly 2007-06-05 02:40:39 UTC
Created attachment 3717 [details]
New source file for ImageMap - ImageMapAreaSet.php
Comment 17 Michael Daly 2007-06-05 02:41:08 UTC
Created attachment 3718 [details]
New source file for ImageMap - ImageMapPres.php
Comment 18 Michael Daly 2007-06-05 02:42:09 UTC
Diff files for three existing programs in ImageMap uploaded.
Three new files to add to ImageMap extension uploaded too.
Comment 19 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-05 02:47:10 UTC
As a general thing, please generate a unified diff file that encompasses all modifications and added/removed files by using the command "svn diff", so your changes can be more easily reviewed/applied.
Comment 20 Brion Vibber 2007-06-05 18:19:58 UTC
Created attachment 3723 [details]
Combined patch

Single patch file combining the above
Comment 21 Brion Vibber 2007-06-05 18:47:37 UTC
The code formatting is a bit hard to read IMHO; it would be nice to reformat it to match the rest of the code and be more consistent. Since it already does a lot of code refactoring, keeping it legible will make it easier to review.

Note to work with current trunk, you'll need to change the 'new Image()' call to 'Image::newFromTitle()' or use wfFindFile(), otherwise it will fail with Commons images.
Comment 22 Michael Daly 2007-06-05 20:06:44 UTC
Created attachment 3724 [details]
Combined diff

It seems that svn diff messes up the formatting by doing some funny expansion of tabs.  The original source looks fine in my editor.

I've updated ImageMapPres.php to use 'Image::newFromTitle()" instead of "new Image()" as per recent fix by Tim and previous comment.

Before testing:

- Note this fixes bugs 8718 and 8788 as well.

- Please review the backward-compatible changes made to the input parameters as documented on, in particular the changes (per Comment #7 From Raimond Spekking) in the section  Other documentation on the changes is in
Comment 23 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-06 01:26:38 UTC
Two more fairly unimportant details with your submission: you didn't click the "patch" checkbox, so it didn't display right; and you didn't follow our formatting standards, which makes the code a bit harder to review and apply (since it has to be modified to follow them).

Our coding standards are mostly pretty obvious by looking at the source: use tabs not spaces for initial indentation (this is why your diff file looks screwed up, it's not svn diff's fault); K&R braces (open block with brace on the same line as the opening keyword, close block with brace on its own line indented to same level as opening line); phpDoc-style block-level comments; and so on.

I don't have any substantive comments, because I never looked at this code before.  I'll CC Tim Starling, if he doesn't mind, since he wrote the extension.
Comment 24 Michael Daly 2007-06-06 04:57:48 UTC
K&R braces are difficult for me to use due to a visual defect that requires me to have more whitespace between lines of code and less dependence on things on the right.  I know that sounds odd, but that's what I deal with.

The code standards I read in the developer's guidelines allow for the use of open braces on the following line if they are left aligned with the closing braces.  I follow that.  I've also seen a mix of comment styles in the code.  As far as tabs only on the left - most of the MW code I've looked at is a mix of tabs and spaces in the first place.  This extension was as well.  If you insist, I can change the code to only use tabs on the left and can change the comments to use # instead of //; just say the word.
Comment 25 Aryeh Gregor (not reading bugmail, please e-mail directly) 2007-06-06 19:20:36 UTC
It doesn't matter much, really, as I said.  What I described is what your code will probably be reformatted to if it's committed.
Comment 26 Michael Daly 2007-06-07 01:57:03 UTC
Created attachment 3729 [details]
Combined patch

Fixed tabs and comment
Comment 27 Michael Daly 2007-06-15 20:09:00 UTC
Created attachment 3789 [details]
Combined patch

Switch default for external links to false as per previous version
Comment 28 Michael Daly 2007-06-27 03:10:27 UTC
Created attachment 3831 [details]
Combined patch

Updated to take into account Tim's latest change to remove $wgImageMapAllowExternalLinks global.
Comment 29 Michael Daly 2007-08-10 22:00:23 UTC
Created attachment 3985 [details]
latest patch

Update patch to include recent changes made to trunk version by Raimond.
Comment 30 Siebrand Mazeland 2008-08-17 22:13:25 UTC
Assigned to extension developer
Comment 31 Michael Daly 2008-08-18 04:40:20 UTC
Note:  I'm in the process of bringing my combined patch up to MW1.13.  I should have this available within a week - it's currently running on 1.12 on my wiki and I waited for 1.13 to be released before posting it here.  My patch fixes bugs 8718 and 8788 as well.  I hope to be done by the end of the week (say around Aug 23).
Comment 32 Michael Daly 2008-08-24 17:20:17 UTC
Created attachment 5215 [details]
Combined patch

Up-to-date for MW 1.13
Comment 33 Michael Daly 2008-08-24 17:28:34 UTC
Now updated to MW 1.13 with the version of ImageMap current as of 2008-08-24 around 16:00 UTC

Please review this patch.  I've been submitting this patch for over a year and it has not been reviewed.  

- Note this patch fixes bugs 8718 and 8788 as well.

- Documentation on the changes is in

- Please review the backward-compatible changes made to the input parameters as
documented on, in particular the
changes (per Comment #7 From Raimond Spekking) in the section 
Comment 34 Michael Daly 2008-08-24 18:09:00 UTC
Created attachment 5216 [details]
Combined patch
Comment 35 Michael Daly 2008-09-13 19:43:44 UTC
Created attachment 5331 [details]
patch file

Update to patch for most recent trunk version of i18n file.
Comment 36 Aaron Schulz 2008-09-13 20:26:04 UTC
Wow, this is fairly complex for an IE fix
Comment 37 Aaron Schulz 2008-09-13 21:09:55 UTC
Why is imagemap_no_image effectively moved to imagemap_no_image2?

Also, some functions have // function() after them, not sure what that is there for.
Comment 38 Aaron Schulz 2008-09-13 21:25:23 UTC
The patch fails to apply to the i18n file :(
Comment 39 Michael Daly 2008-09-13 23:12:50 UTC
Explanation of the change complexity is at the link referred to in Comment #33.

The text of the message "imagemap_no_image" is no longer valid.  My interpretation of the guidelines for changes is to replace the message with a new one - imagemap_no_image2.  I can see this being useful for identifying the change being needed for translators.  If that's not how it's done, I can fix it.

The comment on the end of the function just identifies the ending brace:

function  foobar(){
}  // function foobar()

I'll take a look at the i18n file again.

Comment 40 Aaron Schulz 2008-09-13 23:43:55 UTC
OK, I'm just wondering why imagemap_no_image2 has the same text the old one had. Why not keep that one?
Comment 41 Michael Daly 2008-09-14 05:15:16 UTC
They don't:
'imagemap_no_image'             => 'Error: must specify an image in the first line',
'imagemap_no_image2'            => 'Error: must specify an image',

The restriction that the image be on the first line is gone.
Comment 42 Aaron Schulz 2008-09-14 13:26:06 UTC
Odd, must of been part of the conflict errors. If they are different, then that's fine.
Comment 43 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-14 13:51:26 UTC
>@@ -40,6 +42,7 @@
> $messages['an'] = array(
> 	'imagemap_desc' => "Premite mapas d'imachens punchables en o client fendo serbir a etiqueta <tt><nowiki><imagemap></nowiki></tt>",
> 	'imagemap_no_image' => "Error: ha d'endicar una imachen a primer ringlera",
>+	'imagemap_no_image2'            => 'Error: must specify an image',
> 	'imagemap_invalid_image' => 'Error: a imachen no ye conforme u no esiste',
> 	'imagemap_bad_image' => 'Error: a imachen ye en a lista negra ta ista pachina',
> 	'imagemap_no_link' => "Error: no s'ha trobato garra binclo conforme รก la fin d'a ringlera $1",

Don't do this, updating non-English translations by just adding the English text.  Automated scripts will pick up the untranslated messages and give them to our localizers to translate.  Until they're translated, it will automatically fall back to English, so you don't need to change that explicitly.

Also, with those changes removed, your patch is still over 1500 lines, for a minor IE bug.  Whatever you're doing, it's almost certainly *way* overthinking the problem.  You've added three new files and rewritten the existing ones more or less from scratch.  This probably needs like a dozen lines of JS and maybe a couple of CSS rules.  Whatever you're doing, it's not the right way to fix this problem.  Bug 8718 and bug 8788 almost certainly shouldn't need anything approaching a rewrite either.

Generally speaking, it's pretty hard to get 1500-line patches with multiple pages of documentation reviewed, especially if they're for very minor issues like these three.
Comment 44 Michael Daly 2008-09-14 19:32:52 UTC
I'll remove the English bits.

As far as the changes go - there are not 1500 lines of changes.  There are 1500 lines of code involved in the change - much of it is identical to the old code.  Most of the code change is factoring out the Imagemap_body.php into two pieces.  The old Imagemap_body was large and unwieldy as well as poorly structured.  The existing structure cannot be modified to make these changes - it had to be re-factored to make it work.  If you actually look at the code, you'll see that it is mostly the same.  The other two new programs are just a couple of simple classes used for managing the data.

The original code looks like it was a rush job.  Patching a bad design is worse than redesigning it.  It had inconsistent input parsing and inconsistent error checking - there was one error that occurred under certain conditions that would silently fail, producing an image map that doesn't work correctly!  I condensed the parsing and error checking so that all input is handled the same way with the same degree of error checking.

In the process of fixing the bugs, I've relaxed some of the restrictions on user input formatting.  These restrictions are artificial and there is no reason to have them.  That produces a change to the documentation.  Rather than complaining about the documentation, you should be glad that I actually produced some.
Comment 45 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-14 19:47:37 UTC
Nevertheless, it's 1500 lines that need to be looked over.  It's particularly problematic if you simultaneously move code and change it, because then it's very difficult to tell from the diff what code was actually changed, since the actual lines that have been changed are hidden in the move.

I would suggest that you make a patch set here.  First make a patch that refactors the code without changing anything.  Then make another patch, designed to be applied after that one, for each logically separate change you're making.  You can attach them separately to this bug in the order they're meant to be applied.  If you do that, I'll be willing to review it, and commit it once I think all issues with it are fixed.  Personally, I'm not going to attempt to review such a large patch as the one you've attached here, especially when I can't figure out what exactly it changed due to moving stuff around, or why.
Comment 46 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-15 00:40:07 UTC
Er, wait, reading back to comment 4: this moves the icon off the image?  I don't know if we want that, just to work around an obscure IE bug.  (And it *is* obscure: how often are image maps used inside boxes with overflow: auto?)  You might not want to bother reformatting the patch to my specifications after all, since I'm probably going to have to say I just totally disagree with your approach.  As I said above, this really deserves a few lines of CSS or JS, nothing more.  I will look at your patches if you reformat them so they're readable, but I don't think I'm ever going to be willing to commit them if they refactor the core code and radically alter HTML output for a minor browser bug workaround.
Comment 47 Michael Daly 2008-09-15 03:12:41 UTC
It makes the position of the icon *optional* - on the image (default), off the image or no icon.  The default behavior is *no change* from the current behavior.  This allows the rare case of the scrolling window to provide a proper functioning scroll for IE and other browsers.  

Don't forget that this fixes two other bugs.  Both of those are due to the way that the image is put together in HTML.

I will not undo all the work I've done just so show you some kind of intermediate patch.  If you have such an objection, you should have raised it over a year ago.

If you think these three bugs can be fixed with a bit of CSS and JS, I'd like to see that.  If you're going to drop my work because you don't want to look at it, fine - but stop asking for volunteers.
Comment 48 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-15 15:34:36 UTC
I agree that it seems fairly difficult to fix these bugs without significantly altering the HTML.  I've improved the situation for this bug a bit with r40856, just removing position: relative from the div when there's no icon displayed, as Splarka suggested in comment 3.  It would be easy to work around this specific bug for IE7, but the easy workarounds would result in a change in functionality in all cases, not just in the extremely rare corner case dealt with in this bug.  The other two bugs seem a lot less tractable anyway with the current setup.

As I say, your patch is unreasonably difficult to review.  But I can try to pick out the HTML you generate and see if that's a better solution, and if so commit a patch based on it.  I'll leave this bug in my list of things to review, so I should get back to it sometime.

Sorry for the unresponsiveness -- MediaWiki devs tend to suck at reviewing patches in a timely fashion, or at all.  (Hopefully this will change when more paid developers are brought in.)  It's usually a bad idea to write up a big change without making sure you're getting review from the people who will have to commit it, in any open-source project, because maybe they won't like your approach and you'll have wasted your time.  But that doesn't excuse us encouraging you to write it and then nobody reviewing it for a year.
Comment 49 Michael Daly 2008-09-21 16:42:08 UTC
Created attachment 5355 [details]
Combined patch

Latest patch with i18n file cleaned up as per comments.

I have added a bug (15675) describing one of the bugs fixed with this patch.  This means this patch fixes four Imagemap bugs.

I think that it is a false economy to ignore this patch.  It would take less time to review this patch than to code and test the four bug fixes in a different way.

When I designed these fixes, I had done so while looking at all the other extant Imagemap bugs.  I can't remember which other bugs I was thinking of (since it was a year ago), but these changes set the base for further bug fixes being easier to do.
Comment 50 Tim Starling 2008-10-06 03:56:43 UTC
Fixed in 41720. 

(In reply to comment #49)
> I think that it is a false economy to ignore this patch.  It would take less
> time to review this patch than to code and test the four bug fixes in a
> different way.

I think you're wrong, I think fixing these bugs is much simpler than reviewing that patch. It's not a bugfix patch, it's a rewrite, and reviewing would not be a rubber stamp, there are things in it that would have to be changed. You don't even bother to explain *how* that huge patch fixes these bugs. 
Comment 51 Michael Daly 2008-10-06 17:22:56 UTC
On the contrary.  I specifically pointed to my wiki where the changes are implemented and documented.

The MW programmers have repeatedly complained about my proposed fixes without ever bothering to look at it.

Note You need to log in before you can comment on or make changes to this bug.