Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

In support-legacy-browser(), default threshold should not override browser minimum versions #1524

Closed
wants to merge 2 commits into from

5 participants

@JohnAlbin

By default, support-legacy-browser(ie, "7") returns true because IE 7 browser usage is above the $critical-usage-threshold. That’s by design. Works for me!

However, if I specify $browser-minimum-versions: ('ie': '8'), then support-legacy-browser(ie, "7") should return false. We are explicitly saying we want IE8 and higher only. Unfortunately, support-legacy-browser(ie, "7") still returns true.

Take this file for example: http://sassmeister.com/gist/e0aa37ca8d480d078ca5

.test {
  @if support-legacy-browser(ie, "7") {
    content: 'ie7';
  }
  @else {
    content: 'ie8 and higher';
  }
}

No matter what we set $browser-minimum-versions to, the 'ie7' line is always output.

There's actually a few lines of CSS in compass-core that are broken because of this bug.

@JohnAlbin JohnAlbin referenced this pull request in JohnAlbin/normalize-scss
Closed

Upgrade to Compass 1.0.0's support and vertical-rhythm modules #18

@JohnAlbin

The 4e50cc7 commit is the minimal commit to fix the bug, but I just looked at the code again and 3e48300 makes the code easier to understand and minimally faster.

Assuming this is the proper fix, let me know which commit you would prefer to use and whether you'd like me to rebase the whole branch. :-)

@JohnAlbin

I decided to rebase this as I just realized that the tests for #1520 are dependent on this fix.

@timkelty

@JohnAlbin do you know the current status here?

I'm currently using your patch and would be great if it could be merged in. support-legacy-browser is pretty broken without it.

@JohnAlbin

So I've looked at the code here and I see a larger problem. My patch was based on the assumption that this was a problem of global $support-legacy-browser vs. global $critical-usage-threshold, but it's not. The problem is the way local $[feature]-threshoold interacts with the 2 globals, for example $svg-gradient-shim-threshold.

Current priority of settings in Compass:
1. $[feature]-threshold (which is set to $critical-usage-threshold by default)
2. global $support-legacy-browser
Note: that $critical-usage-threshold is never checked in the current code.

Priority of settings after this patch:
1. $[feature]-threshold (if not equal to $critical-usage-threshold)
2. global $support-legacy-browser
3. $critical-usage-threshold

I've pushed a new commit. But I think I broke some tests, so I need to fix that.

@JohnAlbin

hmm… that's not quite right either.

@JohnAlbin

I pushed another commit since I had messed up the logic in that previous one.

@JohnAlbin JohnAlbin changed the title from In support-legacy-browser(), threshold should not override browser minimum versions to In support-legacy-browser(), default threshold should not override browser minimum versions
@JohnAlbin

Added a test.

@chriseppstein

@JohnAlbin If I understand correctly, I don't consider this a bug. The minimum browser versions list does not exclude browsers, it includes browsers that would have otherwise been excluded by the threshold. If you want to exclude some version of IE, just adjust your threshold higher than the % of users using that version and below. If that excludes some other browsers that you don't want to exclude you add them to the minimum browser list. the logic is an OR between the threshold and the minimum support... not an AND.

@timkelty

@chriseppstein Your explanation makes sense to me, but after playing around a bit, I am decidedly lost as to how to effectively use the new support features.

Example: Suppose I know I need to support IE9 and up. So I start with:

$debug-browser-support: true;
$browser-minimum-versions: (
  "ie": "9"
);

@import "compass";

.foo {
  @include opacity(.5);
}

Which gives me this output:

.foo {
  /* Content for ie 8.
  Min version: 9.
  User threshold to keep: 0.1%. If ie 8 are omitted: 4.46163%. */
  filter: progid:DXImageTransform.Microsoft.Alpha(Opacity=50);
  opacity: 0.5;
}

So here, I don't want the filter stuff, as I don't care about IE8. After your last comment, I at least understand why it is still showing up. So, seemingly the only way I can omit the IE8 filter property is to set $graceful-usage-threshold: 5; (or $opacity-usage-threshold).

So, how is one expected to actually decide what these numbers should be? Surely I can't to go through each capability and see where my threshold should be when really what I care about is supporting a specific minimum browser version.

I can see the potential usefulness of usage based thresholds, but right now I can't see how to reasonably support a specific browser/version minimum.

Am I totally missing the boat? @JohnAlbin's changes seem to make more sense to me...

@chriseppstein

@timkelty you're saying that you support IE 9 in your config. You're NOT saying that you don't support IE 8. If IE 8 is 4.5% of your users, you've decided that you're comfortable excluding 4.5% of users. So yes, you should set your threshold above that

To add that logic, I'd want to create another configuration setting like $unsupported-browsers: (ie: "<8");

Personally, I don't see enough reason to add this, It adds a lot of complexity for a thing that is achievable with our existing approach.

@timkelty

@chriseppstein Ok, I think it's beginning to make a bit more sense now.

So given a project where I know I only need support for IE 9 and up, does this sound right?

$critical-usage-threshold: 4.46163;
$graceful-usage-threshold: 4.46163;
$browser-minimum-versions: (
  "ie": "9"
);
@chriseppstein

@timkelty That threshold draws a line between IE 8 and IE 9, you don't need to declare IE 9 in your minimum versions, it's implicit. However that threshold also excludes old versions of android (~2.3%) and any opera specific output. So you can use minimum versions to add support for things that would be otherwise rejected. Personally, I would never set the critical usage threshold higher than about 0.1%-0.5%. There's just no reason to break someone's web experience to save a few extra bytes in your output. You're saying that for any given feature you're ok with 1 in 20 users having a broken experience, given that there's a lot of varying features, the actual broken experience across all users would probably end up being 1 in 10 or so.

@timkelty

Yep, I get i didn't need $browser-minimum-versions, I just included it to be explicit, in the event that my thresholds changed.

And I get that I probably wouldn't want to set $critical-usage-threshold anywhere near that high. But does setting the $graceful-usage-threshold to that value seem reasonable if the main division I'm focussing on is IE 8/9?

I guess the mental hurdle I'm having (at least in this opacity mixin case) is that I'm getting IE8 specific output in my output, which I know I explicitly don't want, but the only way to get rid of it is to raise threshold to this magic number, which dropping support for other browsers as it raises.

Consider a fairly common setup that uses IE conditional comments and serves up 2 stylesheets: app.ie-lt10.scss for IE 9 and below and app.scss for everything else. They both just import the same master stylesheet, but declare their browser support vars first. How would you suggest setting things up so that app.scss doesn't get any lte IE9 rules, but app.ie-lt10.scss does?

Thanks again for the help!

@cimmanon

What's so special about IE9 that it can't be served the same CSS as IE10? I get the need to serve IE8 something different because it doesn't support media queries, but treating IE9 the same way doesn't make any sense to me.

@timkelty

@cimmanon You're right, MQ fallbacks for IE8 is the main reason for serving separate files, but the question remains the same whether it is app.ie-lt9.scss or app.ie-lt10.scss.

@chriseppstein

Here's how you can see what the usage threshold is for any particular browser version. https://gist.github.com/chriseppstein/db821931dc50b624c84c

To achieve what you're asking, you'll need to set the threshold to that value and then set minimum versions for everything else that you want to keep.

https://gist.github.com/chriseppstein/66b89cac2c6e08fc8ce4 is a modified version of the above gist that only outputs browsers versions that would be rejected so that you can easily see if there's any unintended consequences.

@chriseppstein

The difference is that IE9 is 10x the number of users that IE8 is.

@timkelty

So is omitted-usage("ie", "9"), which gives me 4.88544, adding the omitted users from all IE versions up to and not including 9? Trying to compare with numbers from https://raw.githubusercontent.com/Compass/compass/master/core/data/caniuse.json and it doesn't seem like they match up.

So would I do $graceful-usage-threshold: omitted-usage("ie", "9"); and then add minimum versions for anything that got missed?

@alexisg

If you really need to target your browsers you can also set your threshold to 100 and then simply add all the browsers which you need to support with $browser-minimum-versions. This would be similar to how some use autoprefixer with compass.

@chriseppstein

An idea that I'm entertaining for post 1.0:

$browser-minimum-versions: (
  "ie": ">= 9"
);

Which would remove ie 8 and below from threshold totals.

@timkelty

Thanks all.

I now realize what I'm asking for is technically possible with the current implementation, it's just a far jump if you're looking to recreate the simplicity of $legacy-support-for-ie8: false.

