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 rendering bug when folds cause the vertical scrollbar to disappear with soft wrapping enabled #15800

Merged
merged 1 commit into from Oct 3, 2017

Conversation

Projects
None yet
2 participants
@nathansobo
Contributor

nathansobo commented Oct 3, 2017

Closes #15761
Closes #15549
Refs #15263

Due to the fact that the soft-wrapping boundary can change due to a variety of different factors (scrollbars appearing or disappearing, font size changes, editor dimensions changing, etc), we decided to detect changes in the soft-wrap boundary at the start of every frame before proceeding with any other rendering.

This works fine in principle, but prior to this PR it was causing us to write inconsistent values for the firstRenderedScreenRow to the per-frame derived dimensions cache in certain edge cases, causing rendering bugs downstream. This could occur when folding caused a document that was scrolled down partially to no longer be tall enough to scroll. This would cause the following unfortunate series of events:

  • We would detect the scrollbar becoming invisible due to the shrinking of the content height below the editor height and update the soft-wrap width in updateModelSoftWrapColumn.
  • Updating the soft wrap boundary would reset the display layer, causing it to recompute itself in the background.
  • We would then then immediately compute the soft wrap boundary a second time. We always compute the soft wrap boundary twice because the initial change in the soft wrap boundary could change scroll bar visibility, which causes the soft wrap boundary to change again. Unfortunately, we were computing the soft wrap boundary without forcing a synchronous update for the visible row range of the display layer, and thus not accounting for the presence of the fold and assuming the scrollbar was visible again.
  • In the process of computing the soft wrap boundary a second time, we were caching the incorrect values for the first rendered row in derivedDimensionsCache.
  • Immediately following the update of the soft wrap boundary, we would populate the display layer for visible row range, causing all future derived dimensions to be based on a correctly populated display layer. This would cause a mix of derived values cached against two different versions of the display layer, leading to inconsistent rendering state.

The solution in this PR is to

  • Compute the first rendered row prior to resetting the display layer.
  • After resetting the display layer, immediately force it to synchronously populate itself for the visible row range before making any additional computations.

These two steps ensure that the derivedDimensionsCache is never populated with inconsistent values. I'm hopeful that this issue was also the source of measurement exceptions in #15263, though we'll have to wait to be sure since we never have reproduced them.

Huge props to @Ben3eeE and @ungb for their help tracking down this edge case. Thanks so much.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 3, 2017

Member

I was able to verify that this fixes #15549 💯 🚢

Member

Ben3eeE commented Oct 3, 2017

I was able to verify that this fixes #15549 💯 🚢

@nathansobo nathansobo merged commit f317a45 into master Oct 3, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the ns-fix-soft-wrap-rendering-bug branch Oct 3, 2017

nathansobo added a commit that referenced this pull request Oct 3, 2017

Merge pull request #15800 from atom/ns-fix-soft-wrap-rendering-bug
Fix rendering bug when folds cause the vertical scrollbar to disappear with soft wrapping enabled

nathansobo added a commit that referenced this pull request Oct 3, 2017

Merge pull request #15800 from atom/ns-fix-soft-wrap-rendering-bug
Fix rendering bug when folds cause the vertical scrollbar to disappear with soft wrapping enabled

@as-cii as-cii referenced this pull request Oct 5, 2017

Closed

Atom editor renders random text over each other. #15761

1 of 1 task complete

nathansobo added a commit that referenced this pull request Oct 5, 2017

Merge pull request #15800 from atom/ns-fix-soft-wrap-rendering-bug
Fix rendering bug when folds cause the vertical scrollbar to disappear with soft wrapping enabled

nathansobo added a commit that referenced this pull request Oct 5, 2017

Merge pull request #15800 from atom/ns-fix-soft-wrap-rendering-bug
Fix rendering bug when folds cause the vertical scrollbar to disappear with soft wrapping enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment