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

fix(QScrollArea): prevent content re-rendering on scroll or mousemove (fix #16579) #17041

Closed
wants to merge 12 commits into from

Conversation

thexeos
Copy link
Contributor

@thexeos thexeos commented Mar 27, 2024

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on an Electron app

Other information:

After separating out the thumb rendering logic into a separate component we can isolate the rendering when scroll position is updated or thumb is shown/hidden.

The HTML and CSS output remains unchanged. The functionality remains unchanged. UI tests under ui/dev (ui/playground) all appear to be functioning as expected.

Before the change

chrome_WAZ7HH9OAi

After the change

chrome_NNniFDGsQY

This would fix #16579

@thexeos
Copy link
Contributor Author

thexeos commented Mar 27, 2024

I found an edge case where it still triggers re-rendering. So this requires a little more work.

Copy link

github-actions bot commented Mar 27, 2024

UI Tests Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit d9e2a5f.

♻️ This comment has been updated with latest results.

@thexeos thexeos marked this pull request as draft March 27, 2024 18:17
@thexeos
Copy link
Contributor Author

thexeos commented Mar 27, 2024

It turns out that Vue behaves slightly differently in dev and prod modes. It seems the effect isolation is more strict in dev and so it was not triggering the re-rendering of scrollable content when mainStyle was re-computed after hover ref was updated in @mouseenter and @mouseleave event handlers.

A complete solution is not possible without changing the code flow, as QScrollArea render function ultimately depends on scroll.vertical.thumbHidden and scroll.horizontal.thumbHidden values, which in turn change when showing/hiding the scrollbars in "default" visible mode.

Despite not being a complete solution, this change achieves:

  • No more pointless re-rendering of content when scrolling [had the largest affect on performance]
  • No more pointless re-rendering of content when showing/hiding the scrollbars, as long as contentStyle or contentActiveStyle props of QScrollArea were not set

This PR should be ready for merging.

@thexeos thexeos marked this pull request as ready for review March 27, 2024 22:32
@rstoenescu
Copy link
Member

Hi,

Huge thanks for contributing! There were merge errors so written this myself using your great sub-component idea.
Perf improvement will be available in Quasar v2.17.0

@rstoenescu rstoenescu closed this Sep 11, 2024
@rstoenescu
Copy link
Member

Again, excellent work!

@thexeos thexeos deleted the fix-16579 branch September 12, 2024 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants