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

perf: why is loading-container not removed after hide()? #2013

Closed
dbaq opened this issue Aug 17, 2014 · 11 comments
Closed

perf: why is loading-container not removed after hide()? #2013

dbaq opened this issue Aug 17, 2014 · 11 comments
Assignees
Milestone

Comments

@dbaq
Copy link

dbaq commented Aug 17, 2014

Type: perf

Platform: all

Hey guys,

quick question: why is the div.loading-container not removed after $ionicLoading.hide()?

I am not sure if it affects a lot the performances, but that triggers light-gray frames in the timeline of the chrome developer tools.

what was the choice behind? do you know if it affects the global performances?

Thank you for your answers.

@mhartington
Copy link
Contributor

@perrygovier, any ideas about this?

@marcsyp
Copy link

marcsyp commented Aug 19, 2014

+1

As a note, I have been having major performance issues and even crashing when using the $ionicLoading service with a template containing the spinning icon. Spent all day tracking it down and finally alleviated the crashing by not using the spinner.

@perrygovier
Copy link
Contributor

Removing it, we'd have to add it back in when a button or something is clicked again. Adding and removing elements from the DOM is expensive and can, even if they're hidden, trigger a reflow in some older browsers, so we try not to do it if it's not necessary.

If there's a way to eliminate the noise in the timeline, sweet, but I don't think we want to introduce a possible performance problem just to clean up logs. Thoughts?

@dbaq
Copy link
Author

dbaq commented Sep 2, 2014

IMHO, you should juste use display:none instead of playing on the visibility. I guess you are using the visibility to do a smooth entrance of the loading with an animation. But with a spinning icon, the visibility:hidden is not enough, the icon is still spinning even though it is not visible and triggers light-gray frames (so triggers repaint right?)

why do you think about the display:none?

@marcsyp
Copy link

marcsyp commented Sep 2, 2014

I also want to chime in again to point out that I have really bad crashing
problems when using a spinner in the ionicLoading service, and no such
crashing problems when just using plain text. So, not making any claims,
per se, but it may be more of an issue than just cleaning up logs. That
has been my experience, anyway.

Cheers,
Marc

On Tue, Sep 2, 2014 at 11:53 AM, Perry Govier notifications@github.com
wrote:

Removing it, we'd have to add it back in when a button or something is
clicked again. Adding and removing elements from the DOM is expensive and
can, even if they're hidden, trigger a reflow in some older browsers, so we
try not to do it if it's not necessary.

If there's a way to eliminate the noise in the timeline, sweet, but I
don't think we want to introduce a possible performance problem just to
clean up logs. Thoughts?


Reply to this email directly or view it on GitHub
#2013 (comment).

Marc Syp, Creative Technologist
San Francisco, CA

WARNING: The information contained in this message is legally
privileged and proprietary information intended only for the use of
the individual or entity named above. If the reader of this message is
not the intended recipient, you are hereby notified that any
dissemination, distribution, or copy of this message is strictly
prohibited. If you have received this message in error, please
immediately delete the original message and notify the sender.
Communication of the information contained in this message to any
unauthorized persons may be a violation of state or federal laws.

@chaffeqa
Copy link

chaffeqa commented Sep 3, 2014

@marcsyp just off the top of my head, I wonder if maybe a good solution would be to use a CSS spinner instead of an icon.

May play better with visibility: hidden

Would love to dig into that, once this pressing project is over one of my goals is to give back to the ionic community, and I have some ideas for the $ionicLoader I would love to contribute... but until then 😦

@perrygovier
Copy link
Contributor

display:none to display:block affects how content stacks. Now the loading backdrop covers the whole screen, but changing that property still triggers a reflow (repaint). In general, visibility:hidden is less likely to cause a reflow that would interrupt and other animations or cause a flicker.

This seems like it's breaking in to multiple issues.

To the original point, the animated spinner even when visibility is none, still triggers regular reflows, albeit tiny ones. Time to fix.

@perrygovier perrygovier added ready and removed needs: reply the issue needs a response from the user labels Sep 17, 2014
@perrygovier perrygovier added this to the 1.0.0-rc0 milestone Sep 17, 2014
@dbaq
Copy link
Author

dbaq commented Sep 17, 2014

Thanks @perrygovier! 🍻

@dbaq
Copy link
Author

dbaq commented Sep 17, 2014

Thanks! I'll test that before the end of the week.

@dbaq
Copy link
Author

dbaq commented Sep 20, 2014

With the latest nightly, I confirmed, there is no more light-gray frames triggered in the timeline. Thank you @perrygovier

@ionitron-bot
Copy link

ionitron-bot bot commented Sep 6, 2018

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants