jfx
jfx copied to clipboard
openjdk
8296387: [Tooltip, CSS] -fx-show-delay is only applied to the first tooltip that is shown before it is displayed
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 tests for the behaviour since there were none before.
Progress
- [x] Change must not contain extraneous whitespace
- [x] 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
- Andy Goryachev (@andy-goryachev-oracle - Reviewer)
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
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 11: Full - Incremental (26ec665f)
- 10: Full - Incremental (84f2faae)
- 09: Full - Incremental (f917d18e)
- 08: Full - Incremental (6abc47b1)
- 07: Full - Incremental (bd57e79d)
- 06: Full - Incremental (96310dbe)
- 05: Full - Incremental (dc24b7f7)
- 04: Full - Incremental (a3f9f609)
- 03: Full - Incremental (5af893c1)
- 02: Full - Incremental (093b36f6)
- 01: Full - Incremental (a4a1a49f)
- 00: Full (6d40b527)
@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 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 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.
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.
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
the disappearing tooltip clip (will move to its own ticket later):
https://github.com/openjdk/jfx/assets/107069028/02589ed0-d359-4811-8843-de1109fd23c9
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
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.
Perhaps we should open an issue to solve this for all controls in one go? A solution that would prevent any
Nodefrom 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.
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)
@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:
- 1c6ef3638057e05565d83c79acbfad83046b12c7: 8335934: Change JavaFX release version to 24
- a41dcf3cb7259af0b5feac404aa94c3c1b247460: 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.
@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!
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
@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!
Strange: I still see the same issue described in https://github.com/openjdk/jfx/pull/1394#issuecomment-1986520680 (the very first tooltip after updating stylesheet uses the old value)
Strange: I still see the same issue described in #1394 (comment) (the very first tooltip after updating stylesheet uses the old value)
I don't know what is happening there, are you sure that there isn't a problem on your side? Testing Tooltip with a normal stylesheet added to the scene works perfectly fine for me.
I am not sure what the difference might be. The code in the MonkeyTester generates a stylesheet, encodes it in base64 data url and adds it to all scenes in the application (it also removes the old version, if any).
The stylesheet gets loaded - I can see it by slightly changed colors and the fact that the tooltip show delay gets changed on the second and any subsequent invocations.
Can you try it? https://github.com/andy-goryachev-oracle/MonkeyTest
with the latest monkey tester I see that updating the stylesheet does not update the showingDelay property immediately. When the tooltip gets shown, I see the following output from the change listener added to this property:
showDelay=1000.0 ms
showDelay=100.0 ms
I suppose the second setting of 100ms (the value I actually set) happens too late or simply is ignored.
with the latest monkey tester I see that updating the stylesheet does not update the showingDelay property immediately. When the tooltip gets shown, I see the following output from the change listener added to this property:
showDelay=1000.0 ms showDelay=100.0 msI suppose the second setting of 100ms (the value I actually set) happens too late or simply is ignored.
Ah, I think I know why. Same problem, we copy the stylesheet right before showing, so this is updated to late once again. We may need to do the update stuff always before showing.
Question: Since I add a new protected method to PopupWindow (applyStylesheetFromOwner), this needs a CSR, right?
Another approach would be to access this method via a Helper class, in this case maybe: PopupWindowHelper.
in this case maybe:
PopupWindowHelper.
good point! using a helper class would avoid CSR.
question is - do we really need to expose this method (for custom popup windows, for example), or not?
I just took a quick look and yes, without changes, this would need a CSR. Using a Helper method would be one way to avoid it.
Also, I think a second reviewer would be good here.
/reviewers 2
@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).
question is - do we really need to expose this method (for custom popup windows, for example), or not?
I can not really see why this is needed, other than maybe to not copy any style from the owner. It is hard to guess a usecase, so hiding this for now is better.
TIL how we can test things that involves animations like Timeline. Wrote a lot of unit tests for Tooltip as well now.
The new system test isn't stable on Windows. I ran it several times, and most of the time I see one or two failures. Here are the results from a couple of the test runs:
Test run 1:
TooltipTest > testShowDelayCssShowTooltipTwice() FAILED
org.opentest4j.AssertionFailedError: 193 <= 150 ==> expected: <true> but was: <false>
TooltipTest > testSmallShowDelayCssTooltip() FAILED
org.opentest4j.AssertionFailedError: 160 <= 150 ==> expected: <true> but was: <false>
Test run 2:
TooltipTest > testShowDelayCssShowTooltipTwice() FAILED
org.opentest4j.AssertionFailedError: 165 <= 150 ==> expected: <true> but was: <false>
TooltipTest > testChangeShowDelayTooltip() FAILED
org.opentest4j.AssertionFailedError: 151 <= 150 ==> expected: <true> but was: <false>
This is likely due to either inaccuracies in the timer itself or in measuring the elapsed time.