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

Don't allow vertical scrollbar to affect soft wrap column #17474

Merged
merged 2 commits into from Jun 7, 2018

Conversation

Projects
None yet
8 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Jun 5, 2018

Background

Previously, the decision of whether to show or hide scrollbars in a text editor was made by estimating whether or not a scrollbar was needed based on the editor's content. It involved a circular data flow:

vertical scrollbar visibility
      ↓
editor width
      ↓
horizontal scrollbar visibility
      ↓
editor height
      ↓
vertical scrollbar visibility

From what I can tell, there are some subtle issues with the timing of our measurements that currently cause this calculation to produce unstable results, resulting in the issues linked below.

Aside from these rendering bugs, this overall approach for showing and hiding scrollbars seems different from other applications that I'm familiar with. In other applications, there does not seem to be this circular feedback loop.

For example, in TextEdit.app, note how the soft wrap behavior is never affected by the vertical scrollbar. Rather, the scrollbars' visibility is controlled by the macOS Show Scrollbars setting. They behave as follows:

  • When Show Scrollbars > When Scrolling (aka 'overlay' mode) is selected, the scrollbars appear and disappear based on the editor's content, but they do not affect the editor's soft wrapping.
  • When Show Scrollbars > Always (aka 'legacy' mode) is selected, the scrollbars are always visible, so again the editor's soft wrapping does not change based on the height of its content.

text-edit-scrollbar-2

I did a similar test with Sublime Text 3, and I observed a similar behavior. I think that these apps' behavior seems simpler than Atom's from an implementation perspective, and probably better from a UX perspective as well.

Description of the Change

In this PR, I've changed Atom's logic for showing and hiding scrollbars in two ways:

  1. The editor's soft wrap column always leaves room for the vertical scrollbar, even if it is hidden.
  2. In auto-size mode, the editor always grows large enough to account for both scrollbars, even if they are hidden.

Alternate Designs

We could also fix the bugs referenced below within the current paradigm. @Ben3eeE had one idea for fixing this bug at the cost of performing extra DOM measurements. From my testing, this fix does indeed work.

Benefits

The reasons that I'm proposing this solution instead is:

  1. From what I can tell, it makes the TextEditor behave more like other native widgets.
  2. It does not require any extra DOM measurements; in fact it results in us doing less work overall in TextEditorComponent.

Possible Drawbacks

This is a bigger change, and it definitely requires careful testing on all three platforms.

Verification Process

On all three platforms (and with all possible scrollbar settings on mac):

  1. Open a file with soft wrapping (e.g. a markdown or text file).
  2. Add and remove content at the end of the file so that the vertical scrollbar enables and disables.
  3. Check that the soft wrapping does not change.
  4. Check that the scrollbars look and behave correctly overall.

Also,

  1. Open a file with no soft wrapping and some long lines.
  2. Check that the scrollbars look and behave correctly overall.

Applicable Issues

Fixes #13769
Fixes #17268

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Jun 5, 2018

/cc @nathansobo @as-cii I haven't dealt with this code, so I might be missing some aspects of this whole thing. Before I proceed to fix up the tests that I've broken, could you take a look at this overall approach?

/cc @Ben3eeE @Arcanemagus If you get a chance, could you take this for a spin? I am going to test this visually on a VM now.

@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented Jun 5, 2018

I'm on Windows, so from the description this would make the scrollbar always visible...

This sounds like an improvement for macOS users and a major regression for everyone else, unless I'm not understanding this correctly?

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Jun 5, 2018

I'm on Windows, so from the description this would make the scrollbar always visible...

I think it would just leave room for the scrollbar, similarly to what happens on mac os, when Show Scrollbars > Always is selected. I would think that Chrome would decide whether or not to actually show a scrollbar there based on the content height, just like it does for things like the Tree view. I might be misunderstanding how scrollbars behave on Windows though...

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Jun 5, 2018

I can do a test on Windows tomorrow and a code review if I've got any comments.

@thomasjo

This comment has been minimized.

Member

thomasjo commented Jun 5, 2018

I can test this on Ubuntu 16.04 tomorrow morning (UTC+1), when I get to work.

@maxbrunsfeld maxbrunsfeld changed the title from Base editor scrollbar visibility on platform setting, not content measurements to Base editor scrollbar visibility on system-level setting, not content measurements Jun 5, 2018

@daviwil

daviwil approved these changes Jun 5, 2018

Code changes look great! I prefer the reduced complexity of the code here. I'm running a build to try it out on Fedora Linux right now, will report back

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Jun 6, 2018

From your description, this seems like a big improvement. I never consulted other applications when I implemented it, and it never crossed my mind to do it more simply. Thanks!

@Arcanemagus

This comment has been minimized.

Contributor

Arcanemagus commented Jun 6, 2018

Behavior looks good to me, I'll leave the code review to others that actually know what it does 😉

2018-06-06_00-34-38

@as-cii

This comment has been minimized.

Member

as-cii commented Jun 6, 2018

This looks reasonable, @maxbrunsfeld. Thanks for jumping on it!

@thomasjo

This comment has been minimized.

Member

thomasjo commented Jun 6, 2018

Quick and dirty test on Ubuntu 16.04

peek 2018-06-06 12-37

Been running this branch for a few hours now, and not noticed any regressions. Seems to work great. Awesome job, @maxbrunsfeld 🥇

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Jun 6, 2018

I've checked it on Windows 10 and it works. I think this change makes sense as well.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Jun 6, 2018

Aside from these rendering bugs, this overall approach for showing and hiding scrollbars seems different from other applications that I'm familiar with. In other applications, there does not seem to be this circular feedback loop.

Actually, I take this back. In Chrome itself, regular DOM elements actually do seem to have this circular feedback loop where the state of the vertical scrollbar can affect soft wrap. So it's not as cut and dry as I originally made it sound. Still, in TextEdit, Sublime Text, and VS Code, the vertical scrollbar does not affect soft wrap, so I think it's ok for us to adopt this approach too.

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Jun 6, 2018

+💯 worse is better.

@maxbrunsfeld maxbrunsfeld changed the title from Base editor scrollbar visibility on system-level setting, not content measurements to Don't allow vertical scrollbar to affect soft wrap column Jun 7, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Jun 7, 2018

After testing this on windows, I decided to make this even simpler. I removed the conditional logic based on the system-level scrollbar style detection. I've updated the 'Description of the Change' section.

@maxbrunsfeld maxbrunsfeld merged commit 6c4c47a into master Jun 7, 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

@maxbrunsfeld maxbrunsfeld deleted the editor-scrollbar-style branch Jun 7, 2018

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