Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Round return values of getMaxScrollTop, getScrollHeight #15345

Merged
merged 1 commit into from Aug 16, 2017

Conversation

nathansobo
Copy link
Contributor

Fixes #15343

If the current scrollTop/scrollLeft value exceeds the maxScrollTop / maxScrollLeft, we reassign the values to the max on read. This allows us to behave correctly when the editor has changed in dimensions or the content has changed since the scroll position was previously assigned.

Prior to this PR, we were applying Math.round to the scroll positions when assigning them but not rounding the values when reassigning them to the max.

In situations where the content height contains fractional pixels, this can lead to a fractional-pixel scroll position when scrolled all the way down/right which causes blurry text. This can also cause unexpected firing of onDidChangeScrollTop when the value isn't actually changing due to a decision to notify listeners based on a rounded-up value not matching a fractional clipped value, as was the case in #15343.

/cc @t9md @Ben3eeE

@nathansobo
Copy link
Contributor Author

@t9md Could you give this a try to make sure it fixes your infinite loop issue?

@nathansobo nathansobo merged commit d88ddc3 into master Aug 16, 2017
@nathansobo nathansobo deleted the ns-round-max-scroll-top branch August 16, 2017 23:24
@t9md
Copy link
Contributor

t9md commented Aug 17, 2017

@nathansobo Confirmed fix with

No infinite loop for both now, thanks!

Tested with latest build on cd82330 commit.

[130] ~/Downloads% ./Atom\ 8.app/Contents/MacOS/Atom  --version
Atom    : 1.21.0-dev-cd82330
Electron: 1.6.9
Chrome  : 56.0.2924.87
Node    : 7.4.0

as-cii pushed a commit that referenced this pull request Aug 21, 2017
Round return values of getMaxScrollTop, getScrollHeight
as-cii pushed a commit that referenced this pull request Aug 21, 2017
Round return values of getMaxScrollTop, getScrollHeight
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants