Bug Tracker

Ticket #4295 (closed bug: patchwelcome)

Opened 5 years ago

Last modified 2 years ago

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:
Blocking: Blocked by:

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

index.html Download (556 bytes) - added by dantman 5 years ago.
Testcase file
getstyle.closure.js Download (4.0 KB) - added by dantman 5 years ago.
Patch making use of a closure
rawCss.js Download (1.5 KB) - added by dantman 5 years ago.
Patch making use of a new jQuery.rawCss

Change History

Changed 5 years ago by dantman

Testcase file

comment:1 follow-up: ↓ 4 Changed 5 years ago by 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.

Changed 5 years ago by dantman

Patch making use of a closure

Changed 5 years ago by dantman

Patch making use of a new jQuery.rawCss

comment:2 Changed 5 years ago by dantman

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.

comment:3 follow-up: ↓ 5 Changed 5 years ago by 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.

comment:4 in reply to: ↑ 1 Changed 5 years ago by dantman

Replying to 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.

comment:5 in reply to: ↑ 3 Changed 5 years ago by dantman

Replying to 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.

comment:6 Changed 5 years ago by brandon

#2462 relates to this

comment:7 Changed 5 years ago by brandon

  • Status changed from new to closed
  • Resolution set to wontfix

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.

comment:8 Changed 5 years ago by dantman

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.

comment:9 Changed 5 years ago by brandon

  • Status changed from closed to reopened
  • Resolution wontfix deleted

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.

comment:10 Changed 5 years ago by brandon

  • Owner set to brandon
  • Status changed from reopened to new

comment:11 Changed 5 years ago by brandon

  • Status changed from new to assigned

comment:12 Changed 5 years ago by dantman

.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

comment:13 Changed 5 years ago by dmethvin

See also #4481.

comment:14 Changed 5 years ago by andras

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.

comment:15 Changed 4 years ago by dmethvin

#5749 is a duplicate of this ticket.

comment:16 Changed 3 years ago by dmethvin

  • Status changed from assigned to closed
  • Resolution set to patchwelcome

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.

Note: See TracTickets for help on using tickets.