FXMisc / Flowless

Efficient VirtualFlow for JavaFX
BSD 2-Clause "Simplified" License
185 stars 38 forks source link

JavaFX 17 breaks scrollbar behaviours in VirtualizedScrollPane #97

Closed fthevenet closed 3 years ago

fthevenet commented 3 years ago

Starting with javaFX 17-ea+11 I noticed that scrolling a StyledTextArea (from RichTextFX) no longer worked properly: when clicking the scroll bar elements, the text scrolls for the first couple of clicks and then entirely stop responding to clicks on these elements. It does respond to using the mouse wheel of keyboard arrows however.

https://user-images.githubusercontent.com/7450507/130807718-723b881d-1de7-4dcd-82d3-d95c4fa3848d.mp4

Since it did work with previous early access of jfx (up to 17-ea-9, actually), I did manage to pinpoint that change responsible: https://github.com/openjdk/jfx/pull/454

The above PR changes the type of listener used internally by the built-in bi-directional bindings, from ChangeListeners<T> to InvalidationListeners. This should be transparent to most if not all applications, but because Flowless relies on ReactFX, which interacts with bindings and events in a very intimate way, something was broken by that change.

I managed to workaround the issue in the VirtualizedScrollPane by replacing the affect bi-directional bindings by a couple of mirrored ChangeListeners.

I will submit a PR illustrating this workaround for your consideration.

fthevenet commented 3 years ago

I realized I forgot to take care of cleaning up the added listeners in the dispose method... I'll do that in a new PR tomorrow.

Jugen commented 3 years ago

Can you also put a comment directly in the code changes you've already made referring to this issue or the first PR, so that some bright spark doesn't come along later and say "Hey this should be a bidirectional binding ...." and revert the change ;-)

fthevenet commented 3 years ago

FYI, I have reached out to the JavaFX dev mailing list with regard to this issue: https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-August/031726.html; we might want to see the outcome of that discussion before releasing that change in Flowless.

Jugen commented 3 years ago

Thanks, okay will do ....

fthevenet commented 3 years ago

As it turned out, the root cause of the issue lies with the aforementioned change in JavaFX. The regression has been addressed and is tracked as JDK-8273138 but unfortunately the fix arrived too late to be shipped in the GA release of OpenJFX 17 (it should, however, be back-ported to 17.0.1).

What this all means is that the workaround provided won't be needed once 17.0.1 is released (in about six weeks time). However, if you decide not to release it, there will be a short window in which using the latest JavaFX will break even the most up-to-date RichTextFX.

Also, I'd like to point out that at any rate the original code is not correct and would need to be fixed anyway, as the properties unbound in dispose method do not match those bound in the constructor:

Bindings.bindBidirectional(hbarValue, hPosEstimate);
Bindings.bindBidirectional(vbarValue, vPosEstimate);
.
.
.
hbarValue.unbindBidirectional(content.estimatedScrollXProperty());
vbarValue.unbindBidirectional(content.estimatedScrollYProperty());

So the situation is a bit messy unfortunately; for what it's worth, I would personally prefer that you release a new flowless with the workaround, so that I can ship my own code with OpenJFX 17 as soon as it is released, but of course it is not my call to make.

Jugen commented 3 years ago

Okay, will do a release by the weekend unless something crops up .....

Jugen commented 3 years ago

Hi Frederic, just to let you know that I've just published release 0.6.6 If you're using Maven then it should show up there in 24 - 48 hours, I think.

fthevenet commented 3 years ago

That's great! Thanks!