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

Don't cycle carousel when the page isn't visible #15566

Closed
wants to merge 1 commit into from

Conversation

cvrebert
Copy link
Collaborator

This uses the Page Visibility API (when available) to stop cycling the carousel when the page is hidden (e.g. in an inactive tab, minimized window, etc.).
This seems like better UX and also improves perf when the page is hidden (browser has less work to do).
This also addresses #15298 by not sliding while the page is hidden in Chrome in the first place (thus preventing the problematic situation from occurring).

Improves performance when the page is in a hidden tab or minimized window, etc.
Fixes #15298 as this happens to work around it by preventing the problematic situation from occurring.
@cvrebert cvrebert added the js label Jan 14, 2015
@cvrebert cvrebert added this to the v3.3.3 milestone Jan 15, 2015
@XhmikosR
Copy link
Member

This was one of the suggestions I had made in the past so I'm definitely for it 👍

@cvrebert
Copy link
Collaborator Author

CC: @hnrch02 @fat

@hnrch02
Copy link
Collaborator

hnrch02 commented Jan 21, 2015

Great idea 👍

@StevenBlack
Copy link
Contributor

I suggest this new behaviour should tie-into carousel options; that said, this PR seems like a sensible Carousel.DEFAULTS behaviour, though setting the default to false is guaranteed non-breaking.

@cvrebert
Copy link
Collaborator Author

I'm having a hard time envisioning a case where it's desirable to slide things even when the user isn't watching them. If anyone has such a use case, please speak up.
We're trying to sell this as a bugfix / implementation-detail rather than a new feature per se.

@StevenBlack
Copy link
Contributor

@cvrebert Carousel as news ticker, for example, like tweets and hash tags on current events.

Moreover Carousel already has a pause: 'hover' default, a paused property, and possesses function paused() on its prototype. Therefore pausing looks baked-in already.

Maybe Carousel pausing on visibility should use Carousel.pause()?

@cvrebert
Copy link
Collaborator Author

Carousel as news ticker, for example, like tweets and hash tags on current events.

That seems relatively niche, but I'd be interested to hear the rest of the team's thoughts on it.

Maybe Carousel pausing on invisibility should use Carousel.pause()?

That leads to some questions around "What happens if the page explicitly calls pause() while it's hidden?" In that case, when it becomes un-hidden, does the Carousel start cycling, or not? If not, then the code gets a bit more complicated.

@hnrch02
Copy link
Collaborator

hnrch02 commented Jan 22, 2015

I think it will be fine as long as we point out in the release notes that this changes the default behavior.

@cvrebert
Copy link
Collaborator Author

@XhmikosR What's your opinion after StevenBlack's remarks?

@XhmikosR
Copy link
Member

The use case @StevenBlack describes seems a valid one indeed.

But, wasting power is never good, so I'm still in favor of this change as long as we do it in a major version bump.

@StevenBlack
Copy link
Contributor

@cvrebert just kick me for being redundant but Carousel pausing on scroll should be configurable — default true is certainly OK — also .paused, .paused(), and .pause() exist so let's use these so state is managed consistently.

@cvrebert
Copy link
Collaborator Author

@StevenBlack Scrolling is a separate issue entirely. File a new bug if you care about it.

@cvrebert
Copy link
Collaborator Author

as long as we do it in a major version bump.

Punting this to v4 then.

@cvrebert cvrebert closed this Feb 17, 2015
@cvrebert cvrebert removed this from the v3.3.4 milestone Feb 17, 2015
@cvrebert cvrebert deleted the carousel-page-vis branch February 20, 2015 01:05
cvrebert pushed a commit that referenced this pull request Oct 11, 2015
Avoids cycling carousels when the page isn't visible.
Closes #17710
Refs #15566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants