Skip to main content

Bug Tracker

Side navigation

#4295 closed bug (patchwelcome)

Opened March 04, 2009 11:54PM UTC

Closed March 25, 2011 06:05PM UTC

Last modified March 14, 2012 02:01PM UTC

Output of .css(name); for combined properties like margin and padding is inconsistent across browsers and how the style is set

Reported by: dantman Owned by: brandon
Priority: major Milestone: 1.4
Component: core Version: 1.3.2
Keywords: Cc:
Blocked by: Blocking:
Description

Combined css properties like .css('margin'); and .css('padding'); are fairly inconsistent across browsers and how the style is set.

The primary issue is when you set something like margin inside of a style tag instead of directly in the style attribute.

The html test case is attached, as well I have uploaded it, and the output from Firefox3, Midori (WebKit browser), and Opera 9.64 which I tested with:

An overview of the results:

  • Firefox outputs "t r b l" form whenever it has the data, while midori and opera output the shortened combined value form.
  • Firefox and Midori both do not report margin/padding when defined in a style tag instead of on .style; However they do report marginLeft (style was defined like #a { margin: ...; })
  • Opera is the only one that reports margin/padding when defined in a style tag.

This issue should be fixable by intercepting calls to combined css properties, and instead grabbing the top, right, bottom, left values and combining them into the final result.

Combined css values effected should be: /^(margin|padding|(border|outline)-(style|color|width))$/

All those are ones that use "t r b l". I haven't checked, but theoretically /border-(top|right|bottom|left)/ may have issues as well, similarly for some other combined properties. But that's a different project that would likely involve cataloging the standard on what each combined css property should output.

Attachments (3)
  • getstyle.closure.js (4.0 KB) - added by dantman March 05, 2009 03:02AM UTC.

    Patch making use of a closure

  • index.html (0.5 KB) - added by dantman March 04, 2009 11:55PM UTC.

    Testcase file

  • rawCss.js (1.5 KB) - added by dantman March 05, 2009 03:03AM UTC.

    Patch making use of a new jQuery.rawCss

Change History (17)

Changed March 05, 2009 12:36AM UTC by dmethvin comment:1

If you think margin and padding are bad, take a look at border.

:-)

What is the use-case for this string? It seems that in most cases, the caller will get a string that they'll need to parse to obtain the four values if they need to examine it further. It seems more useful to return a Javascript object with four properties so the caller wouldn't have to parse the string that jQuery just built.

Changed March 05, 2009 03:17AM UTC by dantman comment:2

I've attached two possible fixes for this bug.

Because fixing this means introducing the ability to run a quick query for the value of a single css value without any of the float or opacity fixes there are two options. The getstyle.closure method basically just moves the raw css grabbing stuff into a quick closure. The rawCss method moves the raw grabbing of a style value into a jQuery.rawCss method which can be reused outside of jQuery.

The getStyle bit is similar to what I did earlier today when I experimented with the possibility of making .css extensible so that compatibility improvements can even be added by plugins. However I do like the reuseability of that .rawCss function, so I'd probably recommend that patch.

Changed March 05, 2009 03:29AM UTC by dmethvin comment:3

Messing with this a year or two ago, in FF (at least) when border-right-style is "none", border-right-width can still return a width. Is that the desired behavior, or should it return the actual visible border-right-width of 0 based on "none"?

I'd still like to see how typical client code is improved when using these new properties.

Changed March 05, 2009 03:31AM UTC by dantman comment:4

Replying to [comment:1 dmethvin]:

If you think margin and padding are bad, take a look at border.
:-)
What is the use-case for this string? It seems that in most cases, the caller will get a string that they'll need to parse to obtain the four values if they need to examine it further. It seems more useful to return a Javascript object with four properties so the caller wouldn't have to parse the string that jQuery just built.

This bug was just for fixing the inconsistency of some of the properties. A use case here is as simple as $(node).css('margin', $(anotherNode).css('margin'));

If this bug isn't fixed that will break unexpectedly unless margin is directly set on the .style of the node. And jQuery doesn't make returning multiple keys at once easy at all, $(node).css([key1, key2, ...]); is my proposed syntax for that (returns an object which as a bonus can be fed right back to .css to reset those values after you've changed them), but that's for another report.

I to do like the idea of returning objects, as well has handling them. In our framework at work I have a number of those. margin supports input as [t, r, b, l] or {t: ???, right: ???, horiz: ???, ...}, and there's even a size: that supports [w, h] and {w: ???, height: ???}; But all these are something for another report.

Just as a closing note, if we do want object like stuff in the future something simple like this is possible:

return {

top: jQuery.rawCss( ..., ...Top..., ... ),

...

toArray: function() {return [this.top, this.right, this.bottom, this.left]; },

toString: function() {return this.toArray().join(' ');}

};

Throw in a few wrappers to common string functions like .split and voila you have a fancy object variant that still works in most use cases.

Changed March 05, 2009 04:05AM UTC by dantman comment:5

Replying to [comment:3 dmethvin]:

Messing with this a year or two ago, in FF (at least) when border-right-style is "none", border-right-width can still return a width. Is that the desired behavior, or should it return the actual visible border-right-width of 0 based on "none"? I'd still like to see how typical client code is improved when using these new properties.

borderLeftWidth returning an actual value is consistent across Firefox, Midori (WebKit), and Opera when set directly, but if set in a style tag they do return 0px. Opera also returns something like "1px #ff0000" completely omitting the "none" unlike the rest of the browsers.

The tricky part here is if you change a directly set 1px into 0px invisibly then just pass that back in a setter you invisibly set that 1px to 0px and if style is set to a style later the border no longer reappears as it would.

But there is room to think about making those follow the standard.

Changed March 17, 2009 09:48PM UTC by brandon comment:6

#2462 relates to this

Changed March 17, 2009 09:51PM UTC by brandon comment:7

resolution: → wontfix
status: newclosed

The amount of code/overhead is not worth fixing this when accessing the property directly works just fine. You'd also have to start including fixes for other shorthands like background, border-radius, etc.

Changed March 17, 2009 10:30PM UTC by dantman comment:8

No, "accessing the property directly" does not work just fine. While it is a shorthand, simple properties like margin, padding, and so on are used more than the individual css values.

The logical way for someone to copy the margins of one node to the other would be:

$(otherNode).css('margin', $(oldNode).css('margin'));

However if someone attempts that when the css is defined in the stylesheet in any browser other than Opera (read: browser incompatibility) then they end up confused as to why the css is not being copied over.

To go and properly copy a margin one would need to do something like:

$(otherNode).css('margin', [$(oldNode).css('marginTop'), $(oldNode).css('marginRight'), $(oldNode).css('marginBottom'), $(oldNode).css('marginLeft')].join(' '));

Trying to do something like $(oldNode).css('margin', $(oldNode).css('marginTop')); is not correct because it's perfectly valid (and quite normal) for margins/padding/etc... to be different for each edge.

background in itself is quite complex, I doubt anyone really expects it to work that well. As for borderRadius, jQuery doesn't handle stuff that doesn't work in all browsers it supports (conversely this is a fix that makes things work properly in all browsers when they don't work normally), so that's a separate issue entirely. borderRadius is something that would be handled in a compatibility plugin, I already have code for one, and it already handles the shorthand form better than jQuery handles margin/padding shorthands.

This is fixing of native and commonly used css properties that aren't working as people would expect.

Changed March 22, 2009 03:37PM UTC by brandon comment:9

resolution: wontfix
status: closedreopened

I have seen other requests that jQuery core better support the shorthand version of these basic CSS properties.

However, I believe we should return an object instead of an Array. This will satisfy the proposed use-case of copying margin styles from one element to another in one line.

 $('#div1').css( $('#div2').css('margin') ); 

In regards to the ability to retrieve multiple css values at once I propose we use a space separated string as we do elsewhere within jQuery core.

I still believe the better way to solve this issue is via a plugin. That probably means we need to make the .css() method extendable.

Changed March 22, 2009 03:38PM UTC by brandon comment:10

owner: → brandon
status: reopenednew

Changed March 22, 2009 03:38PM UTC by brandon comment:11

status: newassigned

Changed March 22, 2009 10:16PM UTC by dantman comment:12

.css is still defined as a method of interacting with css styles so I believe we should not be mangling that definition by returning an object instead of a string.

If you Array note is because of the use of an array in my code, that is merely done to make use of .join(' '); I recall there were performance hits with to many uses of + on strings, I don't know the point where it really becomes an issue but the array+join technique was clean for coding.

Compromise: If you still want 'margin' to return something that can be programmability used with object data, we can do something like this:

...
var top = jQuery.rawCSS(...), right = ..., bottom = .......;
var s = [ top, right, bottom, left ].join(' ');
s.top = top; s.right = right; s.bottom = bottom; s.left = left;
return s;

This compromise will still return a string like style methods are supposed to, but that string will also have individual top/right/bottom/left properties on it that can be accessed without parsing the string.

Note that this does not affect the ability to throw this back to .css in any way because jQuery doesn't parse these and I haven't found any browser incompatibilities in just setting the normal shorthands (I could double check if you feel it necessary).

I'm all for making compatibility plugins. Making .css extendable is something I have already been working on. It's almost complete, it just needs to wait on John's refactor of the jQuery.attr and jQuery.curCSS methods because it's not possible to implement the extendibility API cleanly without that code cleanup.

See: http://groups.google.com/group/jquery-dev/browse_thread/thread/fc1873bfd64b1fc4/c54f1fc2759d7fd2?lnk=gst&q=.css+extensible#c54f1fc2759d7fd2

Changed August 08, 2009 02:02AM UTC by dmethvin comment:13

See also #4481.

Changed November 29, 2009 03:24AM UTC by andras comment:14

For this matters I would recommend to return the margin exactly as it is if there's white spacing in a margin/padding property value. For higher requirements, a plugin would be a good idea.

Changed November 14, 2010 04:12AM UTC by dmethvin comment:15

#5749 is a duplicate of this ticket.

Changed March 25, 2011 06:05PM UTC by dmethvin comment:16

resolution: → patchwelcome
status: assignedclosed

If someone wants to implement it as a cssHooks group that would be useful. I think there is a very large amount of code involved in getting this right, so it's unlikely to make its way into core, but it would be a useful plugin.