Skip to content

8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed #1394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from

Conversation

Maran23
Copy link
Member

@Maran23 Maran23 commented Mar 8, 2024

This PR fixes a long standing issue where the Tooltip will always wait one second until it appears the very first time, even if the
-fx-show-delay was set to another value.

The culprit is, that the cssForced flag is not inside Tooltip, but inside the TooltipBehaviour. So the very first Tooltip gets processed correctly, but after no Tooltip will be processed by CSS before showing, resulting in the set -fx-show-delay to not be applied immediately.

Added a bunch of headful and headless tests for the behaviour since there were none before.


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-8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1394

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 8, 2024

👋 Welcome back mhanl! 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 openjdk bot added the rfr Ready for review label Mar 8, 2024
@mlbridge
Copy link

mlbridge bot commented Mar 8, 2024

@Maran23 Maran23 force-pushed the 8296387-tooltip-css branch from 6d40b52 to f0c5224 Compare March 8, 2024 16:03
@openjdk
Copy link

openjdk bot commented Mar 8, 2024

@Maran23 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@Maran23 Maran23 force-pushed the 8296387-tooltip-css branch from f0c5224 to cd9892b Compare March 8, 2024 16:04
@openjdk
Copy link

openjdk bot commented Mar 8, 2024

@Maran23 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

…st tooltip that is shown before it is displayed
@Maran23 Maran23 force-pushed the 8296387-tooltip-css branch from cd9892b to a4a1a49 Compare March 8, 2024 16:05
@openjdk
Copy link

openjdk bot commented Mar 8, 2024

@Maran23 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@andy-goryachev-oracle
Copy link
Contributor

Testing the combined (this PR merged with the latest master) on macOS 14.3.1 M1 Pro (though this is probably not important), using the latest MonkeyTester
https://github.com/andy-goryachev-oracle/MonkeyTest

  • select Tooltip page
  • open the Tools -> CSS Playground tool
  • type .tooltip { -fx-show-delay:1ms; } into Custom CSS text area, press Update button. (this updates the stylesheet in all the open windows)
  • hover over the tooltip page

What I see is the very first time the tooltip appears it's using the old show-delay value of 1000ms. Any subsequent attempts the new delay is in effect.

Then, change back to 1000ms. Same thing happens: the first time the old timeout value is used instead of the new.

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Mar 8, 2024

There is also another problem (though it is a separate issue as it can be seen with the master branch):

  • set the -fx-show-delay to 1ms as described in the previous comment
  • set the Graphic: to "Image"

Notice how sometimes, when slowly entering the tooltip page from the left side, the tooltip appears and then disappears. Async code is hard!

edit 2024/04/12: created https://bugs.openjdk.org/browse/JDK-8330187

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Mar 8, 2024

the disappearing tooltip clip (will move to its own ticket later):

video1956899183.mp4

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Mar 8, 2024

This is another, unrelated issue: If I specify a number instead of duration like -fx-show-delay: 1; we'll get an exception every time the tooltip is about to be shown.

Exception in thread "JavaFX Application Thread" java.lang.ClassCastException: class java.lang.Double cannot be cast to class javafx.util.Duration (java.lang.Double is in module java.base of loader 'bootstrap'; javafx.util.Duration is in module javafx.base of loader 'app')
	at javafx.controls/javafx.scene.control.Tooltip.getShowDelay(Tooltip.java:334)
	at javafx.controls/javafx.scene.control.Tooltip$TooltipBehavior.lambda$0(Tooltip.java:1015)
	at javafx.base/com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:247)
	at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
	at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:232)
	at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:189)
	at javafx.base/com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
	at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
	at javafx.base/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
	at javafx.base/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
	at javafx.base/javafx.event.Event.fireEvent(Event.java:199)
	at javafx.graphics/javafx.scene.Scene$MouseHandler.process(Scene.java:3987)
	at javafx.graphics/javafx.scene.Scene.processMouseEvent(Scene.java:1893)
	at javafx.graphics/javafx.scene.Scene$ScenePeerListener.mouseEvent(Scene.java:2711)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:411)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$MouseEventNotification.run(GlassViewEventHandler.java:1)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$2(GlassViewEventHandler.java:450)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:430)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.handleMouseEvent(GlassViewEventHandler.java:449)
	at javafx.graphics/com.sun.glass.ui.View.handleMouseEvent(View.java:551)
	at javafx.graphics/com.sun.glass.ui.View.notifyMouse(View.java:937)
	at javafx.graphics/com.sun.glass.ui.mac.MacView.notifyMouse(MacView.java:128)

Once that happens, the exception will get thrown every time, even after the correct CSS is entered. The app must be restarted.

But we don't get an exception for a boolean value: -fx-wrap-text:"yo"; or a double value like .tooltip { -fx-graphic-text-gap:"true"; }. The latter will correctly issue a warning

WARNING: Failed to set css [-fx-graphic-text-gap] on [DoubleProperty [bean: javafx.scene.control.Tooltip@468235b3, name: graphicTextGap, value: 4.0]] due to 'class java.lang.Boolean cannot be cast to class java.lang.Number (java.lang.Boolean and java.lang.Number are in module java.base of loader 'bootstrap')'

edit 2024/11/12: created https://bugs.openjdk.org/browse/JDK-8330186

Copy link
Collaborator

@hjohn hjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the change is an improvement, I think a similar problem will still be present when CSS changes. The cssForced flag I think needs to reset to false whenever a CSS styleable property of the tool tip is modified. That would include all such properties...

This is a bit of a general issue in JavaFX (it really shouldn't be possible for any Node to be shown that hasn't had CSS processed for it), yet this can easily happen, and depending on how different the default style is versus the intended style, this can lead to a visual artifact (most noticable is for example a white background, when it is supposed to be black or transparent).

Perhaps we should open an issue to solve this for all controls in one go? A solution that would prevent any Node from being displayed without CSS applied?

@Maran23
Copy link
Member Author

Maran23 commented Mar 9, 2024

What I see is the very first time the tooltip appears it's using the old show-delay value of 1000ms. Any subsequent attempts the new delay is in effect.

Then, change back to 1000ms. Same thing happens: the first time the old timeout value is used instead of the new.

There seems to be a difference when using setStyle and a global stylesheet. I'm investigating.

@Maran23
Copy link
Member Author

Maran23 commented Mar 9, 2024

Perhaps we should open an issue to solve this for all controls in one go? A solution that would prevent any Node from being displayed without CSS applied?

Yes, that sounds like a good idea. Checking the calls to applyCss, it is mostly in table related classes and I remember that I already asked myself the question whether they are really needed there.

Although the change is an improvement, I think a similar problem will still be present when CSS changes. The cssForced flag I think needs to reset to false whenever a CSS styleable property of the tool tip is modified. That would include all such properties...

Will check. Tooltip is unfortunately a corner case where we to get some CSS properties BEFORE we show it in order to correctly setup the animation timeline.

…tylesheets are considered for the e.g. tooltip show-delay
@Maran23
Copy link
Member Author

Maran23 commented Mar 9, 2024

This is another, unrelated issue: If I specify a number instead of duration like -fx-show-delay: 1; we'll get an exception every time the tooltip is about to be shown.

This is because the CSS implementation thinks it is a PX value and therefore parse it wrong. You can check the CssParser to see why this happens. Fix might be simple, but the code in the parser is very error prone for this kind of stuff.

(And Note that the StyleableProperty does not do any typecheck, therefore a double value can be set where a Duration would be expected, which is very very bad)

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

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

8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed

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

  • 1c6ef36: 8335934: Change JavaFX release version to 24
  • a41dcf3: 8336110: Update copyright header for files modified in 2024

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot changed the title JDK-8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed 8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed Apr 10, 2024
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 10, 2024

@Maran23 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!

@Maran23
Copy link
Member Author

Maran23 commented Apr 10, 2024

keep it open.

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Apr 11, 2024

Please sync up with the latest master (should be a SOP for any long outstanding PRs - sorry for long delay!)

Description	Resource	Type	Path	Location
The method shutdown() in the type Util is not applicable for the arguments (Stage)	TooltipTest.java	Java Problem	/systemTests-test/java/test/robot/javafx/scene	line 238

^ please remove the 'stage' argument, see JDK-8325566

@bridgekeeper
Copy link

bridgekeeper bot commented May 11, 2024

@Maran23 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
Copy link
Contributor

Strange: I still see the same issue described in
#1394 (comment)
(the very first tooltip after updating stylesheet uses the old value)

@andy-goryachev-oracle
Copy link
Contributor

org.opentest4j.AssertionFailedError: 193 <= 150 ==> expected: but was:

I don't see testShowDelayCssShowTooltipTwice() checking for the value of 150...
Where did it come from?

@kevinrushforth
Copy link
Member

org.opentest4j.AssertionFailedError: 193 <= 150 ==> expected: but was:

