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

Explicitly set box-sizing to avoid visual bugs when using bootstrap. #2505

Merged
merged 4 commits into from
Feb 22, 2015

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Feb 20, 2015

As discussed in #2479 when using Bootstrap and Cesium together, the default box-sizing model gets changed on us, which created some visual bugs. After this change, there are still visual differences in our widgets when using bootstrap, but the content sizing issues have been fixed.

@mramato
Copy link
Contributor Author

mramato commented Feb 20, 2015

CC @emackey fixes #2479

Use Web Essentials to
1. Add missing vendor prefixes
2. Remove no longer needed prefixes
3. Run auto-formatter
@mramato
Copy link
Contributor Author

mramato commented Feb 20, 2015

While I was at it, I cleaned up the CSS using Web Essentials:

  1. Added missing vendor prefixes
  2. Removed no longer needed prefixes
  3. Ran auto-formatter

transform: translate(0, -20%);
visibility: hidden;
opacity: 0;
-webkit-transition: visibility 0s 0.2s, opacity 0.2s ease-in, -webkit-transform 0.2s ease-in;
-moz-transition: visibility 0s 0.2s, opacity 0.2s ease-in, -moz-transform 0.2s ease-in;
-o-transition: visibility 0s 0.2s, opacity 0.2s ease-in, transform 0.2s ease-in;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Opera still use the -o- prefix now that they've switched to Blink? I thought they ended up using the -webkit- prefix as a result. Cesium was never compatible with Presto and the -o- prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure and a quick Google didn't give an answer. We already had the -o- prefix in a bunch of places, and since by default the tool adds them I figured it was better to be consistent. If you are convinced we don't need them, we should remove them all, including the ones that were already there (and next time I use Web Essentials I'll just tell it we don't care about Opera).

@emackey
Copy link
Contributor

emackey commented Feb 20, 2015

Actually I'm tempted to rip out all vendor prefixes for box-sizing, transform, and transition (possibly leaving only user-select, anything else remaining?) Then test the result in IE, FF, Chrome and see if these effects work without their prefixes.

The tool you're using generates the wrong prefixes. For example, on the -o-transition prefixes it generated, it linked to the un-prefixed transform property, where it should have linked to the -o-transform property.

@mramato
Copy link
Contributor Author

mramato commented Feb 20, 2015

As I said, I have no problem backing out the -o- prefixes, but why hurry and remove the rest? Taking them out buys us nothing and may break old browsers. Or is there some hidden benefit I'm missing?

@emackey
Copy link
Contributor

emackey commented Feb 20, 2015

No hurry to remove the working ones, it's just a pile of old complexity that will only become more obsolete over time. It irks me that automated tools do the "wrong" thing by simply copying the un-prefixed content verbatim into prefixed versions, without taking into account things like vendor-specific syntax differences, or references to other prefixed properties.

@emackey
Copy link
Contributor

emackey commented Feb 20, 2015

But yes you should leave the old ones in for now I guess.

@mramato
Copy link
Contributor Author

mramato commented Feb 20, 2015

When you get right down to it, I trust your judgment. so feel free to remove whatever you like or make any other changes before merging. If there anything you need from me, let me know.

A while back, Opera switched to the Blink/Chromium engine, and as
part of that it inherited the old -webkit- prefixes, and lost the
old -o- prefixes.  Almost everything Cesium does in CSS is now
un-prefixed in modern browsers, so all prefixes may be shed soon.
MS IE did support most of these prefixes at one time, but that time
was before IE supported WebGL, so Cesium doesn't need these prefixes.

Also added a missing -ms- prefix on user-select.
@emackey
Copy link
Contributor

emackey commented Feb 22, 2015

After much exploring of caniuse and testing in Opera and IE11, this is my current strategy:

  • user-select needs prefixes for -ms-, -moz-, and -webkit-.
  • box-sizing, transition, transform, and linear-gradient can all keep their -moz- and -webkit- prefixes for now, out-dated though they may be, as old versions do exist that can run Cesium and require these prefixes. But by next year these should all go away.
  • box-sizing, transition, transform, and linear-gradient do not need -ms- prefixes, as IE did not support WebGL when it needed those prefixes.
  • The -o- prefix is no longer recognized by Opera after its switch to Chromium, and Opera did not run Cesium prior to this switch, so the prefix has been dropped.

It's worth noting that prefixes in general are a legacy system on the web, newer features will use about:config enable flags rather than new prefixes. Prefixes must be curated by hand, as popular tools like Web Essentials will automatically do incorrect things like creating prefixed transitions that use un-prefixed transforms. I expect them to be fully obsolete by 2016, with the possible exception of user-select due to its non-standard status.

emackey added a commit that referenced this pull request Feb 22, 2015
Explicitly set box-sizing to avoid visual bugs when using bootstrap.
@emackey emackey merged commit e242c10 into master Feb 22, 2015
@emackey emackey deleted the box-sizing branch February 22, 2015 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants