Last modified: 2008-10-06 17:22:56 UTC
There is a bug (probably in IE7, if it is unfixable on Wikimedia please WONTFIX) relating to the imagemap extension: http://test.wikipedia.org/wiki/User:Splarka/scroll <-- 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: http://redwall.wikia.com/index.php?title=Template:Series_Books&oldid=18788
It sounds like an IE bug with respect to the overflowing div.
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.
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.
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.
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.
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
> 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.
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.
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.
Created attachment 3713 [details] Diff file for ImageMap.php
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
Created attachment 3715 [details] Diff file for ImageMap_body.php
Created attachment 3716 [details] New source file for ImageMap - ImageMapArea.php
Created attachment 3717 [details] New source file for ImageMap - ImageMapAreaSet.php
Created attachment 3718 [details] New source file for ImageMap - ImageMapPres.php
Diff files for three existing programs in ImageMap uploaded. Three new files to add to ImageMap extension uploaded too.
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 [details] Combined patch Single patch file combining the above
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.
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 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.
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.
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.
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.
Created attachment 3729 [details] Combined patch Fixed tabs and comment
Created attachment 3789 [details] Combined patch Switch default for external links to false as per previous version
Created attachment 3831 [details] Combined patch Updated to take into account Tim's latest change to remove $wgImageMapAllowExternalLinks global.
Created attachment 3985 [details] latest patch Update patch to include recent changes made to trunk version by Raimond.
Assigned to extension developer
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).
Created attachment 5215 [details] Combined patch Up-to-date for MW 1.13
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.
Created attachment 5216 [details] Combined patch
Created attachment 5331 [details] patch file Update to patch for most recent trunk version of i18n file.
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 :(
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?
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.
>@@ -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.
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.
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.
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.
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.
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.
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.
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.
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.
