Skip to content
Permalink
Browse files

Fix #10639. Make percent-specified margins return px values in WebKit.

  • Loading branch information...
mikesherov authored and dmethvin committed Dec 7, 2011
1 parent 2c75a99 commit 7f6a991313380b74d5fb18782fb6b99fd6c4a22d
Showing with 27 additions and 3 deletions.
  1. +13 −2 src/css.js
  2. +7 −1 src/support.js
  3. +7 −0 test/unit/css.js
@@ -7,6 +7,7 @@ var ralpha = /alpha\([^)]*\)/i,
rnumpx = /^-?\d+(?:px)?$/i,
rnumnopx = /^-?\d+(?!px)[^\d\s]+$/i,
rrelNum = /^([\-+])=([\-+.\de]+)/,
rmargin = /^margin/,

cssShow = { position: "absolute", visibility: "hidden", display: "block" },
cssWidth = [ "Left", "Right" ],
@@ -256,7 +257,7 @@ jQuery(function() {

if ( document.defaultView && document.defaultView.getComputedStyle ) {
getComputedStyle = function( elem, name ) {
var ret, defaultView, computedStyle;
var ret, defaultView, computedStyle, width, style = elem.style;

name = name.replace( rupper, "-$1" ).toLowerCase();

@@ -268,6 +269,16 @@ if ( document.defaultView && document.defaultView.getComputedStyle ) {
}
}

// A tribute to the "awesome hack by Dean Edwards"
// WebKit uses "computed value (percentage if specified)" instead of "used value" for margins
// which is against the CSSOM draft spec: http://dev.w3.org/csswg/cssom/#resolved-values
if ( !jQuery.support.pixelMargin && computedStyle && rmargin.test( name ) && rnumnopx.test( ret ) ) {
width = style.width;
style.width = ret;
ret = computedStyle.width;
style.width = width;
}

return ret;
};
}
@@ -299,7 +310,7 @@ if ( document.documentElement.currentStyle ) {
if ( rsLeft ) {
elem.runtimeStyle.left = elem.currentStyle.left;
}
style.left = name === "fontSize" ? "1em" : ( ret || 0 );
style.left = name === "fontSize" ? "1em" : ret;
ret = style.pixelLeft + "px";

// Revert the changed values
@@ -91,7 +91,8 @@ jQuery.support = (function() {
noCloneEvent: true,
inlineBlockNeedsLayout: false,
shrinkWrapBlocks: false,
reliableMarginRight: true
reliableMarginRight: true,
pixelMargin: true
};

// Make sure checked status is properly cloned
@@ -277,6 +278,11 @@ jQuery.support = (function() {
offsetSupport.subtractsBorderForOverflowNotVisible = ( inner.offsetTop === -5 );
offsetSupport.doesNotIncludeMarginInBodyOffset = ( body.offsetTop !== conMarginTop );

if( window.getComputedStyle ) {
div.style.marginTop = "1%";
support.pixelMargin = ( window.getComputedStyle( div, null ) || { marginTop: 0 } ).marginTop !== "1%";
}

body.removeChild( container );
div = container = null;

@@ -542,4 +542,11 @@ test("Do not append px to 'fill-opacity' #9548", 1, function() {
equal( jQuery(this).css("fill-opacity"), 1, "Do not append px to 'fill-opacity'");
});

});

test("outerWidth(true) and css('margin') returning % instead of px in Webkit, see #10639", function() {
var container = jQuery( "<div/>" ).width(400).appendTo( "#qunit-fixture" ),
el = jQuery( "<div/>" ).css({ width: "50%", marginRight: "50%" }).appendTo( container );

equal( el.outerWidth(true), 400, "outerWidth(true) and css('margin') returning % instead of px in Webkit, see #10639" );
});

16 comments on commit 7f6a991

@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 28, 2011

This issue also affects top, bottom, left, right, and text-indent. There may be others. From what I could gather it's related to WebKet using "computed value" instead of "used value." According to the MDN article on getComputedStyle it's supposed to use used values but it appears that WebKit is returning computed values instead. The MDN article explains that it's ok to return percentages for the computed style in a few cases. Not surprisingly, those are the same cases that WebKit returns percentages.

The fix that was committed will fix the majority of cases if it wasn't filtered specifically to the margin property. It won't fix top and bottom because they are relative to the innerHeight of the parent.

I wrote a blog article discussing a more robust fix for this issue. I hope this is helpful. The jQuery bug tracker thinks I'm spamming you but I think this is useful information.

@mikesherov

This comment has been minimized.

Copy link
Member Author

mikesherov replied Dec 28, 2011

@heygrady, thanks for the input. Please read the full text of: https://bugs.webkit.org/show_bug.cgi?id=73334 and look at the CSSOM spec that is referenced in the ticket. I believe that webkit does the correct behavior now for getComputedStyle according to CSSOM.

In regards to the jquery bug tracker, just fill out the captcha :-).

@mikesherov

This comment has been minimized.

Copy link
Member Author

mikesherov replied Dec 28, 2011

@heygrady, I look at your article, and your fix is certainly more robust. However, to include the complete fix would require a lot of bytes for what amounts to an edge case. It also should be fixed in latest webkit, if not, let me know and I'll submit a bgu report to webkit, or you could even do it yourself to help #movethewebforward.

@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 28, 2011

I'm not against doing it myself. I may. I would recommend taking your exact fix and removing the check for which property returned the percentage. Like I mentioned above, the vast majority of cases can be fixed EXACTLY as the margin case. The only exceptions I've found are top and left. I've also got some fixes for some other less common stuff but that isn't strictly necessary.

    if ( !jQuery.support.pixelMargin && computedStyle && rnumnopx.test( ret ) ) {
      if (/^top|bottom$/.test(name)) {
        // top and bottom are relative to the innerHeight of the parent
        ret = $(elem.parentNode||elem).innerHeight() * parseFloat(ret) / 100;
      } else {
          // otherwise the width is usually the exact same
          width = style.width;
          style.width = ret;
          ret = computedStyle.width;
          style.width = width;
      }
    }

I'll take a look at porting my code into jQuery because it would actually be much smaller if my code relied on jQuery.

@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 28, 2011

Also, the bug tracker said Akismet didn't like all of my links to the MDN articles and then everything crashed out and showed me Python errors. It's probably time to move the jQuery tracker to GitHub :)

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron replied Dec 28, 2011

We recently had a barrage of spam on the tracker - its content was "links to other stuff", so it looks like akismet is being aggressive towards tickets that look similar

@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 28, 2011

@rwldrn It's probably not the right time to mention that I have some really cheap, super authentic Viagra pills for sale.

@rwaldron

This comment has been minimized.

Copy link
Member

rwaldron replied Dec 28, 2011

Holding out on us, huh?

@mikesherov

This comment has been minimized.

Copy link
Member Author

mikesherov replied Dec 28, 2011

@heygrady, that code looks great, however it is much slower. It requires you to do a regex on every css lookup. It also relies on innerHeight, which is a lookup of 4 css values (all of which would have to do a regex). I'm going to generate a jsperf to see the effect. Remember, each iteration counts here as .css() is used heavily in loops for animation.

@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 28, 2011

It's not running on EVERY call to $.css, only those that !jQuery.support.pixelMargin. Your code is actually already executing a regex for every call to determine if a non-px unit was returned. In the case of top/bottom, innerHeight looks things up, but only in WebKit when a percentage was returned -- and then it's only when top and bottom were returned. In that case, it's probably better to return the correct result instead of a percentage. If you're worried about the inline regex I added, move that to a var like jQuery does for all regex (I didn't want to complicate my comment by introducing variables).

@mikesherov

This comment has been minimized.

Copy link
Member Author

mikesherov replied Dec 29, 2011

@heygrady, no, it's calling it every time a BROWSER doesn't support pixel-only margins. If the browser fails that test, it then makes sure you're looking for a margin which is a much lighter regex. Although, you may be right that it's neglible. A perf would tell.

Aside from that though, this would need a separate support test, because pixelMargin isn't gauranteed to also do the "wrong" thing for top and left. According to CSSOM, top and left should use computed value (which is a percent if specified).

@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 29, 2011

That's fair enough. Interpretation of the CSSOM is the conversation going on in the WebKit bugs. In some ways that's irrelevant here because we're trying to make $.css return the used value instead of the computed value. It's equally irrelevant if WebKit is already doing things 100% correctly. My testing showed that among the most recent stable versions of IE, Safari, Firefox and Chrome on Windows (as of Christmas 2011) the only browser that was returning percentages was WebKit (Safari/Chrome). The margin bug in the WebKit tracker is supposedly fixed in the dev version of WebKit but there's still an open issue for the positioning properties. It's also unclear based on the Mozilla documentation if the margin bug should've been fixed independent of the other bugs based on the Mozilla definitions of used value and current value -- it seems like the bugs are highly related.

Thankfully we're working in JavaScript and don't need to get tangled up in complicated standards arguments quite as deeply. The expected behavior is the $.css function will return a px unit (unless it's a string like font-family or something). That's how this margin bug got reported to jQuery in the first place.

Anway:

  • WebKit returns percentage in some cases (top, bottom, left, right, margin, text-indent) as of Chrome 16 and Safari 5.1 (This is a bigger deal in the cases of Mobile WebKit and Safari since those don't get updated nearly as often as Chrome)
  • WebKit and Opera returns auto in some cases when an illegal value was set, like setting top: 1em on a position: static element.
  • Mozilla returns specified value when an illegal value was set, like setting top: 1em on a position: static element.
  • IE9 seems to work as expected (hooray)
  • IE8 and below only return specified value so we have to get really hacky and use the special pixel properties (and jQuery already does a fine job fixing this but I would personally change it slightly to always use a pixel property if it exists).
@heygrady

This comment has been minimized.

Copy link

heygrady replied Dec 29, 2011

Also, reading more carefully, you and I seem to be agreeing 100%.

Here's a test that should cover both cases

if( window.getComputedStyle ) {
  // @see https://bugs.webkit.org/show_bug.cgi?id=29084
  // @see https://bugs.webkit.org/show_bug.cgi?id=73334
  div.style.position = "relative";
  div.style.top = "1%";
  support.getUsedValue = ( window.getComputedStyle( div, null ) || { top: 0 } ).top !== "1%";
}
@mikesherov

This comment has been minimized.

Copy link
Member Author

mikesherov replied Dec 29, 2011

Yeah, I think we do. The question remains, however, as it always does, what priority speed, complexity, maintainability, correctness, and size have in this situation. Certainly there's a fix there, but should we also petition www-style to include top,left,bottom,right in the same category as margin et al? That would go a long way to getting parity natively in browsers.

@heygrady

This comment has been minimized.

Copy link

heygrady replied Jan 4, 2012

I've forked the jQuery source and made some changes. It's passing all of the existing tests and I haven't added any new tests yet. It doesn't add very much to the code in the end and could probably be made slightly smaller. Anyway, I haven't done any speed tests either.

https://github.com/heygrady/jquery

@mikesherov

This comment has been minimized.

Copy link
Member Author

mikesherov replied Jan 4, 2012

@heygrady, you should read the contribution guidelines located at https://github.com/jquery/jquery so you can submit a proper pull request if you're interested in making this change. Looks promising.

Please sign in to comment.
You can’t perform that action at this time.