8207333: [Linux, macOS] Column sorting is triggered always after context menu request on table header by jperedadnr · Pull Request #1754 · openjdk/jfx

jperedadnr · 2025-04-01T17:31:14Z

Note: The JBS issue JDK-8207333 refers to Linux, but it happens on macOS too.

When a TableColumn has a ContextMenu, if the user right clicks on the tableColumnHeader, the ContextMenu shows up, but, as described on the issue, on macOS and Linux, the table gets sorted as well.

Currently, TableColumnHeader#mouseReleasedHandler checks for MouseEvent::isPopupTriggered, but it doesn't have a check on mousePressed. However, it can be seen that a right click on the header has the following values for MouseEvent:: isPopupTriggered on the different platforms and mouse pressed and released events:

isPopupTriggered on:	Windows	macOS	Linux
mousePressed	false	true	true
mouseReleased	true	false	false

Also, the JavaDoc for MouseEvent::isPopupTriggered clearly states that:

Note: Popup menus are triggered differently on different systems. Therefore, popupTrigger should be checked in both onMousePressed and mouseReleased for proper cross-platform functionality.

Therefore, this PR adds a check to MouseEvent::isPopupTrigger in the mouse pressed event, that can be used later on to cancel the header sorting when the mouse released event happens.

Also a system test has been added. It fails on macOS and Linux, and passes on Windows before this PR, and passes on the three platforms with this PR.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8207333: [Linux, macOS] Column sorting is triggered always after context menu request on table header (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Ziad El Midaoui (@Ziad-Mid - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1754

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-01T17:32:02Z

👋 Welcome back jpereda! 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 · 2025-04-01T17:32:20Z

@jperedadnr This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8207333: [Linux, macOS] Column sorting is triggered always after context menu request on table header

Reviewed-by: angorya, zelmidaoui

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 18 new commits pushed to the master branch:

9950d33: 8169285: Re-enable javafx.swt tests
59c2b75: 8355740: Update to Xcode 15.4 on macOS
ac12979: 8354940: Fail to sign in to Microsoft sites with WebView
... and 15 more: https://git.openjdk.org/jfx/compare/3fdd21386d6db96294fcecd80afc25d09732c067...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-04-01T17:35:33Z

Webrevs

andy-goryachev-oracle · 2025-04-01T17:39:12Z

I'll take a look.
Should we update the JBS and this PR title to remove [linux] as it applies to both platforms?

andy-goryachev-oracle · 2025-04-01T17:54:23Z

I can only test it on macOS and Windows. Could someone with a linux system help please?
/reviewers 2

openjdk · 2025-04-01T17:55:26Z

@andy-goryachev-oracle
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).

andy-goryachev-oracle

thank you for fixing this bug, @jperedadnr !

testing with the monkey tester looks good on macOS 15.3.2 and windows 11.

andy-goryachev-oracle · 2025-04-07T22:48:44Z

@Ziad-Mid could you be the second reviewer?

Ziad-Mid · 2025-04-08T10:53:24Z

@Ziad-Mid could you be the second reviewer?

I can't test on Linux , I will check same for macOS

beldenfox · 2025-04-08T16:55:51Z

+        popupTriggeredOnMousePressed = me.isPopupTrigger();
        me.consume();

        header.getTableHeaderRow().columnDragLock = true;


On Mac a context menu can be invoked using the right button or the left button in conjunction with the Control key. In the second case the pop trigger flag will be set and the primary button will be down. If I'm reading the code correctly this could get the header stuck in column re-ordering mode.

Testing on macOS 15.3.2, it does not get stuck when using control+left-click, works as expected.

There is one (existing and unrelated) problem - if the user tries to dismiss the popup menu by clicking on the header, it dismisses the popup (correctly), but then proceeds to re-sort the table (unexpected).

One can say it works as designed, since the click falls on a header cell and the expected operation is to toggle the sorting, with the side effect of dismissing the popup.

On the other hand, Excel (both mac and windows) behaves differently - a click on the spreadsheet header dismisses the popup and does not select the column, as it normally does when no popup is present.

What do you guys think?

control+left-click works as expected for me too.

When the context menu is showing, clicking anywhere outside of it, closes the popup (because of the autohide property), but doesn't consume the event. If you click on a button, for instance, it will be fired. In any case, this is the same before and after this PR, so maybe an issue for a later discussion.

definitely an unrelated issue, I just wanted to ask whether you guys think it crosses the threshold for a bug.

control+left-click works as expected for me too.

For control+left-click it looks like the code will call columnReordingStarted but on the released event it won't call columnReorderingComplete. I have no idea if this will lead to problems or not.

@beldenfox Do you think we should address that issue (ctrl+left click + columnReordering) in this PR? Else, do we need to do anything else to move it forward?

You should be good with a few tweaks. If the press event is a popup trigger don't set the columnDragLock or call columnReorderingStarted (since you might not get a release event). In the drag handler check popupTriggeredOnMousePressed and do nothing if it's set. Your released handler looks fine as-is. I think that should cover it.

By the way, in a perfect world you would have more predictable behavior. When the popup menu is shown it would grab the mouse pointer and you would never see the release event. That would allow the user to click to bring up the popup and start dragging across the menu items without having to release the mouse.

Thanks, @beldenfox! I've included your suggestions, and also added the ctrl+left click macOS case to the test.

beldenfox · 2025-04-08T16:56:07Z

@@ -271,7 +274,10 @@ public TableColumnHeader(final TableColumnBase tc) {
        TableColumnHeader header = (TableColumnHeader) me.getSource();
        header.getTableHeaderRow().columnDragLock = false;


On Ubuntu 22.04 the table column header is not receiving a mouse released event if a context menu is shown. Not sure where the released event is going but it's not to any node on the primary stage. The docs are vague on this but this looks like a separate bug that's making it hard to test this PR.

what happens in the master branch (without this PR's changes)?

I was having trouble reproducing the original bug on the master branch; the sorting is done when the mouse is released and most of the time that event never arrives (I think it does every now and then).

this is strange - the bug is immediately apparent. which macOS version do you have? although I don't think the version matters.

I am testing with an external mouse, with control-left-click, and with the double-finger-click gesture on the trackpad.

I was having problems reproducing on Linux. No problem reproducing on macOS.

On Linux I have some issues too: the test that I attached to the JBS issue doesn't fail for me on Linux, but the test that I added to this PR does fail without the fix.

Can you try the monkey tester? it allows setting properties on the table view / table column at runtime...

I just verified that on Linux the released event usually goes to the popup window. Basically it's whatever window is under the cursor at the time; if I move the mouse fast enough it may go to a node on the primary stage. I don't think this is our doing, it's how GTK is delivering the event.

On the JavaFX side I'm not sure this counts as a bug. The docs state that entered and released events should arrive in pairs during drag and drop operations but doesn't address this case.

The Monkey tester app works for me on Linux (Ubuntu 22.04, either mouse or pad), before and after this PR (so I can't reproduce the issue there, as I mentioned, though the test added to the PR fails without the patch).

…t click on macOS

andy-goryachev-oracle · 2025-04-28T14:31:01Z

Some tests in TableColumnHeaderTest are failing in CI.

jperedadnr · 2025-04-28T17:24:31Z

Indeed... this test fails: TableColumnHeaderTest::testColumnRightClickDoesAllowResizingWhenConsumed.

It has this JavaDoc:

/**
     * When a right click is done on a table column header and consumed by a self added event handler, the column
     * drag lock should be set to true in the pressed handler, but still to false again in the released handler.<br>
     * By that we guarantee, that the column resizing still works.
     */

This was changed based on @beldenfox suggestion. I'll change it back.

jperedadnr · 2025-04-28T18:04:12Z

Tests are green now, except for the Windows one, which fails for the chmodArtifactsSdkWin task, that seems totally unrelated to this PR.

andy-goryachev-oracle · 2025-04-28T18:09:32Z

much better, thank you. btw, you can re-start failing tasks - I hope it's just an intermittent failure.

jperedadnr · 2025-04-28T18:22:18Z

It has failed three times in a row. Most likely this has already been fixed with JDK-8354337, so I'll sync from head.

jperedadnr · 2025-04-28T18:50:45Z

It is green now (https://github.com/jperedadnr/jfx/actions/runs/14715013268) after I ran again the failed gradle wrapper validation.

beldenfox · 2025-04-28T20:03:24Z

This was changed based on @beldenfox suggestion. I'll change it back.

If the mouse pressed event triggers a context menu you might never see the mouse released event. So you have to consider what it means if column drag lock gets set and then never cleared.

The failing test strikes me as odd, it is explicitly testing a bit of internal state to verify that a right-click drag operation will work. Users don't expect this to work and overloading right-click makes life complicated so right-click drags are generally disabled even on Windows. I understand that this test case was added because the table header was getting into a persistent bad state but unless I'm missing something (and I certainly might be) the bug should have been resolved by turning right-click drag into a no-op. (It doesn't help that the test uses the MouseEventFirer which sets the isPopupTrigger flag on both pressed and release which doesn't match the behavior of any platform.)

andy-goryachev-oracle · 2025-04-28T20:38:33Z

I see a difference in behavior between this PR and the master branch. Admittedly, the scenario is weird:

click and drag a column
while dragging, right click

What I sort of expect is the behavior of JTable: the right click completes the drag to reorder gesture.

What I see in the master branch is quite weird (testing on macOS): the drag to reorder gesture completes with the column being repositioned, but the popup menu flashes on and then off as the right mouse button is released.

With this PR (again, on macOS) the behavior is different: the popup appears, at the same time the dragging continues uninterrupted. On Windows, the position of the highlight indicating the column being dragged jumps to the original place after right mouse button is released.

It looks to me as if the original developers did not think of such a scenario (while JTable ones did).

We could deal with this scenario in a separate bug, though I think the Windows case needs to be fixed in this PR. Alternatively, we might want to handle this weird scenario in this PR.

What do you think?

P.S. I can't test on Linux, and we should probably also test in on the latest Ubuntu as it seems to work slightly different?

jperedadnr · 2025-04-28T23:22:49Z

@beldenfox I understand your concerns about columnDragLock and the related tests testColumnRightClickDoesAllowResizing() and testColumnRightClickDoesAllowResizingWhenConsumed(), but if I get it right, this PR doesn't really modify the behaviour of such lock.

So I suggest we take this on a follow up issue, revisiting the need to change the lock from true to false when the popup is triggered, and modifying or removing those tests accordingly.

What do you think?

jperedadnr · 2025-04-28T23:40:44Z

@andy-goryachev-oracle About this edge case, on macOS it seems better behaviour after this PR: before the highlighting jumps unexpectedly, while after it, the highlighted area is updated smoothly. And I don't see the popup flashing.
About Windows, do you mean we should try to reproduce the same behaviour as on macOS? I'm not sure this should be part of this PR (the issue was only on macOS/Linux).

andy-goryachev-oracle · 2025-04-29T14:44:59Z

My immediate thought was we should finish the drag to reorder gesture and avoid showing the popup if the right mouse button is pressed during the drag operation, regardless of the platform (this will change the behavior but I think it makes more sense). I would recommend doing it as a part of this PR, but it's up to you.

Doing this will also avoid the issue we saw on windows where the highlighted column being dragged jumps suddenly to the original position which is simply wrong.

What do you think?

beldenfox · 2025-04-29T15:26:32Z

I think this PR should probably be accepted as-is. It fixes an obvious problem and doesn't introduce any new ones. Someone should enter a follow-up PR citing some of the other problems but most of those have been around for years and only affect users who are looking for trouble (most wouldn't even consider using the right mouse button to drag).

Here you have separate behavior on click vs drag-click and have to deal with two different approaches to context menus. I've tackled this sort of thing before and it usually takes two or four or six tries to get right. To reduce the complexity you have be aggressive at ignoring irrelevant events (no right-click drag!) and avoid setting up state related to dragging until the user actually starts dragging. So this code needs a modest re-write but, again, it doesn't need to happen in this PR.

kevinrushforth · 2025-04-29T15:39:30Z

I think this PR should probably be accepted as-is. It fixes an obvious problem and doesn't introduce any new ones. Someone should enter a follow-up PR citing some of the other problems but most of those have been around for years and only affect users who are looking for trouble ...

I tend to agree, as long as any remaining problems are preexisting and not likely to be encountered in the primary use case.

andy-goryachev-oracle · 2025-04-29T16:35:27Z

The windows problem is pre-existing.

The after-the-fix behavior is slightly better. I'll create the follow-up ticket.

andy-goryachev-oracle · 2025-04-29T16:58:49Z

created https://bugs.openjdk.org/browse/JDK-8355939

Ziad-Mid

LGTM, test fail in master and work with the fix

andy-goryachev-oracle · 2025-05-28T14:39:58Z

This PR is ready to integrate.

jperedadnr · 2025-05-28T14:54:40Z

/integrate

openjdk · 2025-05-28T14:55:01Z

Going to push as commit 7e8eff9.
Since your change was applied there have been 18 commits pushed to the master branch:

9950d33: 8169285: Re-enable javafx.swt tests
59c2b75: 8355740: Update to Xcode 15.4 on macOS
ac12979: 8354940: Fail to sign in to Microsoft sites with WebView
... and 15 more: https://git.openjdk.org/jfx/compare/3fdd21386d6db96294fcecd80afc25d09732c067...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-28T14:55:08Z

@jperedadnr Pushed as commit 7e8eff9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Check popupTrigger on mouse pressed too for tableColumnHeaders

612e345

openjdk Bot added the rfr Ready for review label Apr 1, 2025

andy-goryachev-oracle approved these changes Apr 1, 2025

View reviewed changes

jperedadnr changed the title ~~8207333: [linux] Column sorting is triggered always after context menu request on table header~~ 8207333: [Linux, macOS] Column sorting is triggered always after context menu request on table header Apr 1, 2025

beldenfox reviewed Apr 8, 2025

View reviewed changes

Prevent drag or start sorting on if popup is triggered, test ctrl+lef…

561fcf1

…t click on macOS

openjdk Bot removed the rfr Ready for review label Apr 28, 2025

keep column drag lock

526eb88

remove white space

ad47822

openjdk Bot added the rfr Ready for review label Apr 28, 2025

Merge branch 'master' into 8207333-contextmenusort

bd73bcb

andy-goryachev-oracle approved these changes Apr 29, 2025

View reviewed changes

Ziad-Mid approved these changes May 23, 2025

View reviewed changes

openjdk Bot added the ready Ready to be integrated label May 23, 2025

openjdk Bot added the integrated Pull request has been integrated label May 28, 2025

openjdk Bot closed this May 28, 2025

openjdk Bot removed ready Ready to be integrated rfr Ready for review labels May 28, 2025

		@@ -271,7 +274,10 @@ public TableColumnHeader(final TableColumnBase tc) {
		TableColumnHeader header = (TableColumnHeader) me.getSource();
		header.getTableHeaderRow().columnDragLock = false;

Conversation

jperedadnr commented Apr 1, 2025 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Apr 1, 2025

Uh oh!

openjdk Bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle commented Apr 1, 2025

Uh oh!

andy-goryachev-oracle commented Apr 1, 2025

Uh oh!

openjdk Bot commented Apr 1, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ziad-Mid commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle commented Apr 28, 2025

Uh oh!

jperedadnr commented Apr 28, 2025

Uh oh!

jperedadnr commented Apr 28, 2025

Uh oh!

andy-goryachev-oracle commented Apr 28, 2025

Uh oh!

jperedadnr commented Apr 28, 2025

Uh oh!

jperedadnr commented Apr 28, 2025

Uh oh!

beldenfox commented Apr 28, 2025

Uh oh!

andy-goryachev-oracle commented Apr 28, 2025

jperedadnr commented Apr 1, 2025 •

edited by openjdk Bot

Loading

openjdk Bot commented Apr 1, 2025 •

edited

Loading

mlbridge Bot commented Apr 1, 2025 •

edited

Loading

andy-goryachev-oracle commented Apr 7, 2025 •

edited

Loading