Skip to content

CSS: Make .css("width") & .css("height") return fractional values #2439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Jul 1, 2015

Fixes gh-1724

-9 bytes, most likely because the IE11 fullscreen workaround got way smaller.

Safari 8 returns 0 for gBCR().width for table columns and non-zero value for offsetWidth. Older versions don't exhibit this behavior.

As for #1724 (comment), I can reproduce the problem only on jsFiddle so it may require an iframe or even weirder setup, who knows. But browsers not supporting fractional values in gBCR differ in what to return anyway, e.g. Android Browser always rounds down, same with offsetWidth. I don't think we're going to get consistent data here.

IE 10-11 & Edge return non-decimal heights for children divs in dimensions tests so I had to round it. The value is really close but smaller than 100.

Tests pass on IE 9-11, Firefox, Chrome, Safari 6-8, Android 2.3-5.0 & iOS 6-8. Should I add more tests? Not sure what else can I check.

TODO:

  • Perf tests (jsPerf is down, any other service?)
  • compat version of the patch (they should be similar but IE8 needs special handling)

// Support: Safari 8+
// Table columns in Safari have non-zero offsetWidth & zero
// getBoundingClientRect().width unless display is changed.
// Support: IE <= 11 only
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked this is fixed in Edge.

@arthurvr
Copy link
Member

arthurvr commented Jul 2, 2015

Perf tests (jsPerf is down, any other service?)

Not anymore. Mathias just re-enabled it.

@mgol
Copy link
Member Author

mgol commented Jul 2, 2015

Not anymore. Mathias just re-enabled it.

It's still down for me.

@timmywil
Copy link
Member

timmywil commented Jul 6, 2015

LGTM, assuming you can also cherry-pick to compat and make necessary changes.

@markelog
Copy link
Member

markelog commented Jul 6, 2015

So Chrome and FF try this on offset* methods (methods that are commonly associated with ours) and revert that behaviour because it broke a lot of sites.

Now, i gather, if ppl need subpixels they use getBoundingClientRect which i suppose it is pretty low amount of users since basically one use DOM methods directly and because return values of that function is inconsistent depending on environment... and now our return values should be inconsistent too?

So this is:

  1. inconsistent
  2. it will break a lot of sites and plugins (as proved by the reverts of the browser vendors)
  3. most ppl do not need that.

But we want to do it anyway?

@mgol
Copy link
Member Author

mgol commented Jul 6, 2015

So Chrome and FF try this on offset* methods (methods that are commonly associated with ours) and revert that behaviour because it broke a lot of sites.

Site devs cannot choose to not update browsers of their users or defer the update. With libraries it's possible.

Also, as @bzbarsky said in #1724, offset* methods are deprecated, they just can't be dropped/changed due to Web compatibility. We can do it.

@mgol
Copy link
Member Author

mgol commented Jul 6, 2015

Since jsPerf is down, I've set up a simple page locally using Benchmark.js to test it (micro-benchmarks are not great but it's hard to measure otherwise):

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8">
    <title>A page for perf tests</title>
    <style>
        #test-div-container {
            width: 66.6px;
            height: 23.4px;
        }
        #test-div {
            width: 33%;
            height: 50%;
        }
    </style>
</head>
<body>
    <div id="test-div-container">
        <div id="test-div"></div>
    </div>
    <script src="http://code.jquery.com/jquery-git.js"></script>
    <script>
        var $j = jQuery.noConflict();
    </script>
    <script src="jquery.js"></script>
    <script src="benchmark.js"></script>
    <script>
        var suite = new Benchmark.Suite;

        var testDiv1 = $j('#test-div');
        var testDiv2 = $('#test-div');

        // add tests
        suite
            .add('width without fractions', function() {
                testDiv1.css('width');
            })
            .add('width with fractions', function() {
                testDiv2.css('width');
            })
            .add('height without fractions', function() {
                testDiv1.css('height');
            })
            .add('height with fractions', function() {
                testDiv2.css('height');
            })
            // add listeners
            .on('cycle', function(event) {
                console.log(String(event.target));
            })
            .on('complete', function() {
                window.s = this;
                console.log('Fastest is ' + this.filter('fastest').pluck('name'));
            })
            // run async
            .run({'async': true});
    </script>
</body>
</html>

Chrome 43:

width without fractions x 22,304 ops/sec ±2.71% (90 runs sampled)
test-perf.html:49 width with fractions x 18,546 ops/sec ±6.14% (86 runs sampled)
test-perf.html:49 height without fractions x 21,882 ops/sec ±2.57% (87 runs sampled)
test-perf.html:49 height with fractions x 18,710 ops/sec ±2.52% (88 runs sampled)

Firefox 39:

"width without fractions x 21,903 ops/sec ±1.03% (73 runs sampled)" test-perf.html:49:5
"width with fractions x 18,610 ops/sec ±2.48% (82 runs sampled)" test-perf.html:49:5
"height without fractions x 22,274 ops/sec ±1.80% (75 runs sampled)" test-perf.html:49:5
"height with fractions x 18,011 ops/sec ±2.04% (69 runs sampled)" test-perf.html:49:5

IE11:

width without fractions x 4,712 ops/sec ±2.16% (83 runs sampled)
width with fractions x 4,293 ops/sec ±1.67% (89 runs sampled)
height without fractions x 4,960 ops/sec ±1.17% (89 runs sampled)
height with fractions x 4,331 ops/sec ±1.33% (89 runs sampled)

Edge (build 10162):

width without fractions x 5,308 ops/sec ±4.35% (86 runs sampled)
width with fractions x 4,948 ops/sec ±1.70% (88 runs sampled)
height without fractions x 5,527 ops/sec ±1.29% (90 runs sampled)
height with fractions x 4,987 ops/sec ±1.46% (89 runs sampled)

Safari 8:

width without fractions x 18,064 ops/sec ±2.34% (82 runs sampled)
width with fractions x 16,453 ops/sec ±1.95% (88 runs sampled)
height without fractions x 17,670 ops/sec ±3.01% (88 runs sampled)
height with fractions x 16,771 ops/sec ±1.91% (92 runs sampled)

Overall, the perf penalty is at about 6-9% for IE/Edge/Safari & ~16% for Chrome/Firefox.

@markelog
Copy link
Member

markelog commented Jul 6, 2015

You didn't answer, why we want to do this? Because

For example if you have 3 items that are width 33% inside of a 100px element we're returning 33px for each element

So it will now work in Chrome but not in IE9? This will work in mobile Safari but not in Android browser? This is debug nightmare.

Site devs cannot choose to not update browsers of their users or defer the update. With libraries it's possible.

With this trajectory, it is easier not to update jQuery at all or use Zepto instead.

Also, as @bzbarsky said in #1724, offset* methods are deprecated, they just can't be dropped/changed due to Web compatibility. We can do it.

What? Do as they did? Deprecate our methods?

There is a lot of stuff deprecated in DOM API, users still use them. Even you know this because of one offhand comment, you expect people to know this too?

@markelog
Copy link
Member

markelog commented Jul 6, 2015

Overall, the perf penalty is at about 6-9% for IE/Edge/Safari & ~16% for Chrome/Firefox.

Number four to my list

@markelog
Copy link
Member

markelog commented Jul 6, 2015

Btw, i see no deprecation notice in http://dev.w3.org/csswg/cssom-view/#dom-htmlelement-offsetwidth or on mdn.io, i don't see "deprecation" word anywhere in #1724, i see only "Just don't use offset*." This no way near to "deprecation" decision.

@mgol
Copy link
Member Author

mgol commented Jul 6, 2015

Site devs cannot choose to not update browsers of their users or defer the update. With libraries it's possible.

With this trajectory, it is easier not to update jQuery at all or use Zepto instead.

I strongly disagree with such a conclusion. The fact that you upgrade the library yourself is a very strong difference to native APIs. Browsers can't fix a lot of native APIs since they'd break existing sites with no possibility of immediately fix all of them. With libraries it's crucial the developer choses the moment of the upgrade; if the upgrade breaks anything the dev might not deploy it immediately but fix the issues first. This is a huge difference.

For example if you have 3 items that are width 33% inside of a 100px element we're returning 33px for each element

So it will now work in Chrome but not in IE9? This will work in mobile Safari but not in Android browser? This is debug nightmare.

This is no different than what we currently have. Try http://output.jsbin.com/mugudi/ on Chrome & Android 2.3. Some browsers round down, some up. You're safe only if you use decimal values for everything.

Also, as @bzbarsky said in #1724, offset* methods are deprecated, they just can't be dropped/changed due to Web compatibility. We can do it.

What? Do as they did? Deprecate our methods?

No, we can change them.

@timmywil
Copy link
Member

timmywil commented Jul 6, 2015

@markelog To be fair, the issue does include a reason for doing this... #1724 (comment). For browsers that do support subpixel layout, our rounded values are unusable. Returning whatever the browser returns, even if inconsistent across browsers, will be consistent within each browser. In other words, for browsers that support subpixel layout, we can return subpixel values. For the ones that don't, we don't. This makes layout consistent across browsers.

@markelog
Copy link
Member

markelog commented Jul 6, 2015

This is a huge difference....

Right, so why do i need break anything? Between choosing not to break and break, why would user chose the latter?

No, we can change them.

You did get that i was sarcastic, right :-)?

In other words, for browsers that support subpixel layout, we can return subpixel values. For the ones that don't, we don't.

I see justification to not fix any browser bug ever, "if they don't we don't".

This is a big edge-case of small amount of users we are trying to fix here but this fix might be catastrophic to everyone else.

@mgol
Copy link
Member Author

mgol commented Jul 6, 2015

Migrate issue: jquery/jquery-migrate#112.

@mgol mgol removed the Needs review label Jul 6, 2015
@markelog
Copy link
Member

markelog commented Jul 6, 2015

Just for the good record keeping - we decided to land this and see responses from alpha users

@dmethvin
Copy link
Member

dmethvin commented Jul 6, 2015

I think landing this for the alpha makes sense. We don't have a good idea of how much users and plugins depend on these numbers being integers. If this breaks some plugins, the user always has the option to delay an upgrade until the plugin is fixed. If the plugin is abandoned and will never be fixed, they have bigger problems than a jQuery upgrade.

@mgol
Copy link
Member Author

mgol commented Jul 6, 2015

compat version of the PR: #2454.

@mgol
Copy link
Member Author

mgol commented Jul 7, 2015

If no one opposes, I'll merge it today.

@mgol mgol closed this in b60b26e Jul 7, 2015
@mgol mgol deleted the fractional-width branch July 7, 2015 16:13
mgol added a commit that referenced this pull request Jul 7, 2015
@markelog markelog mentioned this pull request Nov 16, 2015
@mgol mgol mentioned this pull request Jun 26, 2016
4 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

width(), height(), etc. shouldn't round
6 participants