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

eduardsdv duke at openjdk.org
Mon Apr 15 14:07:03 UTC 2024


On Fri, 12 Apr 2024 18:11:14 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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
>
> 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?

I agree with the first point. The style related properties are now bound. See the commit [92921e3](https://github.com/openjdk/jfx/pull/1434/commits/92921e36987e3cd1cfe78c17b464547aa73d241e)

As for the second point, this does work in a quick test. But I have some concerns.
The nodes can define minWidth/minHeight, so 0 is not a valid value and can lead to errors in app code.

You could move the nodes outside the visible area instead, but the nodes can have effects and margins, these can still be visible if they are large enough. Furthermore, in order for the toolbar to continue to return correct values for prefWidth(-1)/prefHeight(-1), we need to use a clip node.

I honestly can't validate that this doesn't lead to any errors.

E.g. shadow of the button, that is moved outside visible area:
![image](https://github.com/openjdk/jfx/assets/53449139/8f4009b0-b68f-4bb0-8085-01a33f7cff54)

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1434#discussion_r1565855543


More information about the openjfx-dev mailing list