User talk:Edokter/pngtest

About switching to the 1px transparent image version

Just so you know, one major reason I'm reluctant to switch to the 1px transparent image version is because no pages seem to have speed issues with the currently live code (pages with 500 PNG/SVG images are very rare). Even en:Wikipedia:Route diagram template has been split up so that there are no speed issues there either. So, to me it seems that switching to your recommended code would not give us any significant speed boost and it would introduce new page layout problems. —Remember the dot ^(talk) 19:29, 29 September 2007 (UTC)

I think there are a few issues that warrent looking into:

1. PNGs in collapsed tables still don't show (including on the route diagram page), especially on a lot of user pages with collapsed userbox tables. Not sure if my code will fix that though...
2. Double spans is what makes rendering so slow and takes a lot of resources from the browser.
3. Not all images are transparent.

I have been struggling along and have two scripts that leave me puzzled; one with an outerHTML generated span, in which the vertical spacing between the spans is unstable (based of the set font-size), and one with a dynamically created span, like the current code. That one has the spans placement stable, but the image within jumps 1px down. Could you have a look if I missed something obvious? If either of these implementation could be made to work, (hopefully) all issues mentioned above would also be solved.

In any case, you could consider putting the border-check in; that alone would save a lot of processing with borderless images, as simply adding the filter to the image has no layout problem whatsoever. — Edokter • Talk • 22:40, 29 September 2007 (UTC)

BTW. I find that IE6 template on your userpage extremely derogatory, please remove it. Some, like I, do not have the choice to upgrade or switch. I'm tempted to put it up on MfD as misplaced marketing. Wikipedia should be made to work on all browsers; including for the 37% of visitors that still use IE6. — Edokter • Talk • 22:57, 29 September 2007 (UTC)

Derogatory? Was a single thing I said on it not true?

As for making Wikipedia work perfectly on all browsers, that is simply not feasible. What about Internet Explorer 4.0? Should we expend time and effort trying to make Wikipedia work perfectly on that browser? Internet Explorer 6 started out broken. There's only so much we can do for it.

What you refer to as "outerHTML generated" and "dynamically generated" spans are (or should be) the same things, just done different ways. Both dynamically generate HTML. I'm not sure why they behave differently.

I would be very interested if the script could be made to run only on transparent images. However, I have not found any way to discern whether an image is transparent or not.

CSS directives for padding and margin also increase the display area of the image element. It is best to move all CSS to the outer span to make sure that nothing is lost when the filter forces the image element's display area to be no larger than the image itself.

I'm almost positive that the problems with collapsible tables are caused by this bug. When I copy and paste a collapsible table into the sandbox, all by itself, it works perfectly. —Remember the dot ^(talk) 02:04, 30 September 2007 (UTC)

What I mean by outerHTML generated is that the HTML is built as a string, while the 'dynamic' one is created with a createElement. What I would like is a debug method that allows me to read out the entire (current)Style property of the generated span; I know that somewhere in there is the answer. A method to copy (not just reference) the img style object to the span might also fix the problem. — Edokter • Talk • 12:26, 30 September 2007 (UTC)

I'm also confused as to why you insist that you cannot use Firefox. If you edited Wikipedia from work, school, a library, or an internet café (where you would not have administrator privileges over the computers), you would not be able to edit nearly as frequently, if at all. The frequency of your editing and your ability to install the IE developer toolbar leads me to conclude that you are using a personal computer where you do have administrator access. You could, therefore, install Firefox, which works on the following operating systems: [1]

• Windows 98
• Windows ME
• Windows NT 4.0
• Windows 2000
• Windows XP

Firefox also works on Vista, Linux, and Mac, but you would not be running one of those. —Remember the dot ^(talk) 02:38, 30 September 2007 (UTC)

I edit from home and work, both on Windows 2000, so that rules IE7 out. At home, I do have Firefox, but it is not my main browser; it is a true memory hog and have to restart the broser every half hour or so (with 512 MB!). I find the message derogatory in so far that IE6 users are made to feel inferior,and asumes upgrading is available to all. And no, I don't think IE4 and 5 should be supported; there is of course a limit. However, IE6 is far from dead, thus a major factor to deal with. And I certainly feel that Wikipedia should not be used for these kind of messages; Wikipedia != Microsoft/Mozilla's advertising platform. — Edokter • Talk • 12:26, 30 September 2007 (UTC)

Have you tried Opera?

I've updated the message in my user space to be more NPOV. —Remember the dot ^(talk) 18:39, 30 September 2007 (UTC)

W00T!

I think I cracked it! Solution was to make the img within the span display:block. Please test. — Edokter • Talk • 20:53, 30 September 2007 (UTC)

Doesn't that insert a line break after each image? (I'll have to test it later, sorry) In the meantime, please try Opera and let me know if that works for you. —Remember the dot ^(talk) 22:46, 30 September 2007 (UTC)

No, display is fine; the image inside the span is displayed as a block... But it introduces a new problem (Bill Gates is so dead!): I can't click images anymore! Some help please?

And why has my background suddenly turned green??? Dammit, you scared me good with that one :) — Edokter • Talk • 23:35, 30 September 2007 (UTC)

(edit conflict) That was me [2] trying to make it clear whether or not the transparency fix was actually working. I don't think that Internet Explorer allows links to function on block elements, which is in all likelihood the problem with your latest code. Is Opera working for you, at least? —Remember the dot ^(talk) 23:41, 30 September 2007 (UTC)

I don't think that's the case; there are loads of image button scripts that use display:block, but also have a hidden span with text to allow the lick to work... — Edokter • Talk • 10:22, 1 October 2007 (UTC)

Any combination of block or inline-block disables the links. Back to square one... (notice that you can click the borders though.) — Edokter • Talk • 00:17, 1 October 2007 (UTC)

By the way, inline-block alone shouldn't do it because that's what the currently live code uses. Anyway, have you had any luck with Opera? —Remember the dot ^(talk) 01:02, 1 October 2007 (UTC)

I'm not going to install Opera; I already have Firefoxx when I need it. And I know the current code uses inline-block, but that uses a span, not an img. Only if both my span and img are set to inline (which is default), do they become clickable. This should be fixable. — Edokter • Talk • 08:29, 1 October 2007 (UTC)

Okay. Just don't complain that you "can't" use a different browser, then. —Remember the dot ^(talk) 15:42, 1 October 2007 (UTC)

I'm trying to fix code here, not discuss what browser to use, that is not relevant here; this is IE6 code after all. In any way, I found this, which describes the problem, but does not give a solution. — Edokter • Talk • 17:04, 1 October 2007 (UTC)

I give up...

For now :)

I did make some changes to your code that will:

1. make it faster for border-less images
2. works on imagemaps (as they don't use border)
3. works for onclick images (idem)
4. doesn't need .thumbborder class

That would solve my biggest grudges, If you could just test this code (current revision), I propose we update the current code with these changes.

Hey, good idea to combine the two methods! I redid your code with the same concept, making a couple of changes to help ensure that it will work correctly, and also to make sure that the conventional 4 space indent is consistently used. Here is my revised code:

function PngFix()
{
    if (document.body.filters)
    {
        var documentImages = document.images
        for (var i = 0; i < documentImages.length;)
        {
            var img = documentImages[i]
            var imgSrc = img.src
            if (imgSrc.substr(imgSrc.length - 3).toLowerCase() == "png")
            {
                if (img.currentStyle.borderStyle == "none")
                {
                    img.style.filter = "progid:DXImageTransform.Microsoft.AlphaImageLoader(src='" + encodeURI(imgSrc) + "')"
                    img.src = "http://upload.wikimedia.org/wikipedia/commons/d/db/Must_left-click_image_again_before_saving.gif"
                    i++
                }
                else
                {
                    var outerSpan = document.createElement("span")
                    var innerSpan = document.createElement("span")
                    var outerSpanStyle = outerSpan.style
                    var innerSpanStyle = innerSpan.style
                    var imgStyle = img.currentStyle
                    
                    outerSpan.id = img.id
                    outerSpan.className = img.className
                    outerSpan.title = img.title
                    outerSpanStyle.borderWidth = imgStyle.borderWidth
                    outerSpanStyle.borderStyle = imgStyle.borderStyle
                    outerSpanStyle.borderColor = imgStyle.borderColor
                    outerSpanStyle.display = "inline-block"
                    outerSpanStyle.fontSize = "0"
                    outerSpanStyle.verticalAlign = "middle"
                    if (img.parentElement.href) outerSpanStyle.cursor = "hand"
                    
                    innerSpanStyle.width = img.width + "px"
                    innerSpanStyle.height = img.height + "px"
                    innerSpanStyle.display = "inline-block"
                    innerSpanStyle.filter = "progid:DXImageTransform.Microsoft.AlphaImageLoader(src='" + encodeURI(imgSrc) + "')"
                    
                    outerSpan.appendChild(innerSpan)
                    img.parentNode.replaceChild(outerSpan, img)
                }
            }
            else
            {
                i++
            }
        }
    }
}

That really was a good idea. I hope this code will satisfy both of us in the end :-) —Remember the dot ^(talk) 04:03, 2 October 2007 (UTC)

Just a couple of points:

1. Your border check still triggers the span code when the border-style not none, but the border-width is zero; thus no border. So that should be:
   if (img.currentStyle.borderWidth == "0px") || (img.currentStyle.borderStyle == "none")
2. outerSpanStyle.cssText = img.style.cssText relies on the specially defined .thumbborder class in common.css, copying the border attributes in my code doesn't.
3. var outer/innerSpanStyle really aren't necessary (runtime overhead).

Otherwise, I'm satisfied :) — Edokter • Talk • 08:55, 2 October 2007 (UTC)

I reverted all of your changes. I considered doing the things you recommended, but intentionally chose not to them. Please allow me to explain why.

First, in order to check whether the border has width or not, you also have to consider cases such as "0em", "0ex", "0pt", "0", "00px", "000px", etc. In the end you'd have to parse the border with parseInt(img.currentStyle.borderWidth) == 0. This adds quite a bit of extra processing, especially since it includes parsing. Since all the images we're dealing with turn off the border through the border-style property, it's faster and more direct to just check the border-style and ignore border-width.

Second, in my opinion it is best to not flatten class information by writing it all out to the element. This just does not seem very stable. It deviates from the original XHTML too much. A better solution would be to ask the developers to make .thumbborder apply to all elements, not just img elements (bugzilla:11549). That would make the modifications to common.css unnecessary.

Second, the IE + JavaScript Performance Recommendations recommend caching object pointers such as innerSpan.style and outerSpan.style (see "Cache Variables Whenever Possible").

Finally, removing the braces from the else statement and flattening it to a single line, else i++, might save a couple milliseconds of processing but it makes the code harder to read and consequently harder to modify or adapt in the future.

—Remember the dot ^(talk) 03:20, 3 October 2007 (UTC)

OK, you convinced me for most of your points, but I have to disagree about the class information; I don't see what's not 'stable' about it. Plus, style.cssText does not contain cascaded style information, so having to rely on .thumbborder, or any other class for that matter, is way more unpredictable then setting the border from currentStyle instead, especially considering how many chages are made to Common.css. So that's the only chage I've made this time (cached and all). But other then that, it is working??? — Edokter • Talk • 10:37, 3 October 2007 (UTC)

Well, for one thing the .thumbborder class could theoretically contain any CSS property, not just border ones. That means that in order to ensure that future changes to .thumbborder (whether by changing monobook's main.css or using a different skin) will work, the entire img.currentStyle.cssText would have to be copied into outerSpan.style.cssText. This makes the issue much more significant. Imaging that a user adds a script, or a script is added to common.js, that dynamically changes the class of certain images. Because all the class information would have been flattened onto outerSpan.style, changes to outerSpan.className wouldn't be able to modify properties defined by the original class.

To me, it would be best to just wait until the developers fix bugzilla:11549 and then we can delete the duplicate .thumbborder information from common.css. That would be the best and most flexible solution. —Remember the dot ^(talk) 01:21, 4 October 2007 (UTC)

Actually, changing outerSpan.className would immediately reflect in currentStyle. You keep asuming that simply copying className and style.cssText applies the entire styling information; that is not true, unless every class is declared independently. That is never going to happen, so I'm afraid you 'bug' will never be fixed. Any other custom script will undoubtetly contain classes assigned to elements that will not migrate to our span for the same reason: .thumbborder does not pass it's style information, simply because a span is not an img. What you call a bug is actually the proper behaviour for CSS; you cannot depend on className and style.cssText alone; you need to look at currentStyle. You cannot expect that every other script to be changed in order to eliminate img subclassing, just so that this script works. The script should not depend on any custom class declarations at all.

The span only serves one purpose at his point: simulating the border. So unless other image classes start to include other properties, then it may be time to look into other methods. (The only possible properties that might in the future be important are margin and padding). For now, this method is the safest way to ensure images behave as intended. With that in mind, I think this code is ready to go live. Remember that nothing is permanent and can always be changed later should the need arise. — Edokter • Talk • 11:19, 4 October 2007 (UTC)

Fine, so long as all CSS information is flattened. The border isn't the only important piece of CSS that the user's skin may define.

Ultimately, there's going to be problems either way, and I would much prefer modifying monobook's main.css (nothing but images are going to use .thumbborder, so restricting it to img elements is a bit silly). But what we have right now is fine if you really want it. —Remember the dot ^(talk) 05:19, 5 October 2007 (UTC)

(Deindent) Of course that would be better, and img.currentStyle.cssText was the first thing I tried, but alas, the currentStyle object does not support cssText. So we have to take whatever property we need. I am going to make it live today and post on VP technical to be on the lookout for errors. — Edokter • Talk • 08:48, 5 October 2007 (UTC)