I don't see testShowDelayCssShowTooltipTwice() checking for the value of 150... Where did it come from?

It's 100 (expected value) plus 50 (maximum delta). Here is the call stack:

L72:         assertTrue(tooltipShowTime <= maximumTime, tooltipShowTime + " <= " + maximumTime);
L64:         assertTooltipShowDelay(tooltipShowTime, expectedTime, 50);
L209:        assertTooltipShowDelay(tooltipShowTime, 100);

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few inline comments, including a possible suggestion to fix the intermittent test failures.

@Maran23
Copy link
Member Author

Maran23 commented Jul 10, 2024

The Tooltip timing looks much better now in the tests. I decided to use 80ms as a threshhold. In my testing, I got a maximum difference of ~35 ms.

Also merged in master.

@kevinrushforth
Copy link
Member

The Tooltip timing looks much better now in the tests. I decided to use 80ms as a threshhold. In my testing, I got a maximum difference of ~35 ms.

Also merged in master.

Thanks. I'll test and re-review.

@kevinrushforth
Copy link
Member

I ran it on our CI Linux and macOS systems and it passed on all of them. It also passed on my local Linux system.

On my Windows machine I still get the following test failure almost every time I run it:

TooltipTest > testShowDelayCssShowTooltipTwice() FAILED
    org.opentest4j.AssertionFailedError: 260 <= 180 ==> expected: <true> but was: <false>
        at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
        at app//org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
        at app//org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:210)
        at app//test.robot.javafx.scene.TooltipTest.assertTooltipShowDelay(TooltipTest.java:69)
        at app//test.robot.javafx.scene.TooltipTest.testShowDelayCssShowTooltipTwice(TooltipTest.java:202)

I've seen the actual result as high as 390 one time. The other times were in the 200 - 260 range.

Have you run it on Windows? If it's only my system, we could file a bug and look at it later. We might consider skipping that test on Windows using @Ignore and referencing that follow-on bug.

@Maran23
Copy link
Member Author

Maran23 commented Jul 11, 2024

Have you run it on Windows?

Yes, on Windows 10 and Windows 11. Executed the test multiple time now again, always around 110 - 140 ms for me. Weird.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 11, 2024

With your latest change, the test now passes on my Windows 11 system. Maybe the resetting of tooltipStartTime and tooltipShownTime fixed it? I'll look more closely in a bit.

@Maran23
Copy link
Member Author

Maran23 commented Jul 11, 2024

With your latest change, the test now passes on my Windows 11 system. Maybe the resetting of tooltipStartTime and tooltipShownTime fixed it? I'll look more closely in a bit.

I would not expect that, but maybe it has something to do with it.
I did this change so that the test is more safe in case something is not triggered the way it should. But maybe it had a positive side effect, although not 100% sure.

@kevinrushforth
Copy link
Member

I looked at the code, and you're right, it doesn't seem like it would have mattered. Regardless, it now passes on my system reliably. I also reverted your fix and verified that the new test catches the failures. So all good from my point of view.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.
I'll create the tickets for creating utility methods that use base64 urls and system nano time.

@openjdk openjdk bot added the ready Ready to be integrated label Jul 11, 2024
@kevinrushforth
Copy link
Member

@Maran23 This is ready to integrate. If you can do it in the next 30 minutes, it will make JavaFX 23 before the RDP1 fork. If it is after the fork, it might be a candidate to backport to jfx23.

@Maran23
Copy link
Member Author

Maran23 commented Jul 11, 2024

If you can do it in the next 30 minutes, it will make JavaFX 23 before the RDP1 fork. If it is after the fork, it might be a candidate to backport to jfx23.

Damn, missed the time. I will keep it open until tomorrow, in case someone else want to review or just check the PR.
If we consider this a candidate for backporting, then I can certainly do the request after merging as well! :)

@Maran23
Copy link
Member Author

Maran23 commented Jul 12, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Jul 12, 2024

Going to push as commit 5b448b8.
Since your change was applied there have been 3 commits pushed to the master branch:

  • 0ce4e6f: 8335218: Eclipse Config: Remove Gradle Integration
  • 1c6ef36: 8335934: Change JavaFX release version to 24
  • a41dcf3: 8336110: Update copyright header for files modified in 2024

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 12, 2024
@openjdk openjdk bot closed this Jul 12, 2024
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jul 12, 2024
@openjdk
Copy link

openjdk bot commented Jul 12, 2024

@Maran23 Pushed as commit 5b448b8.

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

@Maran23 Maran23 deleted the 8296387-tooltip-css branch August 11, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants