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

WebKit CSS3 carousel transforms for supported devices #13074

Merged
merged 6 commits into from Aug 3, 2014
Merged

WebKit CSS3 carousel transforms for supported devices #13074

merged 6 commits into from Aug 3, 2014

Conversation

haydenbleasel
Copy link

This issue was originally introduced 2 years ago in issue #1966 but was dismissed by @mdo, stating:

In Chrome and Safari, I didn't notice much of a different at all (in fact I couldn't really discern any difference). We'll pass on this for now, but revisit it in a future point release since I know transitions and moving objects like this can get hairy quickly.

As @petewarden explained:

The -webkit-transform extension causes webkit to switch to hardware-accelerated rendering on supported devices, which results in far smoother animations. This CSS change enables the extension for the carousel when it's available, and in practice we see iPad and iPhone animations improve from a juddery 2 frames-per-second at best, to something smooth enough to be native.

I believe it's time to reconsider this implementation as it suits Bootstrap 3's mobile-first approach. The effects of implementing CSS3 transforms are significant on all WebKit devices I've tested, especially when using high-res images or multiple carousels (due to the large repaint times). For a full description of this issue including technical info and research, check out this Medium article.

I was going to comment on the previous issue but I rewrote the code to explicitly use 3D transforms to ensure hardware acceleration is enabled and some other helpful properties for visual integrity.

@cvrebert cvrebert added the css label Mar 15, 2014
@cvrebert
Copy link
Collaborator

Could you please also include the resulting /dist/css/bootstrap.css in your pull request? I suspect there might be some interaction with the Autoprefixer.

@haydenbleasel
Copy link
Author

@cvrebert Sure. The Travis CI failure is due to stating webkit properties without using mixins. Fixing that now.

left: 0;
}
&.next.left, &.prev.right, &.active {
.translate3d(0%, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be just 0?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it should, sorry about that. Converted all the values to percentages and the linter didn't pick up on it. I'll fix this up. I wonder if it can be removed completely...?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think CSSLint checks for that at all.
You mean remove the property completely? That I don't know, one would need to make sure it's not overwritten from somewhere else...

Copy link
Author

Choose a reason for hiding this comment

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

Turns out it is being overwritten somewhere else. Didn't work too well, so I just removed the percentage sign and committed.

@@ -230,3 +230,26 @@
bottom: 20px;
}
}

// WebKit CSS3 transforms for supported devices
@media all and (-webkit-transform-3d) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'd want to move the media query inside the .carousel block so as to improve mixin-ability.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Moved the media query into the .item class instead as I wasn't modifying the .carousel class.

@cvrebert
Copy link
Collaborator

X-Ref: #13649, #13410

@cvrebert cvrebert added this to the v3.2.1 milestone Jun 9, 2014
@mdo
Copy link
Member

mdo commented Jun 19, 2014

What about Firefox?

@haydenbleasel
Copy link
Author

@mdo Yeah I had a look into Firefox compatibility but I couldn't find any good documentation on transform-3d for Firefox. Every example I find of detecting transform capability with media queries only applies to WebKit and websites like this report transform-3d as being "untestable" on Firefox.

Perhaps it's possible to do something like this (source):

@media (transform-3d),(-webkit-transform-3d),(-moz-transform-3d),(-ms-transform-3d),(-o-transform-3d),(-khtml-transform-3d) {

But really I have no idea if this works. Any suggestions? Also sorry for the delay, just finished uni exams.

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 6, 2014

Do we really need to put this stuff under a media query? We don't do that with our other translate3d usages.

@XhmikosR
Copy link
Member

XhmikosR commented Jul 6, 2014

Regarding Firefox, it seems it supports the unprefixed translate3d since version 14.
Reference http://caniuse.com/#feat=transforms3d

@haydenbleasel
Copy link
Author

@cvrebert As in, rewriting the carousel to purely use CSS3 transitions or just stating the CSS3 rules and letting browsers ignore them at will? First one wouldn't be too good for older browsers, second sounds like it would try animating with two methods at once.

@XhmikosR Added a media query for Firefox and tested it out, seems to work pretty well. Only a small noticeable change in quality though.

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 7, 2014

@haydenbleasel The latter, but for that matter most of the other animations use CSS3, so.. But anyway, thanks for answering my naive question.

@haydenbleasel
Copy link
Author

@cvrebert I've never seen any examples where they're mixed together but I can look into it!

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 7, 2014

@haydenbleasel No need to go on any wild goose chases on my account. :-)

.transition-transform(~'0.6s ease-in-out');
.backface-visibility(~'hidden');
.perspective(1000);
&.next, &.active.right {
Copy link
Member

Choose a reason for hiding this comment

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

Add a line break above this line, and break the selectors onto separate lines.

@haydenbleasel
Copy link
Author

Sorry @mdo, formatting should be fixed now.

@mdo
Copy link
Member

mdo commented Jul 8, 2014

👍

@mdo
Copy link
Member

mdo commented Jul 13, 2014

Did we need to do any more digging into things here? I'd love to see this make it into v3.2.1 if we can test it out thoroughly enough and vet the separate-mixed comments from @cvrebert.

@haydenbleasel
Copy link
Author

@mdo Threw together a quick (horribly designed) demo. I recommend trying it out on your mobile device (especially iPhones) as you can see the difference very clearly.

@mdo
Copy link
Member

mdo commented Jul 15, 2014

Yeah, those look a lot smoother.

@mdo
Copy link
Member

mdo commented Aug 2, 2014

Apologies for the delays—is this all ready to rock?

@haydenbleasel
Copy link
Author

No worries homie, I believe it is unless anyone has edge cases or something they'd like to explore.—
Sent from Mailbox

On Sun, Aug 3, 2014 at 7:29 AM, Mark Otto notifications@github.com
wrote:

Apologies for the delays—is this all ready to rock?

Reply to this email directly or view it on GitHub:
#13074 (comment)

mdo added a commit that referenced this pull request Aug 3, 2014
WebKit CSS3 carousel transforms for supported devices
@mdo mdo merged commit c797010 into twbs:master Aug 3, 2014
@cvrebert cvrebert mentioned this pull request Aug 3, 2014
hnrch02 added a commit to hnrch02/bootstrap that referenced this pull request Aug 4, 2014
mdo added a commit that referenced this pull request Aug 4, 2014
Follow-up to #13074: use spec syntax instead of deprecated mixins
stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
WebKit CSS3 carousel transforms for supported devices
stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014
hnrch02 added a commit that referenced this pull request Nov 17, 2014
@ascotto
Copy link

ascotto commented Nov 21, 2014

I also came across at the poor performance of Bootstrap carousel, far away from being smooth as possible on desktops.

bootstrap-carusel-smooth-as-butter

So my quick and best working solution was to force hardware acceleration on all nodes of the carousel, because only in this any long painting was gone. Maybe it's not the best solution, but it works:

.carousel  *{
    -webkit-backface-visibility: hidden;
    -webkit-perspective: 1000;
    -webkit-transform: translate3d(0,0,0);
    -webkit-transform: translateZ(0);   
}

Maybe adding will-change properties will help too.

@cvrebert
Copy link
Collaborator

cvrebert commented Jan 5, 2015

@ascotto We've seen issues with the translateZ(0)/translate3d(0,0,0) hack (see #13649) and would probably be hesitant to play with that again. Might entertain a PR for will-change or other proposals though.

@twbs twbs locked and limited conversation to collaborators Jan 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants