elanthia-online / illthorn

Electron-based FE
18 stars 10 forks source link

Win 10 game scroll window does not scroll with new input #179

Closed OSXLich-Doug closed 2 years ago

OSXLich-Doug commented 3 years ago

Win 10 Illthorn application.

Commands input or server information sent does not scroll the main window (remains at the first buffered line). Scrolling down to catch up does not initiate window scrolling. It stays where the user scrolls down to.

Windows 10 Pro, built with yarn 1.22.5, node.js 14.15.3 LTS, standard ruby 2.6.6 32 bit with GTK3 standard gems. Only occurs on Windows 10 application.

deatrys commented 3 years ago

happens on Linux. yarn 1.22.5 node.js 14.15.3 ruby 2.7.0p0

chriscoyier commented 3 years ago

This seems like such a bad bug it would make the client entirely unusable, yes?

Maybe we should look at this technique https://blog.eqrion.net/pin-to-bottom/

ondreian commented 3 years ago

Offloading as much as possible to the CSS engine would be ideal for rendering performance too.

On Tue, Feb 16, 2021 at 3:27 PM Chris Coyier notifications@github.com wrote:

This seems like such a bad bug it would make the client entirely unusable, yes?

Maybe we should look at this technique https://blog.eqrion.net/pin-to-bottom/

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/elanthia-online/illthorn/issues/179#issuecomment-780157883, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIKHAVDAEJQKLQ76K3IRCLS7LWL7ANCNFSM4VHOSELA .

chriscoyier commented 3 years ago

I attempted to chase down this idea here: https://github.com/elanthia-online/illthorn/commit/35213e9e5144bb87653c9008e025aa3abfcfbbdc

Ultimately a failure.

The good news is the CSS scroll pinning DOES work. The downside is that it requires user interaction (the usual actually scrolling) in order to kick in. I tried a number of ways to trick it, but it's not happening.

Forcing a user to scroll in order to start the scroll pinning seems like a non-starter on this. And even if the user does scroll to the bottom to get the scroll pinning to work, it doesn't seem to catch 100% of the time. If there is a lot of screen scroll, it's hard to get it to catch. Just not good enough UX.

I don't have a good way to test on Windows and/or Linux, so I don't have a way to look why the existing method of scroll pinning isn't working. I have seen it before though, on macOS as well. I think sometimes zooming effects it.

deatrys commented 3 years ago

I was looking into this a little too. Not sure why it works on other builds, since it doesn't on mine. But, I noticed something curious in feed.js.

has_prompt() does a check for: this.root.lastElementChild.className == "game" but the elements never have that className. Maybe this never even gets called or something.

If you want the prompts to have that className ("game"), you can make these modifications: src/sesion/session.js, line 204, change to: prompt.className = "game"
src/session/feed.js, line 170, add after that line: prompt && prompt.setAttribute("class", "game")

This doesn't change anything from what i can tell, and is not entirely related to the scrolling issue...just looked like an incomplete thought.

deatrys commented 3 years ago

well, reattach_head() gets called twice when the app starts, but not when new content is output, in my build. reattach_head() just scrolls to the bottom when called, so the name doesn't really make sense, i think its supposed to do something else? and there should be a different function just to scroll down? this is most of why i didn't and still don't plan to fix it, it doesn't make sense...

deatrys commented 3 years ago

well, reattach_head() gets called twice when the app starts, but not when new content is output, in my build. reattach_head() just scrolls to the bottom when called, so the name doesn't really make sense, i think its supposed to do something else? and there should be a different function just to scroll down? this is most of why i didn't and still don't plan to fix it, it doesn't make sense...

in /src/session/streams.js the same function is called "advance_scroll()"

deatrys commented 3 years ago

the problem on mine is with: if (!was_scrolling)

for some reason the functions ("reattach_head" for Feed, and "advance_scroll" for Stream) do not get called because of the value of (!was_scrolling)

ken-r-smith commented 2 years ago

I was able to fix this by changing the _scrolling method to calculate differently:

get _scrolling() { // no content scrollable if (this.root.scrollHeight == this.root.clientHeight) return false // check the relative scroll offset from the head return ( this.root.offsetHeight + this.root.scrollTop <= this.root.scrollHeight ) }