Skip to content
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 #114 scrolling issue on presicion touchpads #171

Merged
merged 4 commits into from
Jan 14, 2019

Conversation

MikeKovarik
Copy link
Contributor

@MikeKovarik MikeKovarik commented Jan 10, 2019

Hello,
first of all - amazing app, what I've been longing to find or wanting to make myself for a long time.

Since Surface Pro is my mainly driver, the precision touhpad scrolling issue #114 has been a dealbreaker for a while now so I decided to fix it. I've been making some EdgeHTML & HTML UWP apps for a while so I already came across this is pretty basic bug (unless MS decided to market it a feature and call it a day :D) plaguing all my projects.

What's been happening?
Scroll events are getting captured by <body> or <html> and since it has no scrollbar and no content larger than elements visible portion, EdgeHTML does the overflow bounce effect.

There's also adjacent issue to the scrolling which only surfaces on precision touchpads - pinch zooming. Also side scrolling.

What's I've done

  • Applied overflow hidden on body and html making it not scrollable because there's no content to scroll
  • Made the xterm's container scrollable and threw in pointer events for a good measure
  • prevented zooming, both with touchpad and through touch.
  • bonus: made the scrollbar autohide because it's super cool ;)

Observations
Unfortunately the scrolling isn't smooth as one would expect from UWP app because the scrolling of the terminal is handled by xterm, making it jump an entire line every time you scroll.

Touch scrolling is does not work, instead touch creates text selection. I did not investigate further. I suppose it's also on the xterm's side of thing. And maybe it's a desired behavior. If not, we could create separate issue and fix it later.

This may not be the complete solution, though it's a step in the right direction. I've been testing it for a while now and it works great :)

@hanskokx
Copy link
Contributor

hanskokx commented Jan 10, 2019 via email

@ericcornelissen
Copy link
Contributor

ericcornelissen commented Jan 10, 2019

Works like a charm 👍

Unfortunately the scrolling isn't smooth as one would expect from UWP app because the scrolling of the terminal is handled by xterm, making it jump an entire line every time you scroll.

I don't notice any difference in the smoothness of the scrolling between felixse:master and MikeKovarik:master.


Ps. is in this comment "exterm" a typo, shouldn't it be "xterm"? Moreover, maybe reword the comment to "re-enable scrolling only in the xterm container" for clarity.

@MikeKovarik
Copy link
Contributor Author

@ericcornelissen I rephrased the comment.

What i meant with the comment on scrolling was not that my PR would cause regression. Instead I sighed that the scrolling was never smooth.

To explain further. UWP apps and Edge has the amazing buttery smooth scrolling that moves the content pixel by pixel and also has the inertia when you flick the fingers on touchpad. As compared to for example chrome on windows (up until like a year ago) which always jumped like 40+ pixels at a time instead of animating to the position. Not only is it nice and fancy but it gives visual queue to the user "oh it's going from here to there", just like any good transition in mobile apps, as opposed the the typical "oh, the screen flashed and now my content is gone, where is it? up, or down? how do I go back?."

The moment I first opened Fluent Terminal i hoped it would have a smooth scrolling since it's powered by EdgeHTML. But instead scrolling is (I believe, did not look into it further) handled by xterm so that it only always scrolls exactly one line at a time. e.g. jumps to the next line by the 18+- pixels at a time.

note: found it, it's being discussed here xtermjs/xterm.js#1140

@felixse
Copy link
Owner

felixse commented Jan 12, 2019

Looks great, just one minor issue before we can merge this 👍

FluentTerminal.Client/src/style.css Outdated Show resolved Hide resolved
@MikeKovarik
Copy link
Contributor Author

alright. i didn't notice this was an option. Removed that and simplified related js code

@ericcornelissen
Copy link
Contributor

Considering the JS code simplifications (but unrelated to the PR at large) I would suggest, from a software engineering point of view, to 1) add a default value and 2) convert the string '-ms-overflow-style' into a variable or even move terminalContainer.style['-ms-overflow-style'] = 'none' inside a function/outside the switch-statement. For example (in the spirit of @MikeKovarik's changes):

function setScrollBarStyle(scrollBarStyle) {
	var DEFAULT_OVERFLOW_STYLE = 'scrollbar'; // I'm not sure what the actual default value is
	var setOverflowStyle = (value) => terminalContainer.style['-ms-overflow-style'] = value; // This prevents one from having to update 3 (or 4) lines when e.g. the string '-ms-overflow-style' needs to be changed

	switch (scrollBarStyle) {
	  	case 'hidden':     return setOverflowStyle('none');
		case 'autoHiding': return setOverflowStyle('-ms-autohiding-scrollbar');
		case 'visible':    return setOverflowStyle('scrollbar');
		default:           return setOverflowStyle(DEFAULT_OVERFLOW_STYLE); 
	}
}

@felixse
Copy link
Owner

felixse commented Jan 14, 2019

@ericcornelissen You're right, but I'm quite optimistic that -ms-overflow-style won't change anytime soon 😃

@felixse felixse merged commit 6be29f4 into felixse:master Jan 14, 2019
@ericcornelissen
Copy link
Contributor

That's fair 👍

Riebart pushed a commit to Riebart/FluentTerminal that referenced this pull request Feb 12, 2019
* fix felixse#114 scrolling issue on presicion touchpads, bounce scroll & pinch zoom

* rephrased comment

* removed css that enforced autohiding

* simplified js code handling scrollbar size
felixse pushed a commit that referenced this pull request Feb 13, 2019
* On paste, the TerminalViewModel uses the ShellProfile to translate line endings.

Default line ending translation style is set to not modify the pasted contents, Next up, adding the radio button to shell profile.

* Added UI elements for choosing line ending style.

Using a converter to map between an enum and radio buttons, and another converter to map from enum to text based on the description of the enum value.

* Added "Paste without newlines" command.

* Adding missing converters to project file.

* A few fixes after cherry-picks.

* Adding missing Command property to PasteWithoutNewlines keybinding.

* Cleaned up some unnecessary property settings.

* fix #114 scrolling issue on presicion touchpads (#171)

* fix #114 scrolling issue on presicion touchpads, bounce scroll & pinch zoom

* rephrased comment

* removed css that enforced autohiding

* simplified js code handling scrollbar size

* Press enter to close an Input dialog (#177)

... As if the primary button is pressed.

* Renew certificate

* Update README.md

* Add option to use bold text in terminals (#183)

* Add option to use bold text in terminals

... as per #182

* Add BoldText to setting defaults

* Remove AppVeyor files

* Update README.md

* Add keybinding to change the (current) tab title (#193)

* Add keybinding for change the (current) tab title

Default binding is Ctrl+Shift+N

* Update change tab title default keybind to Ctrl+Shift+R

* Experimental ConPTY support (#199)

* First steps of ConPty support

* Added resize functionality

* fix cwd, args, hide console window

* Handle shell process exited

* Make ConPTY optional

* Fix automerge errors

* Accepted many XAML formatting changes, and changed command handling to switch.

* Fixed spaxcng on the settings page.

* adapted radio button binding to be in line with the rest of the application
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants