On Fri, 13 Feb 2026 16:03:59 GMT, Andy Goryachev <[email protected]> wrote:
>> Tries to address the mystery of missing graphic in the TabPane overflow menu. >> >> ### Summary of Changes >> >> - minor `TabPaneSkin` constructor javadoc clarification >> - added the property >> - changed popup menu to be created on demand >> - removing adding the popup reference to the `TabHeaderSkin` properties (I >> think it was done for testing purposes, I could not find any references to >> it in the code) >> >> For a quick tester, use >> https://bugs.openjdk.org/secure/attachment/114240/TabPaneGraphicFactoryExample.java >> >> # Overflow Menu Graphic Property in the TabPaneSkin >> >> Andy Goryachev >> >> <[email protected]> >> >> >> ## Summary >> >> Introduce a `menuGraphicFactory` property in the `TabPaneSkin` class >> eliminates the current limitation of this skin >> in supporting menu item graphics other than an `ImageView` or `Label` with >> an `ImageView` graphic. >> >> >> >> ## Goals >> >> The goals of this proposal are: >> >> - to allow the application developers to customize the overflow menu items' >> graphic >> - retain the backward compatibility with the existing application code >> - clarify the behavior of the skin when the property is null (i.e. the >> current behavior) >> >> >> >> ## Non-Goals >> >> The following are not the goals of this proposal: >> >> - disable the overflow menu >> - configure overflow menu graphic property via CSS >> - add this property to the `TabPane` control itself >> >> >> >> ## Motivation >> >> The existing `TabPaneSkin` does not allow the overflow menu to show graphic >> other than >> an `ImageView` or `Label` with an `ImageView`. >> >> This limitation makes it impossible for the application developer to use >> other graphic Nodes, >> such as `Path` or `Canvas`, or in fact any other types. The situation >> becomes even more egregious >> when the tabs in the `TabPane` have no text. >> >> Example: >> >> >> public class TabPaneGraphicFactoryExample { >> public void example() { >> Tab tab1 = new Tab("Tab1"); >> tab1.setGraphic(createGraphic(tab1)); >> >> Tab tab2 = new Tab("Tab2"); >> tab2.setGraphic(createGraphic(tab2)); >> >> TabPane tabPane = new TabPane(); >> tabPane.getTabs().addAll(tab1, tab2); >> >> TabPaneSkin skin = new TabPaneSkin(tabPane); >> // set overflow menu factory with the same method as was used to >> create the tabs >> skin.setMenuGraphicFactory(this::createGraphic); >> tabPane.setSkin(skin); >> } >> >> // creates graphic Nodes for tabs as well as the overflow menu > ... > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 19 additional > commits since the last revision: > > - Merge branch 'master' into 8353599.menu.factory > - Merge branch 'master' into 8353599.menu.factory > - 2026 > - Merge branch 'master' into 8353599.menu.factory > - Merge branch 'master' into 8353599.menu.factory > - Merge branch 'master' into 8353599.menu.factory > - Merge branch 'master' into 8353599.menu.factory > - Merge branch 'master' into 8353599.menu.factory > - Merge remote-tracking branch 'origin/master' into 8353599.menu.factory > - Merge branch 'master' into 8353599.menu.factory > - ... and 9 more: https://git.openjdk.org/jfx/compare/11240021...534b81ee Changes requested by mhanl (Committer). modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 166: > 164: /** > 165: * Creates a new TabPaneSkin instance, installing the necessary child > 166: * nodes into the Control {@link TabPane#getChildren() children} > list, as Seems unrelated, and `Control` is fine, so why this change? modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 272: > 270: * @defaultValue null > 271: */ > 272: private ObjectProperty<Function<Tab, Node>> menuGraphicFactory; In JavaFX, `Callback` is always used for this kind of factories. In my opinion, we should be consistent with that modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 528: > 526: } > 527: > 528: private Node prepareGraphic(Tab t) { Should also be around `createGraphic`. Does not prepare anything, but creates a graphic whether by factory or by trying to create a copy. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 538: > 536: } > 537: > 538: private Node extractGraphic(Node n) { Should be renamed to `createGraphicCopy` or something like that. This is not an extract operation, but rather a try to copy the existing graphic modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 1512: > 1510: > 1511: getProperties().put(Tab.class, tab); > 1512: getProperties().put(ContextMenu.class, > tab.getContextMenu()); Seems unrelated - does that have any consequences? modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 2021: > 2019: } > 2020: > 2021: // is this really necessary? Pretty sure it is, otherwise we probably get a memory leak. I don't really like spreading random comments in the codebase which will be there for the next 20 years without any benefit. I would recommend checking if this is needed with a Profiler / `JMemoryBuddy`. But as far as I can see, this is indeed needed. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 2024: > 2022: public void dispose() { > 2023: textProperty().unbind(); > 2024: disableProperty().unbind(); This seems unrelated. modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java line 1372: > 1370: ContextMenu menu = setupMenuGraphicFactory(); > 1371: for (MenuItem mi : menu.getItems()) { > 1372: assertTrue(mi.getGraphic() instanceof Path); It would be good to check if the right graphic was created. Now we just check if it is `Path`. If there is a bug where the wrong graphic for a `Tab` is used (thinnk about `Tab` 1 has the graphic of `Tab` 2), we would not catch that regression. I recommend something like creating a `Label` with the `Tab` text, and check that: - `Tab` with text "1" creates a graphic `Label` with text "1" - `Tab` with text "2" creates a graphic `Label` with text "2" ------------- PR Review: https://git.openjdk.org/jfx/pull/1773#pullrequestreview-3991908464 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975254502 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975189722 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975202116 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975196573 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975247663 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975240334 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975204628 PR Review Comment: https://git.openjdk.org/jfx/pull/1773#discussion_r2975243774
