RFR: 8364049: ToolBar shows overflow menu with fractional scale [v3]

[Andy Goryachev]

On Wed, 30 Jul 2025 18:52:20 GMT, Andy Goryachev <angorya at 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

I can't get it to fail with a unit test, and I don't want a headful test here.

Here is a test which passes in master though it should fail (a part of ToolbarTest):


    @Test
    public void noOverflowAtScales() {
        // typical windows scales
        double[] scales = {
            1.0,
            1.25,
            1.5,
            1.75,
            2.0,
            2.25
        };

        Button b = new Button("Create Schema");
        b.setFont(Font.font("System", 13));
        toolBar.getItems().add(b);

        BorderPane bp = new BorderPane();
        bp.setTop(new HBox(toolBar));

        stage = new Stage();
        stage.setScene(new Scene(bp, 600, 600));
        stage.show();
        tk.firePulse();

        toolBar.setOrientation(Orientation.HORIZONTAL);
        tk.firePulse();
        for (double scale : scales) {
            toolBar.setScaleX(scale);
            toolBar.setScaleY(scale);
            
            double w = bp.prefWidth(-1);
            bp.resize(w, 100);
            
            tk.firePulse();
            tk.firePulse();
            assertOverflowNotShown();
            System.out.println(toolBar.getScaleX());
        }

        toolBar.setOrientation(Orientation.VERTICAL);
        tk.firePulse();
        for (double scale : scales) {
            toolBar.setScaleX(scale);
            toolBar.setScaleY(scale);
            tk.firePulse();
            assertOverflowNotShown();
        }
    }

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1856#issuecomment-3137832092