On Wed, 30 Jul 2025 18:52:20 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> This is a very localized fix for the issue described in >> https://bugs.openjdk.org/browse/JDK-8364049 which resulted from comparing >> snapped and non-snapped values. The issue seems to happen only with >> fractional scale, that is, on Windows at 125%, 150%, 175% etc. scales. >> >> --- >> >> While looking at the `ToolBarSkin` code, I noticed a general pattern related >> to snapping, that might cause similar issues in the tool bar skin and >> elsewhere. Here is an example in `ToolBarSkin::computePrefWidth()` in L411: >> >> >> if (toolbar.getOrientation() == Orientation.HORIZONTAL) { >> for (Node node : toolbar.getItems()) { >> if (!node.isManaged()) continue; >> prefWidth += snapSizeX(node.prefWidth(-1)) + getSpacing(); >> } >> prefWidth -= getSpacing(); >> } else { >> >> >> the general issue, in my opinion, is that doing `prefWidth += >> snapSizeX(node.prefWidth(-1)) + getSpacing();` results in the `prefWidth` >> value differ from its snapped value. In other words, whenever computation >> involves snapped values, the result must be snapped as well - and that >> includes the case when all the parts of the computation are snapped. >> >> Another, related, topic is how to properly snap the values in the >> computation. I would say ideally it should be done like this: >> >> >> snappedResult = snap(snap(value1) .OP. snap(value2) .OP. ... snap (valueN)) >> >> >> It might be possible to skip the snapping of intermediary values, and only >> snap the result, but one must be careful not to accumulate errors. >> >> Getting back to the ToolBarSkin, one can see the issue on LL392, 399, 411, >> 417, 425, 436, 530, and so on. >> >> I decided not to fix the snapping for the purpose of making this PR narrow >> in scope with the goal to backport it to jfx25, but I did want to describe >> the issue. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > get toolbar length Here's a test that fails without, and passes with the fix: @Test public void overflowMenuNotShowingWithDifferentRenderScales() { double[] renderScales = { 1.0, 1.25, 1.5, 1.75, 2.0, 2.25 }; var rect = new Rectangle(100, 100); var toolBar = new ToolBar(rect); toolBar.setSkin(new ToolBarSkin(toolBar)); for (var orientation : Orientation.values()) { toolBar.setOrientation(orientation); for (double scale : renderScales) { Stage stage = new Stage(); stage.renderScaleXProperty().bind(DoubleConstant.valueOf(scale)); stage.renderScaleYProperty().bind(DoubleConstant.valueOf(scale)); stage.setScene(new Scene(new HBox(toolBar), 600, 600)); stage.show(); assertOverflowNotShown(toolBar); stage.close(); } } } private void assertOverflowNotShown(ToolBar toolBar) { Pane overflowButton = (Pane)toolBar.queryAccessibleAttribute(AccessibleAttribute.OVERFLOW_BUTTON); assertNotNull(overflowButton); assertFalse(overflowButton.isVisible()); } I suggest to place this test in `ToolBarSkinTest` (not in `ToolBarTest`), because it tests the implementation of the skin and not the implementation of the control. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1856#issuecomment-3138252523