I'm mostly curious how people would end up tweaking to a desired threshold beyond the defaults. In my experience, support and compatibility is usually outlined pretty specifically in web proposals, especially with problematic browsers. Taking that knowledge and translating it into usage stats feels funny. And going back and forth between bumping a threshold and inspecting my generated css to see what gets included seems laborious.

The problem I'm having is likely a rapidly vanishing one as the browser landscape becomes more homogenous. But, I don't see the notion of clearly outlined browser minimums being replaced by a usage stat minimum in our proposals any time soon.

Related:
Are we able to use any of the "era" tags, e.g. e-2 for "2 versions back"?

@JohnAlbin

Okay, I understand how the system is currently designed. Thanks, Chris and everyone for the explanation!

And, since I can always set the threshold to 100 and set all the browsers, I guess that will work for me. However…

An idea that I'm entertaining for post 1.0:
$browser-minimum-versions: (
"ie": ">= 9"
);

How is that any different than this very pull request? Except with more complicated syntax. I'd argue that the more complicated syntax is not needed at all. Here's my logic:

Under the currently designed system, if a developer sets a value on some element in $browser-minimum-versions they have done so for 2 reasons:
1. They are aware of the threshold value and want to add a browser that does not meet that threshold.
2. They are aware of the threshold value, know that a particular browser meets that threshold, but want to ensure that it will always be supported even when the usage stats change later.

In both of those cases, the developer, when setting 'ie': '5', is implicitly saying “screw you, IE 4!" because they are explicitly adding IE5 support. If they wanted IE4 support, they would have used '4' instead.

So given they don't care about IE4, why not let this simple syntax $browser-minimum-versions: ( 'ie': '5' ); remove IE4 support?

@timkelty

you're saying that you support IE 9 in your config. You're NOT saying that you don't support IE 8.

I agree with @JohnAlbin, to me it seems to make sense that in any case where you'd declare a supported version, you're implicitly declaring that you're not supporting versions below that.

@chriseppstein

Are we able to use any of the "era" tags, e.g. e-2 for "2 versions back"?

This is not supported, but it could be, in theory, in a future release. Can you file a separate issue to track that?

How is that any different than this very pull request? Except with more complicated syntax.
So given they don't care about X, why not let this simple syntax remove support for X.

Well, 1.0 is now a release candidate, and I'm not willing to hold the release to make this change. So the extra syntax would be required to maintain backwards compatibility in a future release.

Furthermore, the way I would implement this exclusionary approach is by not including the excluded browsers in the total browser stats so that they are not included in any threshold check. So this patch is not how I would handle this change. The total code change would be much larger, but also more consistently applied.

In my experience, support and compatibility is usually outlined pretty specifically in web proposals, especially with problematic browsers. Taking that knowledge and translating it into usage stats feels funny.

I don't see the notion of clearly outlined browser minimums being replaced by a usage stat minimum in our proposals any time soon.

The way you're doing it sounds much harder and quickly obsolete. It also doesn't fit with every single conversation I've ever had about browser support. In every case, we looked at usage stats, decided what was a threshold we were comfortable with and then mapped those to supported browser versions. Sometimes we carved out exclusions when there was an outsized amount of effort associated with a particular version or versions. I hope compass can encourage people to start thinking about this problem more holistically (which is what the threshold approach does), especially now that browsers are releasing more regularly -- thresholds as a standard are the most logical way to craft support contracts.

In any respect, you can set your threshold to 100% and your declared browser minimums will be driving all support decisions.

@timkelty

Thanks @chriseppstein, made an issue here: #1761

I agree I don't want anything to hold off 1.0, and I'm fine using any of the techniques mentioned here in the meantime.

I also think you're right that defining support by version is becoming quickly obsolete, and I'm going to use this to push those who write our proposals to try and define things in terms of stats instead.

@chriseppstein

Tracking this issue here: #1762

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 2, 2014
  1. @JohnAlbin

    In support-legacy-browser(), $critical-usage-threshold should not ove…

    JohnAlbin authored
    …rride $browser-minimum-versions. Fixes #1524
  2. @JohnAlbin

    Add test for #1524.

    JohnAlbin authored
This page is out of date. Refresh to see the latest.
View
37 core/stylesheets/compass/_support.scss
@@ -99,12 +99,17 @@ $css-sel2-support-threshold: $critical-usage-threshold !default;
}
// Check agaist usage stats and declared minimums
$min-required-version: map-get($browser-minimum-versions, $browser);
+ $supported-browser: ($min-required-version and
+ compare-browser-versions($browser, $max-version or $min-version, $min-required-version) >= 0);
+ // If a minimum required version is specified and the default threshold is
+ // used, we only check the declared browser minimums.
+ @if ($threshold == $critical-usage-threshold) and $min-required-version {
+ @return $supported-browser;
+ }
$usage: if($max-version,
omitted-usage($browser, $min-version, $max-version),
omitted-usage($browser, $min-version));
- @return $usage > $threshold or
- ($min-required-version and
- compare-browser-versions($browser, $max-version or $min-version, $min-required-version) >= 0);
+ @return $usage > $threshold or $supported-browser;
}
// Include content for a legacy browser
@@ -123,17 +128,23 @@ $css-sel2-support-threshold: $critical-usage-threshold !default;
@include with-browser-ranges(intersect-browser-ranges($current-browser-versions, $ranges)) {
@content;
}
- } @else if $debug-browser-support and browser-out-of-scope($browser, $max-version) {
- /* Content for #{display-browser-range($browser, $min-version, $max-version)} omitted.
- Not allowed in the current scope: #{browser-out-of-scope($browser, $max-version)} */
- } @else if $debug-browser-support and not
- support-legacy-browser($browser, $min-version, $max-version, $threshold) {
- @if omitted-usage($browser, $min-version, $max-version) > $threshold {
- /* Content for #{display-browser-range($browser, $min-version, $max-version)} omitted.
- User threshold to keep: #{$threshold}%. If #{display-browser-range($browser, $min-version, $max-version)} and below are omitted: #{omitted-usage($browser, $min-version, $max-version)}%. */
- } @else {
+ } @else if $debug-browser-support {
+ @if browser-out-of-scope($browser, $max-version) {
/* Content for #{display-browser-range($browser, $min-version, $max-version)} omitted.
- Minimum support is #{map-get($browser-minimum-versions, $browser)}. */
+ Not allowed in the current scope: #{browser-out-of-scope($browser, $max-version)} */
+ } @else if not support-legacy-browser($browser, $min-version, $max-version, $threshold) {
+ // Check agaist usage stats and declared minimums
+ $min-required-version: map-get($browser-minimum-versions, $browser);
+ $usage: if($max-version,
+ omitted-usage($browser, $min-version, $max-version),
+ omitted-usage($browser, $min-version));
+ @if ($threshold == $critical-usage-threshold) and $min-required-version or $usage <= $threshold {
+ /* Content for #{display-browser-range($browser, $min-version, $max-version)} omitted.
+ Minimum support is #{$min-required-version}. */
+ } @else {
+ /* Content for #{display-browser-range($browser, $min-version, $max-version)} omitted.
+ User threshold to keep: #{$threshold}%. If #{display-browser-range($browser, $min-version, $max-version)} and below are omitted: #{$usage}%. */
+ }
}
}
}
View
5 core/test/integrations/projects/compass/css/gradients.css
@@ -303,3 +303,8 @@
background-image: -moz-linear-gradient(#dddddd, #aaaaaa);
background-image: -webkit-linear-gradient(#dddddd, #aaaaaa);
background-image: linear-gradient(#dddddd, #aaaaaa); }
+
+.issue-1524-has-no-svg-because-of-minimums {
+ background-image: -moz-linear-gradient(#dddddd, #aaaaaa);
+ background-image: -webkit-linear-gradient(#dddddd, #aaaaaa);
+ background-image: linear-gradient(#dddddd, #aaaaaa); }
View
4 core/test/integrations/projects/compass/sass/gradients.sass
@@ -179,3 +179,7 @@ $browser-minimum-versions: ('ie': '9')
.issue-1676-has-svg-because-of-minimums
+background-image(linear-gradient(#ddd, #aaa))
+$svg-gradient-shim-threshold: $critical-usage-threshold
+$browser-minimum-versions: ('ie': '10')
+.issue-1524-has-no-svg-because-of-minimums
+ +background-image(linear-gradient(#ddd, #aaa))
Something went wrong with that request. Please try again.