8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked #978

lukostyra · 2022-12-19T15:50:19Z

Creating a not-displayed node and then modifying its contents caused JFX to not consume its old dirty region and thus not update it. When such node was displayed, its old dirty region was used for drawing, which in some cases (ex. new content taking more space - a Label having more text as in bug request) caused it to clip.

Resolved by always unionizing dirty regions with new bounds when calculating Node's transformed bounds.

Change was tested on macOS and Windows 10 and does not affect any tests.

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-8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked

Reviewers

Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 978

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

Using diff file

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

…cked Creating a not-displayed node and then modifying its contents caused JFX to not consume its old dirty region and thus not update it. When such node was displayed, its old dirty region was used for drawing, which in some cases (ex. new content taking more space) caused it to clip. Resolved by always unionizing dirty regions with new bounds.

bridgekeeper · 2022-12-19T15:50:40Z

👋 Welcome back lkostyra! 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-19T15:55:32Z

Webrevs

01: Full - Incremental (f5348154)
00: Full (1fa43f3b)

kevinrushforth · 2022-12-19T16:53:31Z

This looks like the right fix. @arapte can you review?

andy-goryachev-oracle · 2022-12-19T16:55:14Z

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGNode.java

        } else {
            // TODO I think this is vestigial from Scenario and will never
            // actually occur in real life... (RT-23956)
            dirtyBounds = dirtyBounds.deriveWithUnion(transformedBounds);
        }
+        dirtyBounds = dirtyBounds.deriveWithUnion(bounds);


Is it possible to create a unit test that fails with unmodified code but passes after the fix?

Given the nature of the failure, an automated test would be difficult at best. Given the simplicity of the fix, testing it manually might be the best approach.

arapte

The change looks good.
Verified the computed dirtyBounds with and without the change. They look as expected.
I shall test more with some apps(Ensemble).
Providing one minor suggestion.

arapte · 2022-12-21T13:38:31Z

modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGNode.java

            // TODO I think this is vestigial from Scenario and will never
            // actually occur in real life... (RT-23956)


The comments should be removed and updated to explain the scenario when flow enters the else block and I think RT-23956 can be closed with this fix.

I updated the comment. I agree, RT-23956 can now be closed.

arapte

LGTM

openjdk · 2023-01-05T08:34:41Z

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

8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked

Reviewed-by: arapte

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

94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
a35c3bf: 8292922: [Linux] No more drag events when new Stage is created in drag handler
48f6f5b: 8299272: Update copyright header for files modified in 2022
5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
1d9e2af: 8297068: Update boot JDK to 19.0.1
ac3f60c: 8297554: Remove Scene.KeyHandler
9c52605: 8252863: Spinner keeps spinning if removed from Scene
bac8ee8: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed

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.

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

lukostyra · 2023-01-05T09:43:46Z

/integrate

openjdk · 2023-01-05T09:45:34Z

@lukostyra
Your change (at version f534815) is now ready to be sponsored by a Committer.

arapte · 2023-01-05T10:26:05Z

/sponsor

openjdk · 2023-01-05T10:27:29Z

Going to push as commit 0dbc448.
Since your change was applied there have been 9 commits pushed to the master branch:

94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
a35c3bf: 8292922: [Linux] No more drag events when new Stage is created in drag handler
5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
1d9e2af: 8297068: Update boot JDK to 19.0.1
ac3f60c: 8297554: Remove Scene.KeyHandler
9c52605: 8252863: Spinner keeps spinning if removed from Scene
bac8ee8: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-05T10:27:49Z

@arapte @lukostyra Pushed as commit 0dbc448.

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

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

kevinrushforth requested a review from arapte December 19, 2022 16:52

andy-goryachev-oracle reviewed Dec 19, 2022

View reviewed changes

arapte suggested changes Dec 21, 2022

View reviewed changes

NGNode: Change comment at dirty bounds update for clarification

f534815

arapte approved these changes Jan 5, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 5, 2023

openjdk bot added the sponsor Ready to sponsor label Jan 5, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 5, 2023

openjdk bot closed this Jan 5, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Jan 5, 2023

lukostyra deleted the JDK-8231864-label-dirty-region branch February 8, 2023 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked #978

8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked #978

lukostyra commented Dec 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 19, 2022

mlbridge bot commented Dec 19, 2022 •

edited

Loading

kevinrushforth commented Dec 19, 2022

andy-goryachev-oracle Dec 19, 2022

kevinrushforth Dec 19, 2022

arapte left a comment

arapte Dec 21, 2022

lukostyra Jan 2, 2023

arapte left a comment

openjdk bot commented Jan 5, 2023

lukostyra commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

arapte commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

		// TODO I think this is vestigial from Scenario and will never
		// actually occur in real life... (RT-23956)

8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked #978

8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked #978

Conversation

lukostyra commented Dec 19, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 19, 2022

mlbridge bot commented Dec 19, 2022 • edited Loading

Webrevs

kevinrushforth commented Dec 19, 2022

andy-goryachev-oracle Dec 19, 2022

Choose a reason for hiding this comment

kevinrushforth Dec 19, 2022

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

arapte Dec 21, 2022

Choose a reason for hiding this comment

lukostyra Jan 2, 2023

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 5, 2023

lukostyra commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

arapte commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

lukostyra commented Dec 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 19, 2022 •

edited

Loading