robchur wrote:

It sounds like an IE bug with respect to the overflowing div.

ayg wrote:

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.

Attached:

herd wrote:

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.

michaeldaly wrote:

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:
http://en.kayakwiki.org/index.php/Rolling_Pool

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.

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.

michaeldaly wrote:

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.

(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.

lordtbt wrote:

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:
http://redwall.wikia.com/index.php?title=Template:Series_Books

michaeldaly wrote:

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.

michaeldaly wrote:

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:
http://test.kayakwiki.org/index.php/Imagemap_Fixes
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.

michaeldaly wrote:

I've just tested my changes in MW version 1.10 - works fine.

http://test.kayakwiki.org/index.php/Imagemap_Fixes has a link to my SVN page for the changes.

michaeldaly wrote:

Diff file for ImageMap.php

attachment imagemap.diff ignored as obsolete

michaeldaly wrote:

Diff file for ImageMap.i18n.php

Requires updates for all languages except English to conform with the new or changed English messages

attachment ImageMap.i18n.diff ignored as obsolete

michaeldaly wrote:

Diff file for ImageMap_body.php

attachment ImageMap_body.diff ignored as obsolete

michaeldaly wrote:

New source file for ImageMap - ImageMapArea.php

attachment ImageMapArea.php ignored as obsolete

michaeldaly wrote:

New source file for ImageMap - ImageMapAreaSet.php

attachment ImageMapAreaSet.php ignored as obsolete

michaeldaly wrote:

New source file for ImageMap - ImageMapPres.php

attachment ImageMapPres.php ignored as obsolete

michaeldaly wrote:

Diff files for three existing programs in ImageMap uploaded.
Three new files to add to ImageMap extension uploaded too.

ayg wrote:

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.

Created attachment 3723
Combined patch

Single patch file combining the above

attachment 9126.diff ignored as obsolete

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.

michaeldaly wrote:

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 http://pool.kayakwiki.org/index.php/Imagemap, in particular the changes (per Comment #7 From Raimond Spekking) in the section

http://pool.kayakwiki.org/index.php/Imagemap#Known_problems_with_browsers. Other documentation on the changes is in http://test.kayakwiki.org/index.php/Imagemap_Fixes.

attachment imagemap.diff ignored as obsolete

ayg wrote:

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.

michaeldaly wrote:

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.

ayg wrote:

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.

michaeldaly wrote:

Combined patch

Fixed tabs and comment

attachment ImageMap.diff ignored as obsolete

michaeldaly wrote:

Combined patch

Switch default for external links to false as per previous version

attachment ImageMap.diff ignored as obsolete

michaeldaly wrote:

Combined patch

Updated to take into account Tim's latest change to remove $wgImageMapAllowExternalLinks global.

attachment ImageMap.diff ignored as obsolete

michaeldaly wrote:

latest patch

Update patch to include recent changes made to trunk version by Raimond.

attachment ImageMap.diff ignored as obsolete

Assigned to extension developer

michael.daly wrote:

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).

michael.daly wrote:

Combined patch

Up-to-date for MW 1.13

attachment ImageMap.diff ignored as obsolete

michael.daly wrote:

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 http://test.kayakwiki.org/index.php/Imagemap_Fixes.

• Please review the backward-compatible changes made to the input parameters as

documented on http://pool.kayakwiki.org/index.php/Imagemap, in particular the
changes (per Comment #7 From Raimond Spekking) in the section
http://pool.kayakwiki.org/index.php/Imagemap#Known_problems_with_browsers.

michael.daly wrote:

Combined patch

attachment ImageMap.diff ignored as obsolete

michael.daly wrote:

patch file

Update to patch for most recent trunk version of i18n file.

attachment ImageMap.diff ignored as obsolete

Wow, this is fairly complex for an IE fix

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.

The patch fails to apply to the i18n file :(

michael.daly wrote:

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.

OK, I'm just wondering why imagemap_no_image2 has the same text the old one had. Why not keep that one?

michael.daly wrote:

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.

Odd, must of been part of the conflict errors. If they are different, then that's fine.

ayg wrote:

@@ -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.

michael.daly wrote:

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.

ayg wrote:

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.

ayg wrote:

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.

michael.daly wrote:

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.

ayg wrote:

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.

michael.daly wrote:

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.

Attached:

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.

michael.daly wrote:

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.