Skip to content
This repository has been archived by the owner on Apr 6, 2018. It is now read-only.

Deal with not-yet-rendered scroll position changes in scrolling commands #906

Merged
merged 2 commits into from
Nov 2, 2015

Conversation

maxbrunsfeld
Copy link
Contributor

Fixes #903

# TODO: We need to create an API for reading an editor's logical scroll
# position which may not yet have been flushed to the DOM. Currently, the
# only way to access this information is through this piece of private state.
currentScrollTop = @editorElement.component.presenter.pendingScrollTop ? @editorElement.getScrollTop()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @nathansobo @as-cii

In this command, we're trying to read the editor's current scroll position, and update it accordingly. The problem is that if we run this command rapidly (e.g. holding down ctrl-f), calls to TextEditorElement::getScrollTop() will not reflect the value that was previously set via ::setScrollTop(), because a DOM update hasn't happened yet.

Can you think of an easier way to do this? If not, maybe we need to add an API on TextEditorElement for accessing its current desired scroll position (i.e. the last value passed to ::setScrollTop).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @maxbrunsfeld for bringing this up. So, there are a couple of ways in which we could solve this. We may either:

  1. Add something like a TextEditorElement::getPendingScrollTop API;
  2. Flush pending state (such as scroll positions) synchronously whenever methods like ::getScrollTop get called;

Personally, I would prefer 2) over 1) because it both feels more consistent with how the DOM works like and it doesn't expose an internal information which heavily depends on the update scheduling implementation. For example: what does pending mean here? In the current implementation we know that the scroll position's state is either pending or committed, but what if we started handling this differently by introducing a third state?

On the other hand, I understand the implications of having such a synchronous API: forcing reflows is not ideal, especially when the interaction with the user needs to happen repeatedly such as in this case (e.g. holding down a key). It's also true that people are already used to how the DOM behaves when reading/writing from/to it, so I feel like this wouldn't be a deal breaker. Indeed, this piece of code could be changed to make use of requestAnimationFrame (or our ViewRegistry::readDocument + ViewRegistry::updateDocument methods) to orchestrate DOM updates in a way that doesn't affect performance because of synchronous reflows.

What do you think?

@maxbrunsfeld
Copy link
Contributor Author

Gonna 🚢 this as is for a quick fix, but hopefully we can come up with something cleaner.

maxbrunsfeld pushed a commit that referenced this pull request Nov 2, 2015
Deal with not-yet-rendered scroll position changes in scrolling commands
@maxbrunsfeld maxbrunsfeld merged commit b7d11e4 into master Nov 2, 2015
@maxbrunsfeld maxbrunsfeld deleted the mb-fix-scroll-commands branch November 2, 2015 23:01
@as-cii
Copy link
Contributor

as-cii commented Nov 2, 2015

Thanks Max for addressing this, will take a better look at this tomorrow morning. 🙏

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