fix: Console History Not Scrolling to Bottom (DH-14062)

[nbauernfeind]

I discovered DH-14062 independently and found that this method occasionally was prematurely returning when the view should stay stuck to the bottom of the contents.

Sometimes pane.scrollTop is sometimes 0.5 less than the expected value when tailing the content. This Math.round bumps it up. I only saw xxxxx.0 and xxxxx.5 values during investigation, but I did not do any further research on the origin of the value or the rounding error.

Here is a sample groovy snippet that reproduces 100% of the time:

import io.deephaven.internal.log.LoggerFactory
log = LoggerFactory.getLogger(Object.class
log.error().append(new RuntimeException()).endl()
t = io.deephaven.engine.util.TableTools.timeTable("PT1s")

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.04% ⚠️

Comparison is base (0dab8d7) 45.77% compared to head (cc97647) 45.74%.
Report is 1 commits behind head on main.

❗ Current head cc97647 differs from pull request most recent head 9e733d7. Consider uploading reports for the commit 9e733d7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1481      +/-   ##
==========================================
- Coverage   45.77%   45.74%   -0.04%     
==========================================
  Files         517      516       -1     
  Lines       35116    35097      -19     
  Branches     8792     8787       -5     
==========================================
- Hits        16076    16055      -21     
- Misses      18989    18991       +2     
  Partials       51       51              

Flag	Coverage Δ
unit	45.74% <0.00%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
packages/console/src/Console.tsx	18.57% <0.00%> (-0.10%)	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mofojed]

I think use Math.ceil instead to be safe:

Suggested change

	Math.round(pane.scrollTop) < pane.scrollHeight - pane.offsetHeight
	Math.ceil(pane.scrollTop) < pane.scrollHeight - pane.offsetHeight

Although - I'm unable to consistently reproduce with the snippet you provided, as I believe it is dependent on the size of the console (which is dependent on browser size etc). I'm not certain this is the "correct" fix. I think instead we should be checking the size within the requestAnimationFrame ... as I'm fairly certain the issue is that we're not checking the updated scrollHeight at this point.

[mattrunyon]

MDN has an example on checking an element is totally scrolled here. We should also use clientHeight instead of offsetHeight

[nbauernfeind]

Another reproducer is to simply print something and sleep a little in a loop:

for (int ii = 0; ii < 100; ++ii) {
    println("test line " + ii)
    Thread.sleep(20)
}

The fix I originally gave Don was:

pane.scrollTop + 1 < pane.scrollHeight - pane.offsetHeight

(noting the +1).

Interestingly the MDN article states:

scrollTop is a non-rounded number, while scrollHeight and clientHeight are rounded — so the only way to determine if the scroll area is scrolled to the bottom is by seeing if the scroll amount is close enough to some threshold (in this example 1):

Math.abs(element.scrollHeight - element.clientHeight - element.scrollTop) < 1;

The following will not work all the time because scrollTop can contain decimals:

element.scrollHeight - Math.abs(element.scrollTop) === element.clientHeight;

[mofojed]

I still can't consistently reproduce, even with your snippet (testing on latest prod), though I've definitely seen this issue before.
May as well follow the MDN article and add a comment linking to it (use clientHeight, +1)