RFR: 8366739: ToolBar: overflow menu with fractional scale (2) [v3]

[Cormac Redmond]

On Tue, 23 Dec 2025 07:11:04 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> It can get complicated, I had to think a bit on the response.  The main danger is switching to `snapSize` for your final step, as it is always rounds up, which can easily result in a huge jump (ie. 6.0000001 -> 7) that clearly isn't intended at all.
>> 
>> Also, each Node has its own snapping setting.  You are doing calculations for the current node.  The node you are accessing with `prefHeight` may have a different setting.  It is therefore probably wise to snap any external values first so they adhere to the same snapping setting as the current node you are working on before continuing calculations.  This is especially important when there are multiple nodes involved as you may get multiple unsnapped values that you really shouldn't add together before snapping them to your current node's setting.
>> 
>> But you are also now using the `snapSize` function (in the outer part) which IMHO is the "dangerous" variant as it does a ceiling operation.
>> 
>> So if either `spacing` or `x` are even slightly off, it rounds up.  For example on a 150% screen pixels are 2/3, so let's say spacing is `x` is 2/3, `spacing` is 4/3, adding those two together can yield subtle errors, and could result in for example `1.9999` or `2.0001`.  Now because the outer function is rounding up (to the next pixel), that can get amplified badly.  You would expect the result to be `2` (3 pixels of 2/3 size) but it may easily become `2.66666` (4 pixels of 2/3 size)...
>> 
>> By applying the ceiling function only to `prefHeight` then rounding, you are much more likely to get a stable result when adding many values together.
>> 
>> Perhaps a test case is missing that can show this subtle problem :)
>
> To add a bit more to this, I think there are a few general rules when it comes to snapping:
> 
> ## 1. Snap External Values Early
> - Each node has its own snap setting.
> - When using values from other nodes (e.g., `prefHeight`), **convert them as early as possible into the current node’s snap space** to ensure consistency in layout calculations.
> 
> ## 2. Use Ceiling for Size Values from Other Nodes
> - External values are usually **sizes**.
> - It is important to fully respect them—for example, a `Text` node requiring 11.5774 pixels should not be rounded down to 11.5, as that could trigger ellipsis or clipping.
> - When bringing these values into your snap space, use a **ceiling function** (`snapSize`) to guarantee the content fits.
> 
> ## 3. Use Rounding for Internal Calculations
> - When combining values that **belong to your current node** (spacing, margins, gaps) or have been imported already (by snapping an external source), use **rounding functions** (`snapPosition`, `snapSpace`).
> - This prevents accumulation of extra pixels that can cause visual discontinuities.
> 
> ## 4. Current Node Sizes
> - Any sizes that are important for content layout (like text widths or heights) should be snapped with **ceiling** (`snapSize`).
> - Other quantities such as spacing or gaps do not need ceiling; small discrepancies here are visually insignificant and won’t cause clipping.
> - Apply ceiling **once** when needed; repeated ceiling calls can lead to overestimation of total sizes.
> 
> ---
> 
> ## Summary
> 
> | Value Type                     | Snap Function |
> |--------------------------------|---------------|
> | External node sizes             | `ceil` (`snapSize`) |
> | Internal sums / positions       | `round` (`snapPosition`, `snapSpace`) |
> | Current node sizes (important)  | `ceil` (`snapSize`), once only |
> | Internal spacings / gaps        | `round` or unsnapped |

Thanks @hjohn. That all makes sense. I think I am understanding the purpose of these now (have been shooting from the hip a bit).

Side not: would be good to get the above message somewhere more visible (not sure where though...), maybe a JavaDoc in Region?

Anyway, after all that, I've pushed what might hopefully be the final change, and it's confined to just this code in getOverflowNodeIndex()(:


            if (node.isManaged()) {
                if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
                    x = snapPositionY(x + snapSizeY(node.prefHeight(-1)) + getSpacing());
                    length = snapPositionY(length);
                } else {
                    x = snapPositionX(x + snapSizeX(node.prefWidth(-1)) + getSpacing());
                    length = snapPositionX(length);
                }
            }


I decided to fix `length` within getOverflowNodeIndex() itself rather than in the calling method (i.e., don't assume it comes pre-snapped). I think since getOverflowNodeIndex() is doing the comparison, it's its responsibility.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/2016#discussion_r2643165825