8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs #1486

jperedadnr · 2024-06-24T18:05:09Z

This PR considers the horizontal scroll events that can be generated on a trackpad, on an horizontally sided TabPane control (top or bottom), adding to the existing vertical scroll events from mouse wheel and trackpad, to scroll its tabs.

Therefore, scrolling a TabPane will behave in the same way that ScrollBar does regarding those same events and control orientation (vertical/horizontal).

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-8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs (Enhancement - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1486

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

Using diff file

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

Webrev

Link to Webrev Comment

…ntrol

bridgekeeper · 2024-06-24T18:05:25Z

👋 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 · 2024-06-24T18:06:26Z

@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:

8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs

Reviewed-by: angorya, kcr

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 30 new commits pushed to the master branch:

ffbd12d: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet
d538c31: 8336882: Convert CssStyleHelperTest to use JUnit 5
06ac167: 8336798: DRT test cssrounding.html test for linear layout fails with WebKit 619.1
33d82c0: 8335548: testCookieEnabled fails with WebKit 619.1
1117c15: 8336592: Wrong type in documentation for TreeTableView
81f11c4: 8335630: Crash if Platform::exit called with fullScreen Stage on macOS 14
34bbf85: 8328994: Update WebKit to 619.1
e3c1595: 8336272: SizeToSceneTest: fullScreen tests fail on Ubuntu 22.04
5b448b8: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed
0ce4e6f: 8335218: Eclipse Config: Remove Gradle Integration
... and 20 more: https://git.openjdk.org/jfx/compare/6a586b662592be3eb81670f0c5ce48061c2fc07c...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 · 2024-06-24T18:09:37Z

Webrevs

andy-goryachev-oracle · 2024-06-24T19:37:02Z

Considering that this is an improvement and a change in behavior, I would like to suggest a couple of changes:

For Side.LEFT/RIGHT, reverse the direction. What we have now is inconsistent with the movement of e.g. ListView (can be seen if using track pad with the list view and TabPane in the Monkey Tester):

Since now the TabPane movement correctly reflects the orientation of track pad gesture, we need to reverse the movement when TabPane effective nodeOrientation is RTL.
We don't have the behavior documented currently (some initial docs can be found in /doc-files/behavior). Maybe it's a good opportunity at least start a file for TabPane?

And, a question - does this change require a CSR, since we are changing the behavior?

kevinrushforth · 2024-06-24T22:11:11Z

And, a question - does this change require a CSR, since we are changing the behavior?

Good question. Thinking out loud, if what this PR does is to change the existing behavior of top/bottom TabPane scrolling (e.g., horizontal scrollbar) to align with what ScrollPane already does, then... maybe not? (a CSR wouldn't be a bad idea, though)

I'd be happy to hear others thoughts on this.

It does need two reviewers.

/reviewers 2

openjdk · 2024-06-24T22:13:11Z

@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).

kevinrushforth · 2024-06-24T22:17:13Z

For Side.LEFT/RIGHT, reverse the direction. What we have now is inconsistent with the movement of e.g. ListView (can be seen if using track pad with the list view and TabPane in the Monkey Tester):

I don't recommend doing that as part of this PR. That would be a noticeable behavior change, and warrants more discussion.

Since now the TabPane movement correctly reflects the orientation of track pad gesture, we need to reverse the movement when TabPane effective nodeOrientation is RTL.

Do ScrollPane or ListView work this way already? If they do, then it seems reasonable to do the same here. Otherwise, I'm not sure we should do this for TabPane in isolation.

andy-goryachev-oracle · 2024-06-24T23:11:39Z

I don't recommend doing that as part of this PR. That would be a noticeable behavior change, and warrants more discussion

we are changing the behavior already by changing the direction of the gesture, so I thought we should be able to tackle this issue within the same PR.

The other point is the discussion - these comments are already on the mailing list, what alternative discussion is required?

And what are the acceptance criteria for that discussion?

andy-goryachev-oracle · 2024-06-24T23:15:53Z

Do ScrollPane or ListView work this way already?

Good point - both have the same issue (I am sure other controls as well, TableView, others did not check). Definitely deserves a separate ticket, possibly even an umbrella task (RTL navigation).

andy-goryachev-oracle · 2024-06-24T23:29:07Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java

                Side side = getSkinnable().getSide();
                side = side == null ? Side.TOP : side;
                switch (side) {
                    default:
                    case TOP:
                    case BOTTOM:
-                        setScrollOffset(scrollOffset + e.getDeltaY());
+                        // Consider vertical scroll events (dy > dx) from mouse wheel and trackpad,
+                        // and horizontal scroll events from a trackpad (dx > dy)


Maybe we should explain why - to preserve the existing behavior with the mouse (scrolling up/down)?

The trackpad events can have both dx and dy of various magnitude, both non-zero, so the logic seems to me a bit contrived - like filtering some events, although at the end it still feels ok.

What do you think?

(sorry I missed to reply)

I believe the comment already states what are we taking into account, but this is a more detailed explanation:

a mouse wheel only has vertical scroll events (dx == 0, so abs(dy) > abs(dx)), then we just take dy to produce the horizontal scroll offset.

a trackpad has both scroll events, and we take whichever is bigger. Typically the user will do a horizontal scrolling if tabs are horizontally laid out, and therefore abs(dy) <<< abs(dx), so we take dx to produce the horizontal scroll offset, ignoring the smaller dy (that wouldn't have any effect in any case).

I'm happy to reword the comment if needed.

Thank you!

A larger question is that ideally we would need a mechanism to differentiate trackpad from mouse events, as they sometimes require different handling. We can't fully rely on the fact that either dx or dy is exactly 0.0, since theoretically we could get those events from the trackpad.

What do you think?

In this case, the bigger value from the scrolling gesture prevails, ignoring the other small one (it doesn't matter if it is exactly 0 or not).

For theTabPane case, that is good enough, since we only want to scroll in one direction, and it really doesn't matter if the source is a mouse wheel or a trackpad.

bridgekeeper · 2024-07-23T23:44:10Z

@jperedadnr This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle

I am ok with fixing the direction of vertical scrolling in a separate ticket, as well as RTL reversal of the horizontal scrolling, also as a separate ticket.

Left a couple of minor comments, otherwise looks good. I'll re-approve if you decide to make changes.

andy-goryachev-oracle · 2024-07-24T21:06:10Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TabPaneTest.java

+            assertEquals(firstTabBounds.getMinY() - deltaY, newFirstTabBounds.getMinY(), 0);
+        }
+    }
+


please remove unnecessary newline

kevinrushforth

LGTM

I ran the program attached to the bug report and it now recognizes horizontal scroll as expected. I also verified that the newly added test fails without your fix and passes with your fix.

jperedadnr · 2024-07-25T18:28:22Z

/integrate

openjdk · 2024-07-25T18:28:48Z

Going to push as commit 1bdb93c.
Since your change was applied there have been 31 commits pushed to the master branch:

25ac6fe: 8319779: SystemMenu: memory leak due to listener never being removed
ffbd12d: 8336097: UserAgent Styles using lookups are promoted to Author level if look-up is defined in Author stylesheet
d538c31: 8336882: Convert CssStyleHelperTest to use JUnit 5
06ac167: 8336798: DRT test cssrounding.html test for linear layout fails with WebKit 619.1
33d82c0: 8335548: testCookieEnabled fails with WebKit 619.1
1117c15: 8336592: Wrong type in documentation for TreeTableView
81f11c4: 8335630: Crash if Platform::exit called with fullScreen Stage on macOS 14
34bbf85: 8328994: Update WebKit to 619.1
e3c1595: 8336272: SizeToSceneTest: fullScreen tests fail on Ubuntu 22.04
5b448b8: 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed
... and 21 more: https://git.openjdk.org/jfx/compare/6a586b662592be3eb81670f0c5ce48061c2fc07c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-25T18:29:02Z

@jperedadnr Pushed as commit 1bdb93c.

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

Consider horizontal scroll events on an horizontally sided tabPane co…

9e6b89c

…ntrol

openjdk bot added the rfr Ready for review label Jun 24, 2024

andy-goryachev-oracle reviewed Jun 24, 2024

View reviewed changes

jperedadnr added 2 commits June 25, 2024 13:39

Add unit tests

2046b04

Add tests for all four sides

2c4914c

kevinrushforth self-requested a review July 22, 2024 15:02

andy-goryachev-oracle approved these changes Jul 24, 2024

View reviewed changes

remove extra new line

a8969ce

kevinrushforth approved these changes Jul 25, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Jul 25, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jul 25, 2024

openjdk bot added the integrated Pull request has been integrated label Jul 25, 2024

openjdk bot closed this Jul 25, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs #1486

8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs #1486

jperedadnr commented Jun 24, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 24, 2024

openjdk bot commented Jun 24, 2024 •

edited

Loading

mlbridge bot commented Jun 24, 2024 •

edited

Loading

andy-goryachev-oracle commented Jun 24, 2024

kevinrushforth commented Jun 24, 2024

openjdk bot commented Jun 24, 2024

kevinrushforth commented Jun 24, 2024

andy-goryachev-oracle commented Jun 24, 2024

andy-goryachev-oracle commented Jun 24, 2024

andy-goryachev-oracle Jun 24, 2024

jperedadnr Jul 24, 2024

andy-goryachev-oracle Jul 24, 2024

jperedadnr Jul 24, 2024

bridgekeeper bot commented Jul 23, 2024

andy-goryachev-oracle left a comment

andy-goryachev-oracle Jul 24, 2024

jperedadnr Jul 24, 2024

kevinrushforth left a comment

jperedadnr commented Jul 25, 2024

openjdk bot commented Jul 25, 2024

openjdk bot commented Jul 25, 2024

8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs #1486

8334874: Horizontal scroll events from touch pads should scroll the TabPane tabs #1486

Conversation

jperedadnr commented Jun 24, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 24, 2024

openjdk bot commented Jun 24, 2024 • edited Loading

mlbridge bot commented Jun 24, 2024 • edited Loading

Webrevs

andy-goryachev-oracle commented Jun 24, 2024

kevinrushforth commented Jun 24, 2024

openjdk bot commented Jun 24, 2024

kevinrushforth commented Jun 24, 2024

andy-goryachev-oracle commented Jun 24, 2024

andy-goryachev-oracle commented Jun 24, 2024

andy-goryachev-oracle Jun 24, 2024

Choose a reason for hiding this comment

jperedadnr Jul 24, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle Jul 24, 2024

Choose a reason for hiding this comment

jperedadnr Jul 24, 2024

Choose a reason for hiding this comment

bridgekeeper bot commented Jul 23, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle Jul 24, 2024

Choose a reason for hiding this comment

jperedadnr Jul 24, 2024

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

jperedadnr commented Jul 25, 2024

openjdk bot commented Jul 25, 2024

openjdk bot commented Jul 25, 2024

jperedadnr commented Jun 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 24, 2024 •

edited

Loading

mlbridge bot commented Jun 24, 2024 •

edited

Loading