-
Notifications
You must be signed in to change notification settings - Fork 522
8237469: Inherited styles don't update when node is moved #87
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
Conversation
👋 Welcome back dwookey! A progress list of the required criteria for merging this PR into |
Webrevs
|
I mentioned this in the JBS bug as well: I suspect that JDK-8234877 has the same root cause as this one. Can you run the HelloCSS test program as described in that bug with your patch from this PR and see if it also fixes that bug? |
I ran it and it seems to work. The button is always red in the red pane and always yellow in the yellow one. |
…per can return true
@DeanWookey This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type
Since the source branch of this PR was last updated there have been 23 commits pushed to the 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 (@dsgrieve, @aghaisas, @kevinrushforth) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/reviewers 3 |
@kevinrushforth |
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
Outdated
Show resolved
Hide resolved
I updated the PR title to match the JBS title, so that the commit message will be correct when this PR is integrated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I appreciate all of the added unit tests. I also confirmed that this fixed JDK-8234877, so I will close that one as a duplicate.
This is now ready to integrate. Either Ajit or I can sponsor it.
/integrate |
@DeanWookey |
/sponsor |
@kevinrushforth @DeanWookey The following commits have been pushed to master since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit 6968e38. |
Mailing list message from Kevin Rushforth on openjfx-dev: Changeset: 6968e38 8237469: Inherited styles don't update when node is moved Reviewed-by: dgrieve, aghaisas, kcr ! modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java |
Everything passes with the fix and 5 of the new tests fail without the fix.
removingThenAddingNodeToDifferentBranchGetsNewFontStyleTest
movingBranchToDifferentBranchGetsNewCssVariableTest
removingThenAddingNodeToDifferentBranchGetsCorrectInheritedValue
removingThenAddingNodeToDifferentBranchGetsIneritableStyle
movingNodeToDifferentBranchGetsNewFontStyleTest
I doubt this will cause a significant performance decrease. I think it's worth it for correctness. The only other thing is that I kept the fix to the minimum, but technically canReuseHelper now has a side effect. I could try rewrite some things if necessary, or maybe rename the method? Else leave it as is?
Originally introduced here: https://bugs.openjdk.java.net/browse/JDK-8090462/ 834b0d0#diff-9ec098280fa1aeed53c70243347e76ab
Progress
Issue
JDK-8237469: Inherited styles don't update when node is moved
Approvers