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 shaking editor when typing #16886

Merged
merged 1 commit into from Mar 6, 2018

Conversation

Projects
None yet
4 participants
@Ben3eeE
Member

Ben3eeE commented Mar 5, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

When using DPI scaling the scroll position could be set to a position that was not possible causing a scroll event to scroll back. This was causing the editor to shake when editing. This PR fixes this by rounding scrollTop and scrollLeft to a physical pixel instead of rounding using Math.round

Alternate Designs

N/A

Why Should This Be In Core?

It is already in Core

Benefits

  • Fixes a bug causing the editor to shake when typing

Verification Process

Before:
before

After:
after

Applicable Issues

Fixes #15786

/cc: @nathansobo

Fix shaking editor when typing
When using DPI scaling the scroll position could be set to a position
that
was not possible causing a scroll event to scroll back. This was causing
the editor to shake when editing. This PR fixes
this by rounding scrollTop and scrollLeft to a physical pixel instead of
rounding using Math.round

Co-authored-by: Antonio Scandurra <as-cii@github.com>

@Ben3eeE Ben3eeE added the needs-review label Mar 5, 2018

@Ben3eeE Ben3eeE requested a review from as-cii Mar 5, 2018

@daviwil

daviwil approved these changes Mar 5, 2018

@daviwil

This comment has been minimized.

Member

daviwil commented Mar 5, 2018

Definitely worth including this in a potential Atom Beta hotfix this week. Is it also worth shipping a stable hotfix if we have some other fixes to include? /cc @iolsen

@as-cii

as-cii approved these changes Mar 6, 2018

Looks good, nice work reproducing this @Ben3eeE!

To recap, we were setting the scroll position to a position that was not physically addressable due to window.devicePixelRatio being a number between 1 and 2. As such, the browser clipped the requested scroll position, which in turn triggered a new scroll top change to a position that was actually addressable. This new event was intercepted by TextEditorComponent which attempted to scroll again, but rounding differently this time around due to the clipping the browser was applying.

By always rounding the scroll top position to a physically addressable pixel we solve this problem and prevent the browser from doing the clipping itself, thereby avoiding the flickering as well.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Mar 6, 2018

@daviwil I agree that this would be good to include in an upcoming beta hotfix.

@Ben3eeE Ben3eeE merged commit 113b771 into master Mar 6, 2018

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

@Ben3eeE Ben3eeE deleted the b3-fix-shaking-editor branch Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment