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

Destorying modal is slow due to scrollbar reset #1831

Closed
Remo opened this issue May 15, 2018 · 8 comments
Closed

Destorying modal is slow due to scrollbar reset #1831

Remo opened this issue May 15, 2018 · 8 comments

Comments

@Remo
Copy link

Remo commented May 15, 2018

I've got a component with quite a few nested components. It's some kind of a form builder and one form contains a lot of elements and each elements as well as each row has a few options. Those options I'm hiding by default by using a bootstrap-vue modal component.

That works well and reasonably fast, but when I unload the page, things get insanely slow.
The problem starts in resetScrollbar which is called a lot and eventually causes querySelectorAll to consume a lot of CPU time. You can see it in the profile below:

image

Is there some way to get around that? Would it be possible to manually call that method if required and disable it by default? I know that I can customize the component and add a switch, but I'm looking for a solution that doesn't require me overriding code which won't get updated in the future. I'm also wondering if it wouldn't be enough to reset the scrollbar if we close one modal instead of doing so when it gets destroyed.

@emanuelmutschlechner
Copy link
Contributor

The function resetScrollbar restores:

  1. fixed content padding (.fixed-top, .fixed-bottom, .is-fixed, .sticky-top),
  2. sticky content margin (.sticky-top), and
  3. navbar toggler margin (.navbar-toggler)
  4. body padding

What's the length of each node list for selectors 1 to 3?

@Remo
Copy link
Author

Remo commented May 15, 2018

It's 0 for all of them? My form is huge though, document.querySelectorAll('.row').length return 6285

@rinick
Copy link
Contributor

rinick commented May 15, 2018

@Remo this is a performance issue in both the library and your implementation. for things that has 6000 rows, it's better to use a virtual scrolling system, instead of creating all of them in the dom

@rinick
Copy link
Contributor

rinick commented May 15, 2018

@emanuelmutschlechner this problem is similar to the clickout issue, the scrollbar should be reset only when it's set in the first place, not always be called in beforeDestroy.

@emanuelmutschlechner
Copy link
Contributor

@rinick agree. this function should only be called if the modal is visible

@Remo
Copy link
Author

Remo commented May 16, 2018

@rinick I'm aware, I didn't think the customer would add that many elements. In reality he just added 198 rows, but there are a lot of nested elements. While it's not very nicely engineered, removing this one call increased the performance by several factors, that's why I thought something is wrong.

@Remo
Copy link
Author

Remo commented Jun 29, 2018 via email

@rinick
Copy link
Contributor

rinick commented Jun 29, 2018

@Remo unfortunately my PR for this fix is still not approved yet
#1837

shinrox added a commit to shinrox/bootstrap-vue that referenced this issue Oct 26, 2018
* dev: (31 commits)
  feat(card): support left and right image placement (bootstrap-vue#1981)
  fix(collapse/toggle): "collapsed" class cleared when component updated (bootstrap-vue#2102)
  fix(form-file): Add validation of single file (bootstrap-vue#2028)
  chore(docs): minor update to the b-form-input docs
  chore(docs): Minor update to b-form-input docs
  feat(table): Add row-unhovered event (bootstrap-vue#1874)
  feat(table): Support contextmenu event binding for table rows (bootstrap-vue#2064)
  docs(table): fix minor typo (bootstrap-vue#2093)
  feat(table): Support sorting on nested object properties (bootstrap-vue#1868)
  perf(modal): optimize model.resetScrollbar, resolves bootstrap-vue#1831  (bootstrap-vue#1837)
  docs: Update images reference section (bootstrap-vue#1999)
  fix(observe-dom): fix comment typo (bootstrap-vue#2084)
  chore(modal): trivial word fix in comment (bootstrap-vue#2089)
  (docs): Fix grammer in Intro readme (bootstrap-vue#2092)
  fix(modal): prevent scrolling on .modal-content focus, fixes bootstrap-vue#1748 (bootstrap-vue#2060)
  feat(pagination):  added slots for first, prev, next, last, and ellipsis. Fixes bootstrap-vue#1870. (bootstrap-vue#1980)
  Handle state change on validated fields. (bootstrap-vue#1984)
  fix(docs): input group prepend slot typo (bootstrap-vue#2059)
  fix(dependencies): replace opencollective with opencollective-postintall (bootstrap-vue#2067)
  fix(docs): change b-input-group attribute 'left' to 'prepend' (bootstrap-vue#2017)
  ...
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

No branches or pull requests

3 participants