8349091: Charts: exception initializing in a background thread #1697
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@andy-goryachev-oracle |
|
Reviewers: @kevinrushforth @azuev-java |
tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java
Show resolved
Hide resolved
|
Ok, changes look good and they seems to work on Mac OS. I am curious why do we make labels in charts focus traversable when a11y is switched on? May be something to do with accessibility on Windows? I haven't tested this patch on Windows yet. On Mac OS user can move accessibility cursor to non-focusable elements such as text labels and images so we should not have a problem navigating to the chart labels. Needs to be investigated more. |
I think the reason is to announce the axis values for each data point (that's what the VoiceOver announces on macOS for me) - you can readily try it in the MonkeyTester. |
|
Sorry, forgot to enable the pie chart test. |
|
Hi @kevinrushforth and @azuev-java , could you please re-approve this PR to unblock two more? |
tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java
Show resolved
Hide resolved
| @@ -541,7 +543,7 @@ private Node createSymbol(Series<X,Y> series, int seriesIndex, final Data<X,Y> i | |||
| symbol = new StackPane(); | |||
| symbol.setAccessibleRole(AccessibleRole.TEXT); | |||
| symbol.setAccessibleRoleDescription("Point"); | |||
| symbol.focusTraversableProperty().bind(Platform.accessibilityActiveProperty()); | |||
| symbol.setFocusTraversable(isAccessibilityActive()); | |||
There was a problem hiding this comment.
It looks like you are mixing the setting up of the listeners with setting the focus traversable of this symbol to the current state of accessibilityActive, which will be Platform.accessibilityActiveProperty for the FX app thread) and "false" (for background thread. As a result, this method, which is called once per symbol, will set up a new listener each time it is called.
You might consider refactoring this a bit to only ever set up the listeners when the object is constructed. Either that or ensure that you guard against repeatedly recreating the listener.
| * @return | ||
| */ | ||
| // package protected: custom charts must handle accessbility on their own | ||
| boolean isAccessibilityActive() { |
There was a problem hiding this comment.
This can be final, although as I mentioned in an earlier comment, it could probably use to be refactored.
| @Override | ||
| public void changed(ObservableValue<? extends Scene> src, Scene prev, Scene scene) { |
There was a problem hiding this comment.
This is insufficient. It will miss the case where both the node and the scene are created in the background thread, and the scene is later added to a window on the FX app thread. I tested this case and verified my assertion. This also means that in some cases, if this listener is called from a background thread, you will create a new scene listener from the current scene listener.
Consider instead using TreeShowingProperty, which is used by ProgressIndicatorSkin to set up its animation.TreeShowingProperty sets up a listener chain that fires when the the "tree showing" status of a node changed (tree showing means the node and all ancestors are visible and is attached to a scene that is attached to a window that is showing.
Alternatively, since you might not care whether the node is visible, you can just set up a binding using ObservableValue::flatMap. The code snippet of the docs:
ObservableValue<Boolean> isShowing = sceneProperty()
.flatMap(Scene::windowProperty)
.flatMap(Window::showingProperty)
.orElse(false);
And then you can then add a listener (or a value subscription might be better) on isShowing -- when it becomes true, you can setup the binding or listener to Platform.accessibilityActiveProperty.
With either of the above two solutions, the showing state will only become true on the FX app thread.
There was a problem hiding this comment.
thanks for noticing! the old code was indeed flawed.
| @@ -104,6 +102,9 @@ public abstract class Chart extends Region { | |||
| /** Animator for animating stuff on the chart */ | |||
| private final ChartLayoutAnimator animator = new ChartLayoutAnimator(chartContent); | |||
|
|
|||
| // SimpleBooleanProperty or ObjectBinding | |||
| private volatile Object accessibilityActive; | |||
There was a problem hiding this comment.
You can use ObservableValue<?> instead of Object as the type. Alternatively, use two fields, a SimpleBooleanProperty for use by the FX app thread and an ObjectBinding for use by a background thread. They wouldn't need to be volatile in that case. What you have is OK, but using two properties might simplify the logic a bit.
There was a problem hiding this comment.
It's a design decision - I won't want to waste an extra pointer.
The cpu cycles are much cheaper nowadays than bytes.
Extra bytes cost much more in cpu cycles (gc) and electricity.
| ObservableValue<Window> winProp = sceneProperty().flatMap(Scene::windowProperty); | ||
| accessibilityActive = winProp; // keep the reference so it won't get gc | ||
|
|
||
| // lambda cannot be used in place of a ChangeListener in removeListener() |
There was a problem hiding this comment.
Why not use a Subscription then? It seems tailor-made for what you are trying to do.
There was a problem hiding this comment.
I don't know how to use Subscription in this case.
This does not work:
ObservableValue<Window> winProp = sceneProperty().flatMap(Scene::windowProperty);
accessibilityActive = winProp; // keep the reference so it won't get gc
Subscription sub = winProp.subscribe((win) -> {
if (win != null) {
if (accessibilityActive == winProp) {
accessibilityActive = null;
}
if (isAccessibilityActive()) {
handleAccessibilityActive(true);
}
//winProp.removeListener(this);
sub.unsubscribe(); <-- COMPILE ERROR
}
});
Root Cause:
(Multiple) properties are getting bound to the global
Platform.accessibilityActiveproperty. Binding (and I say, accessing) of properties is not thread-safe.I also changed the design a bit. Originally, every symbol in a chart had its
focusTraversablePropertybound toPlatform.accessibilityActivein order to enable the accessibility navigation across the chart data points. This is rather inefficient, as the property has to manage (thousands?) of listeners.Instead, a single boolean property is added to each chart, with a listener added to it which iterates over data symbols to toggle the
focusTraversablePropertydirectly.The exact moment when the new property gets bound is also important, and has to happen in the FX application thread.
With this change, it is safe to create and populate charts with data in a background thread.
NOTES
Platform.accessibilityActiveproperty never transitions back to false after it transitioned to true. Some say it is because there is no mechanism in the platform to get notified which cannot possibly be true.Platform.accessibilityActiveProperty()method stipulates that "This method may be called from any thread" which is patently not true as the current implementation is simply not thread-safe.Note to the Reviewers
To avoid merge conflicts, the preferred order of integrations:
#1697
#1713
#1717
/reviewers 2
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1697/head:pull/1697$ git checkout pull/1697Update a local copy of the PR:
$ git checkout pull/1697$ git pull https://git.openjdk.org/jfx.git pull/1697/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1697View PR using the GUI difftool:
$ git pr show -t 1697Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1697.diff
Using Webrev
Link to Webrev Comment