Posted by espirates on March 24, 2009 at 7:38am
| Project: | External Links |
| Version: | 6.x-1.11 |
| Component: | Code |
| Category: | bug report |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
span.ext Property zoom doesn't exist : 1
span.mailto Property zoom doesn't exist : 1
Comments
#1
Thanks espirates, these were added in #132614: strange graphic placement when link wraps line as an attempt to fix the issue there. However, I'm not entirely sure they're really necessary. The "zoom" property was suggested early as a possible fix, but then we ended up using a different approach in the end. Now it's likely that those properties aren't actually doing anything beneficial at all. We probably just need some browser testing to confirm that they are safe to remove.
#2
I removed them and have not experienced any issues, everything seems to be working correctly.
#3
I've removed the lines and everything works.
#4
I've removed the lines from the CSS and committed the changes.
#5
Automatically closed -- issue fixed for 2 weeks with no activity.
#6
The invalid CSS seems to have reappeared in 6.x-1.9. I'm seeing the same CSS validation errors again after updating to this version.
UPDATE
I realized after removing the "zoom" property that the icons stop showing up in IE when the property isn't there. None of the standard valid "hasLayout" triggers seem to work here without causing problems in other browsers. My temporary solution, which allows the CSS to validate and still shows the icons in IE, is to remove the "zoom" property, then add a line of javascript (using jQuery) to trigger layout in IE7 and lower:
if ($.browser.msie && $.browser.version <= 7.0) $('span.ext, span.mailto').css('display', 'inline-block');This is a bit of an ugly hack, but then so is Internet Explorer.
#7
I stopped supporting ie browser ions ago, if their not using a modern browser that works then foo on them :))
#8
As much as I wish it were, I don't think that not supporting IE is a reasonable option right now. Something like 60% of web users are using IE, and more than half of them are using IE 7 or less.
In any case, the bug in the module is not that it doesn't support IE, but rather that it relies on invalid CSS in order to support IE.
If it's a reasonable solution, I can take a shot at writing a patch that:
1. removes the invalid CSS
2. adds a line to extlink.js that isolates IE 7 and below and sets a CSS property to trigger "hasLayout"
This seems like a bit of a hack to me, though, so I wanted to run it by the maintainers first. There may be a better solution.
#9
I'd be happy to include an IE6/7 JS-only solution to extlink rather than relying on the "zoom" property in CSS. I just want to make sure that it actually works in all browsers this time, since we removed zoom in the 1.8 version at the request of multiple users who claimed that it did not cause an issue, then we quickly then realized it was not safe to just remove it and re-added the zoom in 1.9. The fix you posted in #6 looks pretty good, except that $.browser.msie doesn't exist any more in new versions of jQuery.
#10
This can also be solved by (untested):
<?phpdrupal_set_html_head('<!--[if IE]><style type="text/css" media="all">span.ext,span.mailto{zoom:1}</style><![endif]-->');
?>
Still a hack, but a better looking one, IMHO.
EDIT: typo
#11
Subscribing.
#12
I have to correct myself, the $.browser property still exists even in the latest jQuery (1.4.2), but it's merely deprecated. I took the approach recommended #6 and turned it into this patch. It uses $.support.boxModel if it is available (as in Drupal 7) or uses $.browser.msie if it's not available (as in Drupal 6).
I've tested this in every browser under the sun (Safari, Opera (mac/win), Firefox (mac/win), IE 6/7/8, and Chrome (mac/win) and it seems to work perfectly and equally across all of them.
I've committed this patch to the D6 and D7 branches.
#13
Automatically closed -- issue fixed for 2 weeks with no activity.
#14
Hi guys,
Applying inline CSS to the external link span requires you to use the CSS property !important within your website's stylesheet to override the display:inline-block; placed in there by jQuery.
It would be better to have a class within the extlink.css file that had the property 'display: inline-block' - this way the jQuery could apply the class to the span rather than an inline style for IE7 and below.
#15
@sydneyshan The method you've described does not work in IE6, which does not understand "inline-block". See #132614: strange graphic placement when link wraps line.
#16
Hi quicksketch,
I'm suggesting instead of placing inline styles on the span tag to fix the bug, apply an extra class to the span tag that has a corresponding class in the extlink.css file. The jQuery in extlink.js should only apply the extra class to IE7.
I hope that makes sense.
#17
The problem with this approach is many users complained about using invalid CSS ("zoom") within the CSS file, which would no longer validate. If you can think of any 100% compatible CSS that can be placed within the existing CSS file I'm open to it, but this has been going on for over 2 years with no good solution.
#18
What's wrong with the solution proposed in #10?
Surely, it's a hackery solution, but the non-standard "zoom" property is a much more ugly and harmful hack, IMHO.
Anyone tested that?
#19
Let's open a new issue to discuss any further changes. The "zoom" property is gone and the current solution is 100% validation approved. The only downside is you can't change the "display" CSS attribute in IE6 (not that I'm sure why you'd want to anyway, since the icon doesn't look right if you do this). I think this is pretty much a non-issue at this point.