RFR: 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy.

[Andy Goryachev]

On Mon, 29 Aug 2022 11:37:46 GMT, Jose Pereda <jpereda at openjdk.org> wrote:

> The corner region is currently laid out with this assumption:
> 
>         // position the top-right rectangle (which sits above the scrollbar)
> 
> However, the vertical scrollbar is not always visible. Therefore, when that is the case, the corner region is laid out on top of the table column headers, overlapping any of their graphic or text content. In case of the last visible column header, it is not always possible to scroll horizontally to see the full content, either because of a constrained resize policy or because there is no more space left after the last column.
> 
> This PR fixes this issue by considering as padding for the last visible column header the presence of the corner region over it, only when the vertical scrollbar is not visible and the corner region is.
> 
> A couple of tests have been added to both TableViewTest and TreeTableViewTest.
> 
> Test 1: Before and after the changes :
> 
> <img width="312" alt="image" src="https://user-images.githubusercontent.com/2043230/187190928-efc29e38-8de2-4daf-8eff-a218d6fae181.png">
> <img width="312" alt="image" src="https://user-images.githubusercontent.com/2043230/187190991-deaa88b3-2f52-48bc-b3ec-107a5151f7b5.png">
> 
> Test 2:  Before (sort arrow is there, but not visible) and after the changes:
> <img width="412" alt="image" src="https://user-images.githubusercontent.com/2043230/187191310-fb348d8c-84fc-4254-a1a5-91811bf09b79.png">
> <img width="412" alt="image" src="https://user-images.githubusercontent.com/2043230/187191358-0d4937cb-b3b4-4530-9489-ff8cbf0c8443.png">

The code does appear to work as expected, with and without the vertical scroll bar visible.

When testing very narrow cases, it lays out the menu button over the column header cells, as expected:

<img width="41" alt="Screen Shot 2022-08-29 at 15 26 22" src="https://user-images.githubusercontent.com/107069028/187312041-27fefabf-d0d0-481f-89e6-3624a583a729.png">

<img width="61" alt="Screen Shot 2022-08-29 at 15 38 47" src="https://user-images.githubusercontent.com/107069028/187312050-c9c49f5a-ae46-497c-b89d-b29ad7ea1cb6.png">

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java line 381:

> 379:         }
> 380: 
> 381:         double cornerRegionPadding = tableHeaderRow == null ? 0d : tableHeaderRow.cornerPadding.get();

minor: could we use consistent 0 or 0.0 or 0d?
(I'd suggest 0.0 to avoid int->double conversion, or 0d as very few people use 0d)

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java line 492:

> 490:         if (tableHeaderRow != null) {
> 491:             tableHeaderRow.cornerPadding.addListener(cornerPaddingChangeListener);
> 492:         }

question: would it make sense to go through changeListenerHandler?

doing so will create a weak listener intermediary, but might make the code added to dispose() unnecessary.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableHeaderRow.java line 694:

> 692:                              cornerRegion.getWidth() : 0d;
> 693:                     })
> 694:                     .orElse(0d);

same comment about 0/0d/0.0

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 5883:

> 5881:         assertTrue(thumbMaxX < cornerMinX);
> 5882: 
> 5883:         sl.dispose();

may be in try/finally to dispose of the StageLoader if test throws an exception?

modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 5934:

> 5932:         assertTrue(arrowMaxX < cornerMinX);
> 5933: 
> 5934:         sl.dispose();

same comment about try/finally (applies to all added tests)

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 6996:

> 6994: 
> 6995:     // See JDK-8087673
> 6996:     @Test

would it be possible to add a short description of what the tests is trying to verify?

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

PR: https://git.openjdk.org/jfx/pull/886