8358023: javafx.scene.control.ColorPicker configure size of squares in color pallet

[Ziad-Mid]

Added new method setSquareSize(double size) to be able to customize the size of squares in ColorPalette which was previously hardcoded with a value 15

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion jfx26 to be approved (needs to be created)
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8358023

Issue

• JDK-8358023: ColorPicker: configure size of squares in color palette (Enhancement - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1954/head:pull/1954
$ git checkout pull/1954

Update a local copy of the PR:
$ git checkout pull/1954
$ git pull https://git.openjdk.org/jfx.git pull/1954/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1954

View PR using the GUI difftool:
$ git pr show -t 1954

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1954.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back zelmidaoui! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 00: Full (e44d18c3)

[andy-goryachev-oracle]

Question: should we make it a styleable property instead, so the application can configure it with CSS?

[kevinrushforth]

Since this PR adds public API, it will need a CSR. And the new public methods will need API docs.

You will also need to automated tests.

Question: should we make it a styleable property instead, so the application can configure it with CSS?

Yes, I think this would be best.

/reviewers 2
/csr needed

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[openjdk]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@Ziad-Mid please create a CSR request for issue JDK-8358023 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[kevinrushforth]

I was about to try and figure out why the lack of javadoc comments didn't fail the build. Then I noticed that the class in question, ColorPalette.java, is not a public class.

That means that this PR doesn't actually provide the desired functionality as far as the application is concerned.

This will need more work. I recommend moving it back to Draft until it is ready.

[andy-goryachev-oracle]

Then I noticed that the class in question, ColorPalette.java, is not a public class.

Yes some of its styleable properties are present in modena.css and caspian.css - like color-picker-grid selector for instance. It is not, however, specified in the CSS reference at all (even though the skin itself, ColorPickerSkin is a public class). So if one wants to provide a custom ColorPicker skin, there is no middle ground - either use the stock implementation with a limited range of customization, or create your own. This is the kind of difficulties one finds working with JavaFX - it's extremely difficult to extend.

So adding (an undocumented) styleable property to ColorPalette.java might solve the issue described in the ticket, without requiring a CSR since no public APIs are introduced, right?

[kevinrushforth]

Yes some of its styleable properties are present in modena.css and caspian.css - like color-picker-grid selector for instance. It is not, however, specified in the CSS reference at all (even though the skin itself, ColorPickerSkin is a public class).

Ideally, everything used by modena.css would be documented in the CSS guide.

So if one wants to provide a custom ColorPicker skin, there is no middle ground - either use the stock implementation with a limited range of customization, or create your own. This is the kind of difficulties one finds working with JavaFX - it's extremely difficult to extend.

This highlights the inadequacy of these type of undocumented "back door" properties.

So adding (an undocumented) styleable property to ColorPalette.java might solve the issue described in the ticket, without requiring a CSR since no public APIs are introduced, right?

I'd prefer not to implement this enhancement by adding even more hidden, undocumented, CSS properties. How many such properties are there on ColorPickerSkin? Would it make sense to document them or would that open a larger can of worms?

[andy-goryachev-oracle]

This is a good question. We already have a can of worms on our hands, so we need to figure out what to do with it.

First of all, we should decide (speaking generally) what properties should be in the control, to be applied to all the possible skins, and which properties are specific to a particular Skin implementation. In this particular case, the size of the grid cell does not seem to belong to the control, it's clearly a property of the skin.

Then, we need to decide whether we allow extending that particular skin, and the degree in which we allow this. We may want to document the structure (hierarchy and style classes) of the skin - maybe in cssref.html, or in the skin's javadoc or similar place (Example: VFlow:78).
Keep in mind that we do support both modena and caspian themes, to any changes to styles need to be reflected in both.

Or maybe even go further and design the skins explicitly to allow extension - using extendable classes and methods.

Then, of course, there is a question of extending behavior and that's where the discussion stalls.

What do you think?

[kevinrushforth]

This is a good question. We already have a can of worms on our hands, so we need to figure out what to do with it.

You are right.

In this specific case, we have many color picker skin properties in modena.css and none are documented. The only thing about the skin that is documented is this, from the CSS guide:

Substructure

    color display node — Label
    arrow-button — StackPane
        arrow — StackPane

Nothing at all about the popup or the palette.

First of all, we should decide (speaking generally) what properties should be in the control, to be applied to all the possible skins, and which properties are specific to a particular Skin implementation.

Right. One thing to note is that if a property is in the skin, and we document it and provide public API in our skin implementation, we are effectively "suggesting" that all skins implement that property.

In this particular case, the size of the grid cell does not seem to belong to the control, it's clearly a property of the skin.

Perhaps, although there are many controls where sizing information is taken from the control. The most obvious example is text font and size for controls that have a label, which propagates from the control (Button, Label, ComboBox, etc) to the skin. That may or may not make a case for adding the size of the grid cell to the control.

Then, we need to decide whether we allow extending that particular skin, and the degree in which we allow this. We may want to document the structure (hierarchy and style classes) of the skin - maybe in cssref.html, or in the skin's javadoc or similar place (Example: VFlow:78). Keep in mind that we do support both modena and caspian themes, to any changes to styles need to be reflected in both.

Yes, this is the larger issue. Whatever we do would probably be limited to those attributes that are not overly specific to one particular skin implementation.

Or maybe even go further and design the skins explicitly to allow extension - using extendable classes and methods.

This would be a large effort, and probably not something we want to tackle.

Then, of course, there is a question of extending behavior and that's where the discussion stalls.

Right. We are unlikely to address this any time soon.

Getting back to this specific enhancement request, we seem to have a few options, presuming we want to provide this capability without having it blocked by a much larger effort:

1. Add a private, undocumented styleable CSS property that app developers have to discover by looking at modena.css, reading the source code, or reading the comments in the JBS issue that implemented it. (as you can guess, I don't like this option)
2. Provide a documented styleable CSS property that would apply to the skin, but without way to set it via API on the skin
3. Provide a documented styleable CSS property in ColorPickerSkin so that there is public API as well as it being settable via CSS.
4. Add a styleable property to the ColorPicker control, which the skin would then implement.
5. Something else?

[andy-goryachev-oracle]

Good points!

I might suggest option 1 for this PR, with a follow-up created for improving CSS ref regarding the missing structure and styles (and maybe a discussion on the mailing list about what to document and where). I think we ought to document the default skin structure, but make it clear that the structure and the styles describe the default skin implementation for the purposes of styling.

I don't know whether it makes sense to extract the skins css ref into a separate document or not - it might still be easier from the hosting point of view to keep everything in one place since we don't really have a proper CMS.

[Ziad-Mid]

I changed the status of the PR to draft as suggested to review the comments.