8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true #965

karthikpandelu · 2022-12-01T17:03:03Z

Cause: When slider is dragged for first time after tooltip appears, setOnMousePressed event was not invoked, hence dragStart was null in the subsequently invoked event handler (setOnMouseDragged).

Fix: Initialized dragStart in initialize method.

Test: Added system test to validate the fix.

Note: Initializing dragStart in layoutChildren method as suggested in the bug was causing side effects. Hence initialized dragStart in initialize method.

Progress

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

Issue

JDK-8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true

Reviewers

Ambarish Rapte (@arapte - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Committer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 965

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

Using diff file

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

bridgekeeper · 2022-12-01T17:04:20Z

👋 Welcome back karthikpandelu! 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.

mlbridge · 2022-12-02T05:29:54Z

Webrevs

arapte

Providing few quick comments about the test.

arapte · 2022-12-02T06:17:35Z

tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java

+    public static void main(String[] args) {
+        initFX();
+        try {
+            SliderTooltipNPETest test = new SliderTooltipNPETest();
+            test.testSliderTooltipNPE();
+        } catch (Throwable e) {
+            e.printStackTrace();
+        } finally {
+            exit();
+        }
+    }
+


main() method is not required, can be removed.

arapte · 2022-12-02T06:27:52Z

tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java

+        dragSliderAfterTooltipDisplayed(DRAG_DISTANCE, true);
+    }
+
+    private void dragSliderAfterTooltipDisplayed(int dragDistance, boolean xIncr) throws Throwable {


boolean xIncr is not required for the test. It can be removed along with it's use on line 101.

arapte · 2022-12-02T06:38:11Z

tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java

+            robot.mouseMove((int)(scene.getWindow().getX() + scene.getX() + SCENE_WIDTH/2),
+                (int)(scene.getWindow().getY() + scene.getY() + SCENE_HEIGHT/2));


The x and y position calculation should consider the sliders layout as well.
I think it should be as:

robot.mouseMove((int)(scene.getWindow().getX() + scene.getX() + slider.getLayoutX() + slider.getLayoutBounds().getWidth()/2), (int)(scene.getWindow().getY() + scene.getY() + slider.getLayoutY() + slider.getLayoutBounds().getHeight()/2));

and similar change on line 102.
Line 105 would get removed when removing xIncr

arapte · 2022-12-02T06:45:39Z

tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java

+                (int)(scene.getWindow().getY() + scene.getY() + SCENE_HEIGHT/2));
+        });
+
+        Thread.sleep(3000); // Wait for tooltip to be displayed


This sleep can be removed, The sleep of 1000 ms on line 85 should be sufficient.
I ran by removing this sleep on Mac, Test ran well.
Please check on other platforms.

karthikpandelu · 2022-12-02T11:10:57Z

@arapte , Addressed review comments and added CountDownLatch for tooltip display as discussed.

arapte · 2022-12-02T13:10:37Z

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

@@ -343,6 +343,7 @@ public Object queryAccessibleAttribute(AccessibleAttribute attribute, Object...
        };
        thumb.getStyleClass().setAll("thumb");
        thumb.setAccessibleRole(AccessibleRole.THUMB);
+        dragStart = new Point2D(thumb.getLayoutX(), thumb.getLayoutY());


dragStart should be the exact mouse position where mouse pressed has occurred.
With this change mouse gets moved to the layout X, Y position of thumb, even though the mouse press occurred at any other location inside the thumb.

This change makes sure that dragStart is not null when setOnMouseDragged event handler is called.
As suggested in the issue, I tried to initialize dragStart in layoutChildren method but it was causing side effects.
I checked for other event handlers such as onDragDetected and onMouseDragEntered, but none of these event handlers were invoked. Looks like these events are also consumed by tooltip similar to how it consumes setOnMousePressed.
I haven't seen above mentioned side effect while checking the fix. I will check again.

andy-goryachev-oracle · 2022-12-05T16:57:05Z

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

@@ -389,6 +390,19 @@ public Object queryAccessibleAttribute(AccessibleAttribute attribute, Object...
                    cur.getX() - dragStart.getX() : -(cur.getY() - dragStart.getY());
            behavior.thumbDragged(me, preDragThumbPos + dragPos / trackLength);
        });
+
+        thumb.setOnMouseEntered(me -> {
+            if (getSkinnable().getTooltip() != null && getSkinnable().getTooltip().isAutoHide()) {


If I may suggest an edit in these two event handlers - create a local variable:

Tooltip t = getSkinnable().getTooltip(); if (t != null && t.isAutoHide()) { tooltipConsumeAutoHidingEvents = t.getConsumeAutoHidingEvents(); t.setConsumeAutoHidingEvents(false); }

Also, since the reason for this change is not immediately obvious, I'd add a comment explaining why (perhaps above tooltipConsumeAutoHidingEvents declaration, line 64?)

Added above suggested edit and added comments

tests/system/src/test/java/test/robot/javafx/scene/SliderTooltipNPETest.java

andy-goryachev-oracle · 2022-12-05T17:22:10Z

@aghaisas @karthikpandelu :
could you guys check if this change fixes JDK-8293916 ?
I don't have a touchscreen machine here, and these two bugs seem related.

karthikpandelu · 2022-12-06T11:21:08Z

I do not have a touchscreen machine as well.
@aghaisas please check if bug given above is fixed.

aghaisas · 2022-12-06T12:28:46Z

@aghaisas please check if bug given above is fixed.

I tested the program provided in JDK-8190411 with touch based Windows system. The proposed fix fixes the issue.

For JDK-8293916

The bug does not have a reproducible test program.
It is reported that the bug does not occur on Windows system - and - I have a touch based Windows system.
It is reported that the bug is observed without ToolTip. This indicates that there exists another path to the reported NPE.

My take is on this is - we need to close JDK-8293916 as "incomplete" till a reproducible sample is provided.

andy-goryachev-oracle · 2022-12-06T16:28:06Z

Pre-submit test status Skipped — Testing is not configured

Please enable github actions, so the unit tests can be run automatically.

kevinrushforth

Other than my general concern about not setting the value of a property that is under app control, this looks like it will work, unless the app binds to the property. See my comment inline.

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

karthikpandelu · 2022-12-09T09:35:46Z

Hi All,
All the changes mentioned in the review are now addressed. Please review.

andy-goryachev-oracle

lgtm

andy-goryachev-oracle · 2022-12-09T16:31:57Z

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

@@ -407,7 +407,7 @@ public Object queryAccessibleAttribute(AccessibleAttribute attribute, Object...

        thumb.setOnMouseExited(me -> {
            Tooltip t = getSkinnable().getTooltip();
-            if (t != null && t.isAutoHide()) {
+            if (t != null && t.isAutoHide() && !t.consumeAutoHidingEventsProperty().isBound()) {


I still think a second property or a boolean might be better. What if the app code binds autoHidingEventsProperty after onMouseEntered? I know this is highly unlikely, but there is a potential.

At the same time, I think we should proceed with this fix as it addresses a problem which occurs always.

Agreed. Let's file a follow-on bug to consider the larger question of whether we should change the default value of consumeAutoHidingEventsProperty for Tooltip, so that they will not consume auto-hiding events by default.

I have filed JDK-8298529 for this.

andy-goryachev-oracle

lgtm

kevinrushforth

The fix looks good to me. Please give @arapte time to comment on this, since he had raised earlier concerns.

kevinrushforth · 2022-12-10T15:42:23Z

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

@@ -407,7 +407,7 @@ public Object queryAccessibleAttribute(AccessibleAttribute attribute, Object...

        thumb.setOnMouseExited(me -> {
            Tooltip t = getSkinnable().getTooltip();
-            if (t != null && t.isAutoHide()) {
+            if (t != null && t.isAutoHide() && !t.consumeAutoHidingEventsProperty().isBound()) {


Agreed. Let's file a follow-on bug to consider the larger question of whether we should change the default value of consumeAutoHidingEventsProperty for Tooltip, so that they will not consume auto-hiding events by default.

openjdk · 2022-12-10T15:46:31Z

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

8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true

Reviewed-by: arapte, 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 19 new commits pushed to the master branch:

dadfcaf: 8295339: DatePicker updates its value property with wrong date when dialog closes
9f6ec88: 8295755: Update SQLite to 3.39.4
c900a00: 8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize
6abbe08: 8295809: TreeTableViewSkin: memory leak when changing skin
07e693b: 8245145: Spinner: throws IllegalArgumentException when replacing skin
5b0d3b5: 8296621: Stage steals focus on scene change
f96b350: 8296854: NULL check of CTFontCopyAvailableTables return value is required
6f36e70: 8295175: SplitPaneSkin: memory leak when changing skin
bb13920: 8295531: ComboBoxBaseSkin: memory leak when changing skin
f333662: 8297413: Remove easy warnings in javafx.graphics
... and 9 more: https://git.openjdk.org/jfx/compare/0a785ae036f48c736b65df865a3b93f954d46fe5...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@arapte, @andy-goryachev-oracle, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

arapte

Looks good to me too.

karthikpandelu · 2022-12-12T05:56:14Z

/integrate

openjdk · 2022-12-12T05:58:10Z

@karthikpandelu
Your change (at version 883fcf5) is now ready to be sponsored by a Committer.

arapte · 2022-12-12T06:14:59Z

/sponsor

openjdk · 2022-12-12T06:16:21Z

Going to push as commit 8763e8b.
Since your change was applied there have been 19 commits pushed to the master branch:

dadfcaf: 8295339: DatePicker updates its value property with wrong date when dialog closes
9f6ec88: 8295755: Update SQLite to 3.39.4
c900a00: 8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize
6abbe08: 8295809: TreeTableViewSkin: memory leak when changing skin
07e693b: 8245145: Spinner: throws IllegalArgumentException when replacing skin
5b0d3b5: 8296621: Stage steals focus on scene change
f96b350: 8296854: NULL check of CTFontCopyAvailableTables return value is required
6f36e70: 8295175: SplitPaneSkin: memory leak when changing skin
bb13920: 8295531: ComboBoxBaseSkin: memory leak when changing skin
f333662: 8297413: Remove easy warnings in javafx.graphics
... and 9 more: https://git.openjdk.org/jfx/compare/0a785ae036f48c736b65df865a3b93f954d46fe5...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-12T06:16:43Z

@arapte @karthikpandelu Pushed as commit 8763e8b.

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

karthikpandelu added 2 commits December 1, 2022 21:28

SliderSkin NPE fix

815ccd9

Merge branch 'master' into slider_npe_fix

52cd74a

karthikpandelu added 2 commits December 2, 2022 10:45

Correct line endings

97c4c4c

Correct file permissions

289fda5

openjdk bot added the rfr Ready for review label Dec 2, 2022

arapte suggested changes Dec 2, 2022

View reviewed changes

Address review comments

86e7823

arapte reviewed Dec 2, 2022

View reviewed changes

karthikpandelu added 2 commits December 5, 2022 14:26

Fix for behavioural side effect

104ddb7

Update in mouse enter and exit events. Update in unit test

26150c0

andy-goryachev-oracle suggested changes Dec 5, 2022

View reviewed changes

Address review comment. Add comments for changes

3e4de9c

kevinrushforth reviewed Dec 6, 2022

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/SliderSkin.java Show resolved Hide resolved

Add check whether consumeAutoHidingEventsProperty is bound

883fcf5

andy-goryachev-oracle approved these changes Dec 9, 2022

View reviewed changes

kevinrushforth requested review from kevinrushforth, arapte and andy-goryachev-oracle December 9, 2022 21:49

andy-goryachev-oracle approved these changes Dec 9, 2022

View reviewed changes

kevinrushforth approved these changes Dec 10, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 10, 2022

arapte approved these changes Dec 10, 2022

View reviewed changes

openjdk bot added the sponsor Ready to sponsor label Dec 12, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 12, 2022

openjdk bot closed this Dec 12, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Dec 12, 2022

karthikpandelu deleted the slider_npe_fix branch December 12, 2022 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true #965

8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true #965

karthikpandelu commented Dec 1, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 1, 2022

mlbridge bot commented Dec 2, 2022 •

edited

Loading

arapte left a comment

arapte Dec 2, 2022

arapte Dec 2, 2022

arapte Dec 2, 2022

arapte Dec 2, 2022

karthikpandelu commented Dec 2, 2022

arapte Dec 2, 2022

karthikpandelu Dec 2, 2022

andy-goryachev-oracle Dec 5, 2022

andy-goryachev-oracle Dec 5, 2022

karthikpandelu Dec 6, 2022

andy-goryachev-oracle commented Dec 5, 2022

karthikpandelu commented Dec 6, 2022

aghaisas commented Dec 6, 2022

andy-goryachev-oracle commented Dec 6, 2022

kevinrushforth left a comment

karthikpandelu commented Dec 9, 2022

andy-goryachev-oracle left a comment

andy-goryachev-oracle Dec 9, 2022

kevinrushforth Dec 10, 2022

karthikpandelu Dec 12, 2022

andy-goryachev-oracle left a comment

kevinrushforth left a comment

kevinrushforth Dec 10, 2022

openjdk bot commented Dec 10, 2022 •

edited

Loading

arapte left a comment

karthikpandelu commented Dec 12, 2022

openjdk bot commented Dec 12, 2022

arapte commented Dec 12, 2022

openjdk bot commented Dec 12, 2022

openjdk bot commented Dec 12, 2022

		robot.mouseMove((int)(scene.getWindow().getX() + scene.getX() + SCENE_WIDTH/2),
		(int)(scene.getWindow().getY() + scene.getY() + SCENE_HEIGHT/2));

8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true #965

8190411: NPE in SliderSkin:140 if Slider.Tooltip.autohide is true #965

Conversation

karthikpandelu commented Dec 1, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 1, 2022

mlbridge bot commented Dec 2, 2022 • edited Loading

Webrevs

arapte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikpandelu commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Dec 5, 2022

karthikpandelu commented Dec 6, 2022

aghaisas commented Dec 6, 2022

andy-goryachev-oracle commented Dec 6, 2022

kevinrushforth left a comment

Choose a reason for hiding this comment

karthikpandelu commented Dec 9, 2022

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Dec 10, 2022 • edited Loading

arapte left a comment

Choose a reason for hiding this comment

karthikpandelu commented Dec 12, 2022

openjdk bot commented Dec 12, 2022

arapte commented Dec 12, 2022

openjdk bot commented Dec 12, 2022

openjdk bot commented Dec 12, 2022

karthikpandelu commented Dec 1, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 2, 2022 •

edited

Loading

openjdk bot commented Dec 10, 2022 •

edited

Loading