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 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 this to the v3.2.0 milestone Feb 11, 2014
@XhmikosR
Copy link
Member

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
Collaborator

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
Collaborator

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
Collaborator

@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

@mdo: exactly my point of view too. 👍

@mathiasbynens
Copy link

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

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:
postcss/autoprefixer#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.

options: {
browsers: ['last 2 versions', 'ie 8', 'ie 9', 'android 2.3', 'android 4', 'opera 12'],
},
compileCore: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Collaborator

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
Collaborator

@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?

@cvrebert
Copy link
Collaborator

Yes, we should at least:

  • explain Autoprefixer in CONTRIBUTING.md
  • in the docs HTML, mark as deprecated or entirely remove mentions of the deprecated mixins

Also: @BBosman, if you remove test-infra/npm-shrinkwrap.canonical.json from the PR, that would help it auto-merge cleanly.

@XhmikosR
Copy link
Member

👍 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@zlatanvasovic
Copy link
Contributor

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.

@zlatanvasovic
Copy link
Contributor

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.

@zlatanvasovic
Copy link
Contributor

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.

@zlatanvasovic
Copy link
Contributor

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

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
Collaborator

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
@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 🤘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.