Opened 13 years ago

Closed 7 years ago

#1441 closed defect (wontfix)

[patch] [cla] ContentPane: fixPathsInCssText issues in MSIE

Reported by: robert.coup@… Owned by: Sam Foster
Priority: low Milestone: future
Component: Dojox Version: 0.3
Keywords: Cc: robert.coup@…
Blocked By: Blocking:

Description (last modified by bill)

MSIE 5.5/6 use the filter/AlphaImageLoader hack to work around their lack of alpha-PNG support. There are three issues with MSIE & dojo.html.fixPathsInCssText - this addresses two of them, and leaves the third as a question:

  • When the compressor is run over dojo.style.fixPathsInCssText the function iefixPathsInCssText() should use the rejexTrim, url, etc variables. Problem is that the compressor has renamed them in the outer function to something like _a12 and so you get missing object errors. While this may be a compressor 'feature', it's fixed in the attached patch.
  • Valid syntax for the AlphaImageLoader? filter includes:
    filter: AlphaImageLoader(src='123.png');
    filter: AlphaImageLoader(src='123.png', sizingMethod='crop');
    filter: AlphaImageLoader(sizingMethod='crop', src='123.png');
    
    The third case above is not handled by the rejexIe expression. This is fixed in the attached patch.
  • AlphaImageLoader? paths are relative to the document/page location, while normal css url() paths are relative to the stylesheet location. Yay for MSIE. Anyway if one uses intern-strings then the paths get broken, because fixPathsInCssText adds the path from the src=' ' to the absolute uri to the stylesheet. The contents of the path src tags aren't related to the stylesheet uri though! Example:
    http://host/
      hostdir/
        page/  <-- Page is served from here when not using compressed JS
          js/
            dojo/   <-- djConfig.baseScriptUri points to here
              dojo.js
          widgets/
            my.css
            images/
              img1.png
          images/
            img2.png
    
    my.css:
      ...
      background-image: url('images/img1.png');
      filter:AlphaImageLoader(src='widgets/images/img1.png');
      ...
      background-image: url('../images/img2.png');
      filter:AlphaImageLoader(src='images/img2.png');
      ...
    
    Paths get fixed to:
      background-image: url('http://host/hostdir/page/widgets/images/img1.png');
      filter:AlphaImageLoader(src='http://host/hostdir/page/widgets/widgets/images/img1.png');
    
      background-image: url('http://host/hostdir/page/images/img2.png');
      filter:AlphaImageLoader(src='http://host/hostdir/page/widgets/images/img2.png');
    
    I'm not sure what to do in this case, apart from access the widget's templateCssPath: dojo.uri.dojoUri('../../widgets/my.css'); parameter and using that to figure out which bits to ignore and which not to. Any brighter ideas?

Attachments (2)

fixPathsInCssText_1.diff (2.2 KB) - added by guest 13 years ago.
Fixes issues #1 and #2 above
fixPathsInCssText_2.diff (2.4 KB) - added by robert.coup@… 13 years ago.
Fix for issue 3 as noted above.

Download all attachments as: .zip

Change History (27)

Changed 13 years ago by guest

Attachment: fixPathsInCssText_1.diff added

Fixes issues #1 and #2 above

comment:1 Changed 13 years ago by dylan

Milestone: 0.4
Owner: changed from anonymous to Bryan Forbes
Summary: fixPathsInCssText issues in MSIE[patch] fixPathsInCssText issues in MSIE
Version: 0.40.3

comment:2 Changed 13 years ago by bill

Cc: robert.coup@… added
Milestone: 0.40.4.1

Robert, we'd like to look at your patch but first you need to sign an ICLA: www.dojotoolkit.org/icla.txt

Thanks!

comment:3 Changed 13 years ago by robert.coup@…

Its been in for ages (Sep 8).

comment:4 Changed 13 years ago by robert.coup@…

Another patch is attached... this is a solution for issue 3 above. Not a nice one by any stretch but it does work. Basically, you declare your filter like this:

_filter:progid:DXImageTransform.Microsoft.AlphaImageLoader(sizingMethod='scale', src='js/otm/project/widgets/templates/images/panel/navi_shadow.png', cssPathOffset='../../../../../');

where the proper CSS for other browsers is:

background-image: url('images/panel/navi_shadow.png');

and the css file is located at: js/otm/project/widgets/templates/mywidget.css

The cssPathOffset is the relative path from the CSS file to the base HTML page.

In fixPathsInCssText the code looks for the cssPathOffset attribute and alters the path to be correct when its loading the style sheet and fixing the image paths, so the AlphaImageLoader? ends up with a path relative to the HTML page, not the CSS file.

Changed 13 years ago by robert.coup@…

Attachment: fixPathsInCssText_2.diff added

Fix for issue 3 as noted above.

comment:5 Changed 13 years ago by bill

Owner: changed from Bryan Forbes to mumme
Summary: [patch] fixPathsInCssText issues in MSIE[patch] [cla needed] fixPathsInCssText issues in MSIE

Robert, we don't have your CLA. Please send it in again. Maybe it got lost or something, not sure.

comment:6 Changed 13 years ago by bill

Summary: [patch] [cla needed] fixPathsInCssText issues in MSIE[patch] [cla] fixPathsInCssText issues in MSIE

Ah, you sent in a CCLA, for your company (OneTrackMind?). Sorry, didn't notice it before. (In the future please note in bug reports that there's a CCLA filed for your company, and list the company name)

comment:7 Changed 12 years ago by mumme

Status: newassigned

First of all, thank you for the patches!

I been looking at them for a couple of days, and the first 2 is pretty straitforward.

The third one however is a bit harder to decide on, I contacted the contributer for the AlphaImageLoader? thing, Morris Johns, I would like to hear his original intension with the path fix.

If the intension was to make the path relative to the cssfile, I hope it was but one can never be certain, then I have a patch makes it work without the offsetPath='../wherever'

You can view a examples here: http://mumme.se/iePngtestBuilt.html http://mumme.se/iePngtestUnbuilt.html

So I'm kind of holding this patch until I hear how the original intension was in the first place

/ Fredrik

comment:8 Changed 12 years ago by robert.coup@…

Yeah, its a bit of a hack presently but I didn't see any nice way to get the CSS path with respect to the html page (dojo js, yes).

A nicer solution would be much more preferrable.

comment:9 Changed 12 years ago by robert.coup@…

After testing it, your change for the 3rd issue works for me and is a bunch nicer than mine ... thanks :)

For the archives: Using Fredrik's solution you should specify AlphaImageLoader? paths with respect to the CSS file, not with respect to the page. When the CSS file is loaded by dojo (either directly or via a build) the paths get fixed to be relative to the page.

eg. Normally:

.mydiv-normalbrowsers {
  background-image: url('images/mydiv.png');
}
.mydiv-ie {
  filter: progid:DXImageTransform.Microsoft.AlphaImageLoader(src='js/acme/widgets/templates/images/mydiv.png', sizingMethod='scale');
}

Using Fredrik's solution:

.mydiv-normalbrowsers {
  background-image: url('images/mydiv.png');
}
.mydiv-ie {
  filter: progid:DXImageTransform.Microsoft.AlphaImageLoader(src='images/mydiv.png', sizingMethod='scale');
}

comment:10 Changed 12 years ago by mumme

Milestone: 0.4.10.5

After discussing this with Bill we decided to punt it for 0.5 release, to get more thoughts and opinions of the problem.

the patch in question is: (Roberts patch slightly Modified)

Index: src/html/style.js
===================================================================
--- src/html/style.js   (revision 6368)
+++ src/html/style.js   (working copy)
@@ -467,27 +467,26 @@
        //      summary
        // usage: cssText comes from dojoroot/src/widget/templates/Foobar.css
        //      it has .dojoFoo { background-image: url(images/bar.png);} then uri should point to dojoroot/src/widget/templates/
-       function iefixPathsInCssText() {
-               var regexIe = /AlphaImageLoader(src=['"]([	sw()/.\'"-:#=&?~]*)['"]/;
+       if(!cssStr || !URI){ return; }
+       var match, str = "", url = "", urlChrs = "[\t\s\w\(\)\/\.\\'"-:#=&?~]+";
+       var regex = new RegExp('url\(\s*('+urlChrs+')\s*\)');
+       var regexProtocol = /(file|https?|ftps?):///;
+       regexTrim = new RegExp("^[\s]*(['"]?)("+urlChrs+")\1[\s]*?$");
+       if (dojo.render.html.ie55 || dojo.render.html.ie60) {
+               var regexIe = new RegExp("AlphaImageLoader\((.*)src=['"]("+urlChrs+")['"]");
+               var rootDir = (new dojo.uri.Uri(location, URI).toString());
                while(match = regexIe.exec(cssStr)){
-                       url = match[1].replace(regexTrim, "$2");
+                       url = match[2].replace(regexTrim, "$2");
                        if(!regexProtocol.exec(url)){
-                               url = (new dojo.uri.Uri(URI, url).toString());
+                               url = (new dojo.uri.Uri(rootDir, url));
                        }
-                       str += cssStr.substring(0, match.index) + "AlphaImageLoader(src='" + url + "'";
+                       str += cssStr.substring(0, match.index) + "AlphaImageLoader(" + match[1] + "src='" + url + "'";
                        cssStr = cssStr.substr(match.index + match[0].length);
                }
-               return str + cssStr;
+               cssStr = str + cssStr;
+               str = "";
        }

-       if(!cssStr || !URI){ return; }
-       var match, str = "", url = "";
-       var regex = /url(s*([	sw()/.\'"-:#=&?]+)s*)/;
-       var regexProtocol = /(file|https?|ftps?):///;
-       var regexTrim = /^[s]*(['"]?)([w()/.\'"-:#=&?]*)1[s]*?$/;
-       if (dojo.render.html.ie55 || dojo.render.html.ie60) {
-               cssStr = iefixPathsInCssText();
-       }
        while(match = regex.exec(cssStr)){
                url = match[1].replace(regexTrim, "$2");
                if(!regexProtocol.exec(url)){

comment:11 Changed 12 years ago by robert.coup@…

Would be good if we can get the 1st and 2nd parts committed - since the current behaviour is b0rked and doesn't work and all the fixes do is make it act correctly.

The 3rd part is an enhancement of sorts and invites discussion.

comment:12 Changed 12 years ago by mumme

[quot] Would be good if we can get the 1st and 2nd parts committed - since the current behaviour is b0rked and doesn't work and all the fixes do is make it act correctly.

The 3rd part is an enhancement of sorts and invites discussion. [end quot]

Of course, sorry for my dense head... checkout rev 6435, should adress 1 and 2

/ Fredrik

comment:13 Changed 12 years ago by bill

I talked to Morris about issue #3; I think he hadn't realized that AlphaImageLoader?()'s src attribute is relative to the currently displayed page (rather than relative to the CSS file, or to the WEB_ROOT, like you would expect). So he doesn't mind you changing it to the patch Fredrik suggested (or even removing it altogether).

comment:14 Changed 12 years ago by robert.coup@…

+1 for Fredrik's patch - it does exactly whats required and works well across development and built versions, and even when dojo and widgets are loaded from another domain.

/ Rob

comment:15 Changed 12 years ago by mumme

Perhaps we should remove it, move it out to a separate function or at least make it optional by some param or something The reason is that this function isn't used solely by the Widget templates, ContentPane? uses is also (among others), I can see it beeing considered buggy if the css pulled in by ContentPane? needs path adjustments relative to file vs page (as one would expect).

/ Fredrik

comment:16 Changed 12 years ago by morris

Perhaps we should remove this feature. Ideally the solution would be to generate the alphaImageLoader filter from the background rule. However because alphaimageloader doesnt support all the different background options (offset, repeat etc) it would have to be optional.

comment:17 Changed 12 years ago by robert.coup@…

I think we need it in there, otherwise peoples widgets will break if they use PNGs solely from broken paths. If it needs to be disabled for ContentPane? then thats fine, but I don't think taking it out actually helps - people will just need to hack another solution.

comment:18 Changed 12 years ago by bill

Component: StyleDojox
Milestone: 0.91.0
Summary: [patch] [cla] fixPathsInCssText issues in MSIE[patch] [cla] ContentPane: fixPathsInCssText issues in MSIE

fixPathsInCssText() won't be part of dijit. I think we said we were getting rid of it altogether? Or did we say it would be in the super ConentPane? in dojox?

comment:19 Changed 12 years ago by guest

Well, all dijit css is now in dijit.css/tundra.css. However, nobody's really answered what to do about CSS for custom widgets or the widgets in dojox - see thread on d.contrib.

comment:20 Changed 12 years ago by mumme

(In [9850]) Cleaned up Regex/string munging for easier maintainence, be faster. Add support for declaring media type on style/link nodes. Add support for path adjustment on AlphaImageLoader? statements in html/css. (not external css files) bugfix leave [script type=dojo/method] as is.

Added several regression tests for these and other features. refs: #3594, refs #1441

comment:21 Changed 12 years ago by Adam Peller

Milestone: 1.01.1

mumme says he hasn't closed this because of some issue with the dojox ContentPane?. Moving to 1.1 for consideration

comment:22 Changed 11 years ago by bill

Milestone: 1.21.3
Owner: changed from mumme to Sam Foster

Sam can you take a look at these? Hopefully these are fixed by your refactor or you can fix them

comment:23 Changed 10 years ago by bill

Description: modified (diff)
Milestone: 1.3future

This code is in dojox/html/_base.js now.

comment:24 Changed 7 years ago by ben hockey

Keywords: needsreview added
Priority: highlow

i'm trying to identify stale tickets. if there is a need to keep this ticket open, please replace the "needsreview" keyword with "reviewed". if there is no need to keep this ticket open then please close it.

comment:25 Changed 7 years ago by bill

Keywords: needsreview removed
Resolution: wontfix
Status: assignedclosed

Too bad this ticket got abandoned, but now with IE6 and IE7 on their last legs, presumably we won't be addressing any more issues with AlphaImageLoader.

Note: See TracTickets for help on using tickets.