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

Autoprefixer (fixes #12452) #12670

Merged
merged 1 commit into from Mar 7, 2014
Merged

Autoprefixer (fixes #12452) #12670

merged 1 commit into from Mar 7, 2014

Conversation

@BBosman
Copy link
Contributor

BBosman commented Feb 9, 2014

This is a work in progress! I'm posting it here to solicit feedback.

Currently it's a couple of separate commits to more easily see the different changes, but I can do a rebase if needed when it's done.

These commits add autoprefixer to the build tasks, removes the redundant manually prefixed styles from the less and docs files and does a grunt build to show the final output.

I have it setup to support the latest 2 versions of the major browsers, Internet Explorer 8 & 9, Android 2.3 (based on the comments on #12640) and Android 4 (which autoprefixer doesn't consider a major browser)

I had to split up the less Grunt task, as we want to do the prefixing after compiling the less files, but before minifying them. It also reprocesses the map files.

Most of the changes in the final css look okay to me, but I'm not entirely sure about the stuff it does to background-image.

What do you guys think?

@BBosman
Copy link
Contributor Author

BBosman commented Feb 9, 2014

As a side note: Explicitly adding Opera 12 (referencing #12641) would add an additional +/- 50 -o prefixed styles spread over the 3 .css files.

@cvrebert cvrebert added grunt labels Feb 11, 2014
@cvrebert cvrebert added this to the v3.2.0 milestone Feb 11, 2014
@XhmikosR
Copy link
Member

XhmikosR commented Feb 12, 2014

I definitely like this PR. But how about older Opera? Also, does everyone agree on removing older browsers support for 3.x? I mean, it's much easier to support browsers with autoprefixer, so perhaps older browsers support could be reconsidered and be dropped in 4.x?

Just wondering what the others think.

/CC @mdo @cvrebert

@ai
Copy link

ai commented Feb 12, 2014

Unfortunately, you should still use Opera 12 :(. Opera is not popular in global market, but very popular in some countries (12 % in Russia).

After Opera 12, next Opera 15 was totally rewriten to use Chromium engines. But not all of features from Opera 12 is realized in current Chromium’s Opera. So a lot of users prefer to use old Opera 12. Also Opera decide to forget about Linux users and will not release next Opera for Linux.

So, unfortunately, Opera 12 is still very popular (about 7 % in Russia).

@cvrebert
Copy link
Member

cvrebert commented Feb 12, 2014

Opera's engine change means Presto / Opera 12 is a dead-end, so there's less value/importance in supporting it, IMHO.

@mdo
Copy link
Member

mdo commented Feb 13, 2014

Isn't Opera 12 the most used version of Opera still though? Can't find where or recall why I'm thinking that, but it sounds about right to me.

@mgol
Copy link

mgol commented Feb 13, 2014

@cvrebert Yes, Opera 12.1x is no longer developed but in some countries Opera has significant market share and 12.1x is the most common used Opera version and it'll remain that way for some time.

OTH, Opera 12.1x doesn't have a lot of prefixed properties.

@cvrebert
Copy link
Member

cvrebert commented Feb 13, 2014

For comparison, both Foundation and YUI don't seem to support any version of Opera (according to their docs).

@mdo
Copy link
Member

mdo commented Feb 13, 2014

Let's keep it for the time being. Autoprefixer should know what's up with regards to specific support for v12 and up. Keeping it in a few areas isn't going to do us any harm. We don't need to officially support it, but we can be helpful.

@mdo
Copy link
Member

mdo commented Feb 13, 2014

Put another way, I won't go out of the way to make Opera work if it's broke, but if there's shit like this, I'm all for it.

@cvrebert
Copy link
Member

cvrebert commented Feb 13, 2014

@BBosman Could you remove the *.min.css and *.css.map files from this so that it will cleanly merge automatically?

@mgol
Copy link

mgol commented Feb 13, 2014

@mdo Yeah, I can understand not wanting to support Opera but if Autoprefixer gives it out for free, why not.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 13, 2014

@mdo: exactly my point of view too. 👍

@mathiasbynens
Copy link

mathiasbynens commented Feb 13, 2014

Opera 11.10-12.00 had -o-linear-gradient, -o-transform, -o-animation/@-o-keyframes, and -o-transition. Opera 12.10+ supports the unprefixed versions of these properties.

The only property I can think of (off the top of my head) that should be prefixed in Opera 12.10+ is -o-tab-size.

@mgol
Copy link

mgol commented Feb 13, 2014

The full list of prefixed properties that don't have their unprefixed counterparts in Opera 12.16 are:

  • -o-border-image
  • -o-link
  • -o-link-source
  • -o-object-fit
  • -o-object-position
  • -o-table-baseline
  • -o-tab-size

Of these, -o-link* are proprietary, the rest is standard. Opera Mobile declares -o-device-pixel-ratio as well.

EDIT: a more detailed list with descriptions is here: http://www.opera.com/docs/specs/presto2.12/css/o-vendor/

@BBosman
Copy link
Contributor Author

BBosman commented Feb 13, 2014

A new set of commits, incorporating most of the suggestions and some new stuff:

  • Added Opera 12
  • Added deprecation notices to obsolete mixins
  • Dropped the min and map files from the commits
  • Autoprefixed the examples
@pepelsbey
Copy link

pepelsbey commented Feb 13, 2014

Speaking of Opera 12.x support, I wouldn’t recommend you to drop it: audience is slowly moving to Blink-based Opera but still half of it left on 12.x. Especially in Russia, CIS and other regions, see StatCounter.

Especially since you could have it “for free” with Autoprefixer.

@BBosman
Copy link
Contributor Author

BBosman commented Feb 13, 2014

I found a small bug in autoprefixer:
https://github.com/ai/autoprefixer/issues/201

If a line before a line needing prefixing ends with a (block) comment, it copies that comment to all prefixed lines as well.

You can see this happening in the last change in my Grunt commit with the /* IE6-9 */ comment.

It doesn't impact the resulting code, with the exception of one spurious (and incorrect) comment as far as I can tell. I don't consider it a major issue, but I'll let you guys decide. :)

@ai
Copy link

ai commented Feb 13, 2014

@BBosman It is already fixed. I release fix in Autoprefixer 1.1 at next week.

@mgol
Copy link

mgol commented Feb 13, 2014

Current Autoprefixer config is wrong, you are missing Opera 12.1 which, as I said, is different than 12.

@cvrebert
cvrebert reviewed Feb 13, 2014
View changes
Gruntfile.js Outdated
options: {
browsers: ['last 2 versions', 'ie 8', 'ie 9', 'android 2.3', 'android 4', 'opera 12'],
},
compileCore: {

This comment has been minimized.

Copy link
@cvrebert

cvrebert Feb 13, 2014

Member

This isn't quite compilation per se, so how about just core (and similarly, theme)?

@BBosman
Copy link
Contributor Author

BBosman commented Feb 20, 2014

A new set of commits, incorporating most of the suggestions and some new stuff:

  • Updated deprecation notices to point to 3.2.0 as deprecation version
  • Renamed the targets as suggested by @cvrebert
  • Updated to use autoprefixer 1.1
  • Reverted the incorrect changes caused by the bug in autoprefixer 1.0
  • Updated the shrink-wrap

I kept the Opera version on 12, as @mdo referenced version 12 in general in his comment above. Explicitly adding 12.1 doesn't change the resulting css, so 12 also works for 12.1, although it will contain some prefixes no longer needed on 12.1.

I think it's good to go. If you guys agree, I'll collapse it to a single commit for merging.

@BBosman
Copy link
Contributor Author

BBosman commented Feb 20, 2014

  • Updated grunt-autoprefixer to 0.7.1, released earlier today.
  • Also incorporated the new cascade option. Most of the css already used that style of indenting so it only affects docs.css and brings that up to par with the rest.
@boulox
Copy link
Contributor

boulox commented Feb 20, 2014

A point to take in consideration with this move. Some dev, project use directly the less files for build their own style on top of bootstrap, remove the prefixed properties mixins will end up to no prefixed styles for thoses projects. They will need to include an autoprefixer in their build process too.

This need to be documented or mention somewhere

@cvrebert
Copy link
Member

cvrebert commented Feb 20, 2014

Yeah, gutting the mixins would seem to be a backward-compatibility breaker; can't do that until v4.

@hnrch02
Copy link
Collaborator

hnrch02 commented Feb 20, 2014

That's why they are marked as deprecated and not completely removed.

@cvrebert
Copy link
Member

cvrebert commented Feb 20, 2014

@hnrch02 But like @boulox said, until the mixins are removed completely, it requires Less users to actively change their build process. And if they don't make the change to their build, there won't be a nice compile-time error or similar to remind them.

@hnrch02
Copy link
Collaborator

hnrch02 commented Feb 20, 2014

Right, I didn't think that through, but punting this to v4 seems a bit exaggerated. Maybe if we'd handle this similar to #11933 and message it well, it could happen in v3?

@XhmikosR
Copy link
Member

XhmikosR commented Feb 22, 2014

👍 for the whole work @BBosman!

@BBosman
Copy link
Contributor Author

BBosman commented Feb 22, 2014

Updated:

  • Rebased and squashed to a single commit
  • Removed the shrinkwrap from the commit
  • Added the -o prefixes to the mixins. This way people using the less files directly get the same behavior as with the css, until we can remove those mixins in v4.
  • Marked the relevant mixins as deprecated in the docs
  • Added a short reference to the CSS styleguide in CONTRIBUTING.md

As English isn't my native language I'd like to leave the extended documentation part to someone who is more fluent in that department.

@@ -169,6 +169,7 @@ license your work under the terms of the [MIT License](LICENSE.md).
- Always a space after a property's colon (e.g., `display: block;` and not `display:block;`).
- End all lines with a semi-colon.
- For multiple, comma-separated selectors, place each selector on its own line.
- 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.

This comment has been minimized.

Copy link
@XhmikosR

XhmikosR Feb 22, 2014

Member

Are you sure about "automagically"? Is that on purpose or did you mean "automatically"?

This comment has been minimized.

Copy link
@BBosman

BBosman Feb 22, 2014

Author Contributor

Not on purpose, although it has a nice twist to it. :)

Will fix it.

This comment has been minimized.

Copy link
@BBosman

BBosman Feb 22, 2014

Author Contributor

On the other hand. It's used (on purpose?) in more places throughout the docs. See line 2890 of css.html for an example.

Will leave it for now until I get more feedback on this.

This comment has been minimized.

Copy link
@XhmikosR

XhmikosR Feb 22, 2014

Member

Might be on purpose then. Wait for feedback just to be sure.

This comment has been minimized.

Copy link
@mdo

mdo Feb 22, 2014

Member

Hah, I'm a fan of the automagic :).

@@ -5481,6 +5518,9 @@ button.close {
right: 0;
left: auto;
background-image: -webkit-linear-gradient(left, color-stop(rgba(0, 0, 0, .0001) 0%), color-stop(rgba(0, 0, 0, .5) 100%));
background-image: -o-linear-gradient(left, rgba(0, 0, 0, .0001) 0%, rgba(0, 0, 0, .5) 100%);

This comment has been minimized.

Copy link
@XhmikosR

XhmikosR Feb 22, 2014

Member

Maybe you could update less/.csscomb to reflect this?

This comment has been minimized.

Copy link
@BBosman

BBosman Feb 22, 2014

Author Contributor

What would need to be updated? As far as I can tell .csscomb.json only contains properties, not values and all -o prefixed properties used are already in there.

This comment has been minimized.

Copy link
@XhmikosR

XhmikosR Feb 22, 2014

Member

I meant if it was something missing to get the order right but i guess that's how autoprefixer does it.

@zdroid
Copy link

zdroid commented Feb 25, 2014

There is a little problem: Autoprefixer may collide with Normalize.css styles.

@mdo
Copy link
Member

mdo commented Feb 25, 2014

@zdroid Good looking out. The few changes this does make to the CSS from Normalize isn't something to worry about as far as I can tell.

@zdroid
Copy link

zdroid commented Feb 25, 2014

Of course, Normalize.css is a small lib. I'm just trying to avoid changing vendor libs. 😉

@mdo
Copy link
Member

mdo commented Feb 25, 2014

@zdroid Meh. It only affects the compiled CSS, so I'm not concerned.

@zdroid
Copy link

zdroid commented Feb 25, 2014

It should be OK then.

@BBosman
Copy link
Contributor Author

BBosman commented Feb 26, 2014

Updated it to grunt-autoprefixer 0.7.2. No further changes.

@zdroid
Copy link

zdroid commented Feb 26, 2014

This can be done without task reordering, just remove cascade property. CSSComb aligns vendor prefixes, so no need for it.

@BBosman
Copy link
Contributor Author

BBosman commented Feb 27, 2014

Currently it does have an effect on docs.css, as that isn't run through CSSComb.I agree that the cascade option wouldn't be explicitly needed otherwise, but it does no harm either.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 27, 2014

I thought we run CSSComb for all the files?

@BBosman
Copy link
Contributor Author

BBosman commented Feb 27, 2014

Rebased and dropped the cascade option as it is no longer needed due to #12861.

@BBosman
Copy link
Contributor Author

BBosman commented Mar 7, 2014

Rebased so it cleanly merges again after the recent flip-css changes.

@cvrebert
Copy link
Member

cvrebert commented Mar 7, 2014

@BBosman Thanks, much appreciated.

@mdo
Copy link
Member

mdo commented Mar 7, 2014

@BBosman Amazing <3.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 7, 2014

So what's left in order to merge this, guys?

@mdo mdo mentioned this pull request Mar 7, 2014
@mdo
Copy link
Member

mdo commented Mar 7, 2014

Lol, well, nothing—I just merged it 😍.

@mdo mdo merged commit cb7eb67 into twbs:master Mar 7, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@mdo
Copy link
Member

mdo commented Mar 7, 2014

We do have to document some things more perhaps though. So far, everything else can just carry-on though. Let's open new issues as shit comes up.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 7, 2014

Awesome 🤘

@BBosman BBosman deleted the BBosman:autoprefixer branch Mar 7, 2014
@glebm

This comment has been minimized.

Copy link
Contributor

glebm commented on less/progress-bars.less in cb7eb67 Jun 24, 2014

Vendor prefixes are only deprecated until v3.2.0. This will break for people without autoprefixer. twbs/bootstrap-sass#616

@mdo mdo mentioned this pull request Aug 19, 2015
114 of 208 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.