Skip to content
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

backward-compatibility prefixing (and try to understand the gradient mixins) #13643

Closed
bassjobsen opened this issue May 21, 2014 · 16 comments
Closed
Labels
Milestone

Comments

@bassjobsen
Copy link
Contributor

Based on #13620 i compared the output with and without autoprefixing.

  1. i found some -webkit-boxprefixes missing in the current less code. For instance in nomalize.less the hr ruleset has no -webkit-boxprefix. How should this be fixed? It seems the following code could fix this:

hr { .box-sizing(content-box); height: 0; }

But the preceding code uses the deprecated code from vendor-prefixes.less. How should this be done?
I found the -webkit-boxprefix missing eight times. An other example is the bod-shadow for the kbd ruleset in code.less Again is fixing this by calling the .box-shadowmixin from vendor-prefixes.less allowed or not?

  1. the progress-bar-stripes in progress-bars.less is not prefixed. Suggested solution to add the following code to less/vendor-prefixes.less:
.keyframe(@name, @roules) {
  @-webkit-keyframes @name {
    @roules();
  }
  @-o-keyframes @name {
    @roules();
  }
  @keyframes @name {
    @roules();
  }
}

And in progress-bars.less call the above mixin with:

.keyframe(progress-bar-stripes, {
  from  { background-position: 40px 0; }
  to    { background-position: 0 0; }
});

This solution is based on: less/less.js#1640 by @wallaroo

  1. the Gradient mixins

I don't understand the following code:
background-image: -webkit-linear-gradient(left, color-stop(@start-color @start-percent), color-stop(@end-color @end-percent));

As i understand the color-stop() should be use with the -webkit-gradient prefix and not with -webkit-linear-gradient, also is not clear:
-why does it uses the color-stop() twice and not to()
-why does the .vertical mixins don't have this rule (but uses background-image: -webkit-linear-gradient(top, @start-color @start-percent, @end-color @end-percent);)

Running the current #gradient > .horizontal trough the autoprefixer generates a second : -webkit-linear-gradient similar with background-image: -webkit-linear-gradient(top, @start-color @start-percent, @end-color @end-percent);

which compiles into for instance:

.carousel-control.left {
  background-image: -webkit-linear-gradient(left, color-stop(rgba(0, 0, 0, .5) 0%), color-stop(rgba(0, 0, 0, .0001) 100%));
  background-image:      -o-linear-gradient(left, rgba(0, 0, 0, .5) 0%, rgba(0, 0, 0, .0001) 100%);
  background-image: -webkit-gradient(linear, left top, right top, from(rgba(0, 0, 0, .5)), to(rgba(0, 0, 0, .0001)));
  background-image: -webkit-linear-gradient(left, rgba(0, 0, 0, .5) 0%, rgba(0, 0, 0, .0001) 100%);
  background-image:         linear-gradient(to right, rgba(0, 0, 0, .5) 0%, rgba(0, 0, 0, .0001) 100%);
  filter: progid:DXImageTransform.Microsoft.gradient(startColorstr='#80000000', endColorstr='#00000000', GradientType=1);
  background-repeat: repeat-x;
}

In accordance with the autoprefixer rules i think the .horizontal mixins should look like that shown below:

  .horizontal(@start-color: #555; @end-color: #333; @start-percent: 0%; @end-percent: 100%) {
    background-image: -webkit-gradient(linear, left top, right top, from(@start-percent, @start-color), to(@end-percent,@end-color));
    background-image: -webkit-linear-gradient(left, @start-color @start-percent, @end-color @end-percent);  // Safari 5.1-6, Chrome 10+
    background-image: -o-linear-gradient(left, @start-color @start-percent, @end-color @end-percent); // Opera 12
    background-image: linear-gradient(to right, @start-color @start-percent, @end-color @end-percent); // Standard, IE10, Firefox 16+, Opera 12.10+, Safari 7+, Chrome 26+
    background-repeat: repeat-x;
    filter: e(%("progid:DXImageTransform.Microsoft.gradient(startColorstr='%d', endColorstr='%d', GradientType=1)",argb(@start-color),argb(@end-color))); // IE9 and down
  }

The above code has still one issue. With the default values of 0 and 100% auto prefixer adds a second -webkit-gradient prefix with no percentages (due to 0 and 100 are default) and doesn't remove the first -webkit-gradient in that case the compiled css will look like that shown below:

  background-image: -webkit-gradient(linear, left top, right top, from(0%, #555), to(100%, #333));
  background-image: -webkit-linear-gradient(left, #555 0%, #333 100%);
  background-image:      -o-linear-gradient(left, #555 0%, #333 100%);
  background-image: -webkit-gradient(linear, left top, right top, from(#555), to(#333));
  background-image:         linear-gradient(to right, #555 0%, #333 100%);
  filter: progid:DXImageTransform.Microsoft.gradient(startColorstr='#ff555555', endColorstr='#ff333333', GradientType=1);
  background-repeat: repeat-x;
@cvrebert
Copy link
Collaborator

My understanding is that we won't be adding any new calls to the vendor prefix mixins, as they're deprecated, and unnecessary with Autoprefixer. However, for backward compatibility, we won't remove any existing calls to the vendor prefix mixins until v4.

@cvrebert
Copy link
Collaborator

  1. the progress-bar-stripes in progress-bars.less is not prefixed

To clarify, do you mean that the final CSS (with Autoprefixer enabled) is still missing prefixes?
If yes, which browser versions require the missing prefixes?

@cvrebert
Copy link
Collaborator

(3) might be a legitimate bug. Please open a separate issue for it. Thanks.

@cvrebert
Copy link
Collaborator

normalize.less is simply a vendored version of https://github.com/necolas/normalize.css
Please file bugs regarding it in its own GitHub project issue tracker.

@hnrch02
Copy link
Collaborator

hnrch02 commented May 21, 2014

WebKit/Blink apparently doesn't need the change in box-sizing, see necolas/normalize.css#289.

@bassjobsen
Copy link
Contributor Author

@cvrebert thanks for your response. I will post a new issue for the gradient mixins.

For both point 1 and 2, i don't agree with your opinion, i think they are all "bugs" / "missings" which should be fixed before you can guarantee backward compatibility. This fixes are needed to give the current 3.1.1. the browser support as promised.

@hnrch02 the bootstrap team should chose here. Support the prefixes of autoprefixer or make a difference for normalize.css.

i think the process should be done in two steps:

  1. from the current version 3.1.1 rewrite all code which needs an prefix to use the mixins from less/vendor-prefixes.less
  2. introduce the autoprefixer after step 1) has been finished

After step 1 and 2 new code can be written add without prefixes (and prefixed with autoprefixer).
Personally i think you shouldn't introduce the autoprefixer before v4.

Creating a consistent less/vendor-prefixes.less also opens the option to generate that prefixes automatically instead of prefixing the final css.

I agree that using single line w3c property declarations only (and prefix the final css) will be more intuitive than using mixins from less/vendor-prefixes.less every where.

@bassjobsen
Copy link
Contributor Author

to illustration the current inconsistency:

See navbar.less ( both the latest develop version and 3.1.1 )

For the .navbar-collapse class the box-shadow is set with: box-shadow: inset 0 1px 0 rgba(255,255,255,.1); but for the .navbar-form class it is see with .box-shadow(@shadow);

the .navbar-collapse is not new (since 3.1.1+), so should backward compatible and use the .box-shadow() mixin!

The contribution notes now tell me: "Don't add vendor prefixed properties to their unprefixed counterparts (e.g., only box-sizing and not also include -webkit-box-sizing), as this is done automagically at build time."

What do you expect if someone writes a PR for navbar.less?

@bassjobsen
Copy link
Contributor Author

  1. the progress-bar-stripes in progress-bars.less is not prefixed

To clarify, do you mean that the final CSS (with Autoprefixer enabled) is still missing prefixes?
If yes, which browser versions require the missing prefixes?

The Autoprefixer fixes this issue and adds the @-webkit-keyframes and @-o-keyframes at rules. I'm not sure is the @-keyframes is part of the css 3 animations, see: http://caniuse.com/#feat=css-animation. If the @-keyframes is part of the css 3 animations all the current versions of chrome, opera and safari should got the -webkit prefix. v3.1.1. don't add this prefix which seems a bug / missing too. In the @-keyframes situation this is only theoretical maybe because of the animation works at least in chrome without any prefix for the @-keyframes .

@cvrebert
Copy link
Collaborator

the .navbar-collapse is not new (since 3.1.1+), so should backward compatible and use the .box-shadow() mixin!

The inconsistency is admittedly annoying, but as long as the pre-Autoprefixer version also didn't use a vendor prefix mixin, then it's still backward-compatible; in this particular example, regardless of whether you actually use the Autoprefixer or not, the browser compatibility of the v3.2.0 code is the same or greater than the old code.
Yes, the old code was probably buggy due to not including the prefixes.

@bassjobsen
Copy link
Contributor Author

Currently I found doing what is deprecated, using and extending the mixins in mixins/vendor-prefixes.less, will be the easiest way to quick fix my problem. See also: https://github.com/bassjobsen/Bootstrap-prefixed-Less. I understand that using w3c css only and using the autoprefixer is a better and far more clean solution, for now i'm able to update Bootstrap for my theme without too much worry.

@cvrebert
Copy link
Collaborator

Yup, that's probably your best option at this point.

@cvrebert
Copy link
Collaborator

@mdo Could you double-check whether our -webkit-linear-gradient syntax in #gradient > .horizontal (in /less/mixins/gradients.less) is correct? My little bit of investigation seems to suggest that Bass is right about that line of our CSS being incorrect.

@cvrebert cvrebert added this to the v3.2.0 milestone Jun 9, 2014
@cvrebert
Copy link
Collaborator

cvrebert commented Jun 9, 2014

@mdo Ping regarding ^^^

@mdo
Copy link
Member

mdo commented Jun 9, 2014

Yup, gradient syntax in the .horizontal() mixin was fubared. Fixed in master.

mdo added a commit that referenced this issue Jun 9, 2014
@mdo
Copy link
Member

mdo commented Jun 9, 2014

Anything else here needing immediate fixing? If so, perhaps we break out into separate issues and clarify anything else that needs addressing?

@cvrebert
Copy link
Collaborator

cvrebert commented Jun 9, 2014

IMHO, we're done here now that you've sorted out the gradient mixins.

@mdo mdo closed this as completed Jun 9, 2014
stempler pushed a commit to stempler/bootstrap that referenced this issue Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants