RFR: 8328577: Toolbar's overflow button overlaps the items [v4]

[Andy Goryachev]

On Fri, 12 Apr 2024 15:21:14 GMT, eduardsdv <duke at openjdk.org> wrote:

>> This change fixes the calculation of which nodes go to the toolbar and which go to the overflow menu.
>> It is now determined before the nodes are removed from the scene graph.
>> This is important because the values returned by ``Node.prefWidth(..)``/``Node.prefHeight(..)`` may depend on whether the node is added to the scene graph.
>> 
>> Furthermore I corrected the ``hasOveflow`` value passed to the ``organizeOverflow(double, boolean)`` in ``correctOverflow(double)``.
>
> eduardsdv has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8328577: Collect overflowed items in a shadow (overflowBox) pane

It looks like the flicker issue has been resolved.

There might be an issue with runtime style update which the proposed solution may not handle (I can't readily test it right now, but I think it is a valid concern).

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java line 570:

> 568:         // The overflowBox must have the same style classes, otherwise the overflow items may get wrong values.
> 569:         overflowBox.setId(box.getId());
> 570:         overflowBox.getStyleClass().addAll(box.getStyleClass());

I think this solution might not be the best one.  One reason is that both the `id` property and the style class list can change at runtime, and this code does not handle that (using `bind()` and `bindContent()` for property and the observable list correspondingly).

On a higher level, do you think it is possible to simply lay out hidden buttons using width/height of 0 instead?  This way there will be no need for the `overflowBox` and its maintenance.

What do you think?

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java line 783:

> 781:                     CustomMenuItem customMenuItem = new CustomMenuItem(node);
> 782: 
> 783:                     // RT-36455:

since we moved this code, could we change this line to
`     // RT-36455 (JDK-8096292):`

and also make sure that this change introduced no regression w.r.t. JDK-8096292 ?
(My limited testing indicates that it still works as expected, but please double check)

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

PR Review: https://git.openjdk.org/jfx/pull/1434#pullrequestreview-1998162402
PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1562978834
PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1562984112