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

Carousel code partially doesn't use vendor-prefixes mixins #14937

Closed
komor72 opened this issue Oct 30, 2014 · 30 comments · Fixed by #15473
Closed

Carousel code partially doesn't use vendor-prefixes mixins #14937

komor72 opened this issue Oct 30, 2014 · 30 comments · Fixed by #15473
Milestone

Comments

@komor72
Copy link

komor72 commented Oct 30, 2014

https://github.com/twbs/bootstrap/blob/master/less/carousel.less#L30
and also in Sass variant:
https://github.com/twbs/bootstrap-sass/blob/master/assets/stylesheets/bootstrap/_carousel.scss#L30

The @media code from line 30 to 47 doesn't use vendor-prefixes mixin names but plain CSS attributes without prefix variants. This breaks Carousel transitions on my Safari 6.1 on OS X 10.7, in my code using Middleman and Bootstrap-Sass. After googling I'm pretty convinced I'm not the only victim. Curiously, the Carousel example from project's homepage works OK and CSS inspection shows me all vendor-specific declarations.

I've fixed this in my local gem copy, and now it works in my project. Sorry for Sass code instead of Less, but I work on Sass and verified my change on Sass version only:

    // WebKit CSS3 transforms for supported devices
    @media all and (transform-3d), (-webkit-transform-3d) {
      @include transition-transform (.6s ease-in-out);
      @include backface-visibility (hidden);
      @include perspective (1000);

      &.next,
      &.active.right {
        @include translate3d (100%, 0, 0);
        left: 0;
      }
      &.prev,
      &.active.left {
        @include translate3d(-100%, 0, 0);
        left: 0;
      }
      &.next.left,
      &.prev.right,
      &.active {
        @include translate3d(0, 0, 0);
        left: 0;
      }
    }
@juthilo
Copy link
Collaborator

juthilo commented Oct 30, 2014

// Vendor Prefixes
//
// All vendor mixins are deprecated as of v3.2.0 due to the introduction of
// Autoprefixer in our Gruntfile. They will be removed in v4.

https://github.com/twbs/bootstrap/blob/master/less/mixins/vendor-prefixes.less

@juthilo juthilo closed this as completed Oct 30, 2014
@komor72
Copy link
Author

komor72 commented Oct 30, 2014

But vendor mixins are still in use, just look at a couple of lines above my issue, line 19. And first of all, the generated CSS code is vendor-less in problematic fragment, i.e. – is broken. Or am i wrong?

@juthilo
Copy link
Collaborator

juthilo commented Oct 30, 2014

I don't think anything is broken. If you compare the Less source with the CSS output, you can see that, e.g., less/carousel.less#L30 got autoprefixed to bootstrap.css#L5863-5865.

@juthilo
Copy link
Collaborator

juthilo commented Oct 30, 2014

less/carousel.less#L19 might be an oversight. /cc @mdo on that.

@komor72
Copy link
Author

komor72 commented Oct 30, 2014

True, there are prefixes in Less-generated version. Then only my Sass-version issue is still valid. Sass-conversion on line 30 and below doesn't use prefixes.

@glebm
Copy link
Member

glebm commented Nov 12, 2014

This only compiles correctly because autoprefixer is used. Vendor prefixes are missing from https://github.com/twbs/bootstrap/blob/master/less/carousel.less#L30. /cc @mdo

@glebm glebm reopened this Nov 12, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Nov 12, 2014

Yeah, we mostly rely on Autoprefixer for vendor prefixes these days.

@glebm
Copy link
Member

glebm commented Nov 12, 2014

Then we should probably mention autoprefixer as a dependency and get rid of the prefixes. People tend to think of deprecated as still working.

@juthilo juthilo added this to the v3.3.2 milestone Nov 12, 2014
@cvrebert
Copy link
Collaborator

The prefix mixins do work, we just don't make use of them in new code.

@cvrebert
Copy link
Collaborator

We definitely ought to document Autoprefixer better though.

@komor72
Copy link
Author

komor72 commented Nov 12, 2014

Documenting is good, but it would be helpful if Sass convertion of Bootstrap was correct, i.e. generated CSS actually using the prefixes. Is Autoprefixer work preserved during conversion? Or it happens after Less-to-Sass conversion? Or is it completely missing from the Sass fork?

@cvrebert
Copy link
Collaborator

Autoprefixing is a postprocessing step that happens after the Less or Sass has been compiled to CSS.

@glebm
Copy link
Member

glebm commented Nov 13, 2014

If autoprefixer is required, we should say so. Current documentation makes it seem optional, which is misleading.

@komor72 Sass conversion just provides the Sass files, you can then use autoprefixer (e.g. via autoprefixer-rails).

@komor72
Copy link
Author

komor72 commented Nov 13, 2014

Thanks for additional info, @glebm . It explains why Bootstrap-Sass is now generating the non-prefixed CSS-code in this particular fragment and maybe some others too. In my opinion this autoprefixer is an overkill in projects like Bootstrap, where you have a complete control over what’s generated, thanks to mixins. Just another layer of complexity. But that’s my opinion. :)

@cvrebert
Copy link
Collaborator

It's a layer of simplicity for us; we don't need to research what set of prefixes is needed for current-generation browsers (since they're a moving target) and we don't need to decipher the nonstandard syntaxes that some of the prefixed properties use. Autoprefixer takes care of all this for us/you.

@komor72
Copy link
Author

komor72 commented Nov 13, 2014

Well, targeting only current-generation browsers is a bad idea IMO. I understand that ditching IE6, IE7 etc. is explainable due to problematic support, testing requirements etc. But saving a few bytes for prefixes only, for browsers which support CSS3 just fine – is too much. OK, border-radius is a good example of long unneeded prefix. But for many other there is no clear win, except saving some bytes. You have the power of mixins in BS – adding/dropping a prefix for any reason is just a click away. There is no problem of nonstandard syntax, if Bootstrap code follows the standard syntax, based on mixins. Users of BS should use the mixins too.

I understand that my knowledge of this project and the CSS topic in general is far smaller than yours, guys. So I might be completely wrong. But sometimes it is helpful to stand back and have a fresh look, what you get and what you loose. That’s why I dare to write these words.

Anyway I will implement the Autoprefixer in my project, there is already a gem for Middleman, ready to use. As an end result of this issue, there should be a documentation change at least – Autoprefixer is required to properly build the final CSS with all suported browsers/features enabled.

@glebm
Copy link
Member

glebm commented Nov 13, 2014

@komor72 Nobody is saving bytes (I agree, that would be wrong) but developer time. We are delegating the responsibility of generating prefixes to a tool that does it well (better than humans can hand-code). With autoprefixer it's up to you which browsers to target. Browser compatibility, however, is a lot more complicated than just prefixes.

@cvrebert cvrebert added the docs label Nov 13, 2014
@jrrudolph
Copy link

FYI, the bootstrap customizer (http://getbootstrap.com/customize) is also generating css without the proper vendor prefixes.

@cvrebert
Copy link
Collaborator

@montanajob Should be working in the current version thanks to #14980...

@hnrch02
Copy link
Collaborator

hnrch02 commented Nov 14, 2014

Just checked again, Autoprefixer works fine in the Customizer.

@jrrudolph
Copy link

Confirmed, thank you.

@adamsea
Copy link

adamsea commented Nov 17, 2014

@glebm "People tend to think of deprecated as still working." I think this is the first place I've ever heard a deprecated feature defined as something that people should not expect to be working. Please fix this Sass port to have the same level of support as your LESS counterpart.

@shannonmoeller
Copy link

@adamsea Agreed. To me "deprecated" means "functional, but on its way out." Not, "broken, won't fix."

@cvrebert
Copy link
Collaborator

We still need to document that Autoprefixer usage is assumed/required.

@cvrebert cvrebert reopened this Nov 17, 2014
@hnrch02
Copy link
Collaborator

hnrch02 commented Nov 17, 2014

I don't see how it is required now. Different story come v4 but works fine without at the moment.

@glebm
Copy link
Member

glebm commented Nov 17, 2014

@hnrch02 Thanks for the fix! 🎉

@komor72
Copy link
Author

komor72 commented Nov 18, 2014

As a sidenote of this problem. Is there any test in this project to check if Less and Sass versions, compiled to CSS with default settings match each other? I don’t know how you do tests in BS project, but it might be helpful to (semi)automatically spot problems like this one.

@glebm
Copy link
Member

glebm commented Nov 18, 2014

@komor72 You misunderstand: the issue existed in both versions (but LESS uses autoprefixer in the default build). No test like this exists, but would be great if someone contributed it.

@komor72
Copy link
Author

komor72 commented Nov 18, 2014

Issue of not using mixins in source code – yes. Issue of lacking prefixes in ready-made CSS – only in Sass version, because Autoprefixer took care of Less version, as you’ve mentioned. I’m asking about a test that compares final distribution CSS (I know, everybody is using source versions, gems, bundler, etc.) generated both ways – Less and Sass. Unfortunately I’m lacking knowledge to contribute it, but maybe some day, because formal testing is on my to-do list. If only I could stretch time…

@pauljonescodes
Copy link

For those looking to solve this problem using grunt, check out grunt-autoprefixer.

wilsaj pushed a commit to wilsaj/bootstrap that referenced this issue Apr 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants