8295078: TextField blurry when inside an TitledPane -> AnchorPane #910
Conversation
|
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
|
/reviewers 2 |
|
@kevinrushforth |
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/AnchorPaneTest.java
Show resolved
Hide resolved
|
NOTE: this is blocked by the fix for JDK-8296283, which @arapte plans to do next week. |
| @@ -355,15 +350,15 @@ private double computeChildHeight(Node child, Double topAnchor, Double bottomAnc | |||
| } | |||
|
|
|||
| if (leftAnchor != null) { | |||
| x = insets.getLeft() + leftAnchor; | |||
| x = snappedLeftInset() + leftAnchor; | |||
There was a problem hiding this comment.
Question: should we use unsnapped coordinates in the computations, and only snap at the final stage?
For example, if leftAnchor is 0.5 the child will be positioned between the pixels, so to speak.
Or is this indeed an intended functionality?
There was a problem hiding this comment.
Not 100% sure about this. It sounds somewhat logical. If we set the left anchor to be 5. Do we expect it to be 10 with a render scale of 2 -> 200% resolution?
There was a problem hiding this comment.
Not sure about render scale - it should be done elsewhere.
As for snapping, I think this code might be incorrect - it should honor Region.snapToPixel setting. All the intermediary computations need to use unsnapped (original) values, then, at the last moment, call proper Region.snap*() which either rounds or not, depending on the snapToPixel.
The only exception for this rule (of using snapped values in the intermediary computations) is to ensure some kind of specific alignment, which I believe is not the case here.
edit: my reasoning here is incorrect. any computation should use snapped values.
There was a problem hiding this comment.
I'm not sure about this. Other places do the same, and there are lot of places actually which do something like snappedLeftInsets + snappedRightInsets or final double w = snapSizeX(getWidth()) - x - snappedRightInset(); or similar.
I think this was for a reason.
And I have only oriented myself to the existing code in this case. If you are right, then this was done wrong everywhere.
Btw. I had a look and other container do snap their constraints, e.g. H/VBox its spacing, GridPane his H/VGap. So pretty sure we also should snap the anchor values.
There was a problem hiding this comment.
I think the idea is this (and it only applies when snapToPixels=true). We only need to use snapped coordinates/sizes when placing components. In order to avoid accumulating errors, which can be seen during gradual resizing for example, we should avoid snapping of intermediary results, UNLESS there is a possibility of introducing visual artifacts.
What I mean by this is imagine a container that aligns its children in a table-like layout (similar to https://github.com/andy-goryachev/FxDock/blob/master/doc/CPane.md). Depending on constraints, there might be a situation where rounding might shift certain children in relation to the vertical guide lines - we don't want that.
So in this case we need to snap the guide lines as an intermediate step (i.e. compute the first guide line position, then compute the next column, snap, next column, etc.)
Whereas in a simple case (one child node), we can simply use snapped*Inset() - we don't have intermediate computations.
In the case of AnchorPane, I think, we should first sum insets and anchor, then snap - because developers might want to use or not to use snapToPixels, and might set a fractional anchor.
There was a problem hiding this comment.
I think I now understand the snapping better, especially after discussion in #1111
it looks like we should be operating in terms of final pixels, that is, in snapped coordinates. this might mean what we should not, generally, mix snapped and unsnapped values:
result = snap(a) + snap(b); // ok
result = snap(snap(a) + b) // also ok
result = snap(snap(a) + b + c) // not ok, should be written as
result = snap(snap(a) + snap(b) + snap(c)) or
result = snap(a) + snap(b) + snap(c)
What do you think?
There was a problem hiding this comment.
I agree but that is also what I do here, isn't it?
There was a problem hiding this comment.
This looks good to me, the snapping as explained by Andy is how it should be. A source of error can be still be that floating point calculations can introduce small deviations so sometimes it is needed to resnap after doing calculations that seem harmless (ie. snappedSize * 10 or snappedWidth - snappedLeftInset may need resnapping).
There was a problem hiding this comment.
Good point. That is something we should think about - this may or may not be a problem with some of the snapping code.
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/AnchorPaneTest.java
Show resolved
Hide resolved
| @@ -355,15 +350,15 @@ private double computeChildHeight(Node child, Double topAnchor, Double bottomAnc | |||
| } | |||
|
|
|||
| if (leftAnchor != null) { | |||
| x = insets.getLeft() + leftAnchor; | |||
| x = snappedLeftInset() + leftAnchor; | |||
There was a problem hiding this comment.
Not sure about render scale - it should be done elsewhere.
As for snapping, I think this code might be incorrect - it should honor Region.snapToPixel setting. All the intermediary computations need to use unsnapped (original) values, then, at the last moment, call proper Region.snap*() which either rounds or not, depending on the snapToPixel.
The only exception for this rule (of using snapped values in the intermediary computations) is to ensure some kind of specific alignment, which I believe is not the case here.
edit: my reasoning here is incorrect. any computation should use snapped values.
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
Show resolved
Hide resolved
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/AnchorPaneTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Not a complete review, but I did leave some inline comments. I note that there is precedent for using the snapped insets in intermediate computations, so that part of the change seems OK to me. If that is going to be the pattern, you need to make sure that you are snapping the values you add to the snapped insets.
I'd like @arapte to take a closer look as well.
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
Show resolved
Hide resolved
| @@ -355,15 +350,15 @@ private double computeChildHeight(Node child, Double topAnchor, Double bottomAnc | |||
| } | |||
|
|
|||
| if (leftAnchor != null) { | |||
| x = insets.getLeft() + leftAnchor; | |||
| x = snappedLeftInset() + leftAnchor; | |||
There was a problem hiding this comment.
I looked at the other layout panes and they already do something similar to what this PR does, using the snapped insets for intermediate compuation. I also note that some of the computation is calling superclass methods that do snapping already, and expect it to be added to snapped insets (if there are insets), for example, computeWidth(). In this particular case -- snappedLeftInset() + leftAnchor -- I don't think leftAnchor is snapped, but it probably should be.
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
Show resolved
Hide resolved
| @@ -355,15 +350,15 @@ private double computeChildHeight(Node child, Double topAnchor, Double bottomAnc | |||
| } | |||
|
|
|||
| if (leftAnchor != null) { | |||
| x = insets.getLeft() + leftAnchor; | |||
| x = snappedLeftInset() + leftAnchor; | |||
There was a problem hiding this comment.
I still think this code is incorrect.
To illustrate, let's set insets to 0.8 and anchor to 0.5
Here is what I think it should do:
snap=off
result=1.3
snap=on
result=1.0 (and not 1.5, as I think the proposed fix would produce)
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
Show resolved
Hide resolved
|
@andy-goryachev-oracle |
|
@mstr2 : Having said that, my earlier comment is still valid - if snapToPixel==true, the final location and sizes must go through snap*() methods. And the earlier example is also valid, assuming scale=1.0. |
…textfield-blurry � Conflicts: � modules/javafx.graphics/src/test/java/test/javafx/scene/layout/AnchorPaneTest.java
|
I thought about this one more time.
Now the UI would look like this currently: If we would not snap intermediate values, the left side (inset + anchor) would be 20 instead of 20.8. Thinking about this again, isn't the current behaviour what we would expect from the layout with a scale of 1.25? |
|
@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 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/open |
|
@Maran23 This pull request is now open |
modules/javafx.graphics/src/main/java/javafx/scene/layout/AnchorPane.java
Show resolved
Hide resolved
| @@ -355,15 +350,15 @@ private double computeChildHeight(Node child, Double topAnchor, Double bottomAnc | |||
| } | |||
|
|
|||
| if (leftAnchor != null) { | |||
| x = insets.getLeft() + leftAnchor; | |||
| x = snappedLeftInset() + leftAnchor; | |||
There was a problem hiding this comment.
I think I now understand the snapping better, especially after discussion in #1111
it looks like we should be operating in terms of final pixels, that is, in snapped coordinates. this might mean what we should not, generally, mix snapped and unsnapped values:
result = snap(a) + snap(b); // ok
result = snap(snap(a) + b) // also ok
result = snap(snap(a) + b + c) // not ok, should be written as
result = snap(snap(a) + snap(b) + snap(c)) or
result = snap(a) + snap(b) + snap(c)
What do you think?
| } | ||
| return computeChildPrefAreaWidth(child, -1, Insets.EMPTY, height, true); | ||
| } | ||
|
|
||
| private double computeChildHeight(Node child, Double topAnchor, Double bottomAnchor, double areaHeight, double width) { | ||
| if (topAnchor != null && bottomAnchor != null && child.isResizable()) { | ||
| final Insets insets = getInsets(); | ||
| return areaHeight - insets.getTop() - insets.getBottom() - topAnchor - bottomAnchor; | ||
| return areaHeight - snappedTopInset() - snappedBottomInset() - topAnchor - bottomAnchor; |
There was a problem hiding this comment.
same comment about areaHeight - should it be snapSize'd?
There was a problem hiding this comment.
Well, given that we (should) snap everything including the calculation of the width, I expected that the areaWidth and height, which are basically getWidth and getHeight are already snapped.
That was at least my thinking back then, not sure now. I need to check everything again since this was a while ago. :)
There was a problem hiding this comment.
I wonder.
This might be a contrived example: what would happen if the parent containing the TitledPane is unsnapped, but TitledPane is?
In this case getWidth() may not be snapped since the titled pane has been laid out by an unsnapped parent.
There was a problem hiding this comment.
I think once there is an unsnapped parent, all children should probably also be unsnapped (ie. the property should work like the visible property; if I turn it off for one parent, all children become invisible/unsnapped).
There would otherwise be little point in setting a parent to unsnapped if one didn't also turn off snapping on all the children.
Note that we tested what happens when you put snapped content in an unsnapped container; currently this results in all children to be misaligned (not pixel aligned). I think that's the only real solution, as trying to resnap your positions when your parent container is misaligning you would otherwise need to introduce some kind of flexible border that "absorbs" some of the misalignment so the rest of the content can be snapped again...
Basically, there are three strategies for children when dealing with an unsnapped parent:
- Just do snapping as normal (JavaFX currently) and when the parent translation is added, all final positions will be shifted by an unsnapped amount resulting in all calculations (even though they used snapping logic) to be in the "wrong" positions
- Ignore snapping when a parent is unsnapped and use standard calculations
- Somehow try to resnap content taking the parent misalignment into account; this is going to cause visual artifacts as the parent is moved, and requires some kind of flexible border
There was a problem hiding this comment.
In this case getWidth() may not be snapped since the titled pane has been laid out by an unsnapped parent.
And when that happens, you can't do anything about it either. If you adjust the width, you risk not covering the entire area that the parent gave you (or more if you round up), and if you use it in any calculations you need to resnap any values resulting from it, and you would still end up with some dead space that your snapped calculations can't cover...
The only real solution seems to be to treat snapping like visibility IMHO.
There was a problem hiding this comment.
I think once there is an unsnapped parent, all children should probably also be unsnapped (ie. the property should work like the visible property; if I turn it off for one parent, all children become invisible/unsnapped).
One complication with this is that the snapToPixel property is on Region, so Group (or any other Parent that isn't a Region) doesn't have that property. What you describe might be OK as long as you ignore any parent that isn't a Region when asking the question "are all my ancestors have snapToPixel".
Basically, there are three strategies for children when dealing with an unsnapped parent:
- Just do snapping as normal (JavaFX currently) and when the parent translation is added, all final positions will be shifted by an unsnapped amount resulting in all calculations (even though they used snapping logic) to be in the "wrong" positions
Right. If a non-snap-to-pixel ancestor lays out it children, then all bets are off. Absent a compelling reason to change it, this seems like a reasonable default (perhaps it should be documented better).
- Ignore snapping when a parent is unsnapped and use standard calculations
A change to do this might be OK, with the caveat I mentioned above about non-Region parents, but this would need careful analysis. As part of this, I would want to understand the motivation for doing this (i.e., what is the benefit?).
- Somehow try to resnap content taking the parent misalignment into account; this is going to cause visual artifacts as the parent is moved, and requires some kind of flexible border
Yeah, this doesn't seems like a good idea.
If you want to explore this further, you could file a new RFE to consider making a change.
There was a problem hiding this comment.
I think it's probably not worth doing. When the application developer sets snapping off when it's on by default, they do it for a reason, so the current behavior is the right one. May be they want to smooth some transition during animation (a-la scrolling ticker tape), who knows.
FX gives this choice at per-Region basis for this kind of flexibility, otherwise there would be one global setting, right?
There was a problem hiding this comment.
I think it's probably not worth doing. When the application developer sets snapping off when it's on by default, they do it for a reason, so the current behavior is the right one. May be they want to smooth some transition during animation (a-la scrolling ticker tape), who knows.
FX gives this choice at per-Region basis for this kind of flexibility, otherwise there would be one global setting, right?
For sure it shouldn't be a global setting, and the per Region choice seems absolutely the right one. What I was talking about is more how sensible it would be for children of an unsnapped Region to still use snapping logic, and if it shouldn't work similar to how visibility works (ie. if a parent is invisible, all its children are invisible, regardless of those children's individual visible status).
If the scrolling ticker tape you mentioned contains any other children, the user probably wants snapping off for those as well as the ticker tape would otherwise not be as smooth as it could be; if the parent region is misaligned, all the children (if snapped) would still advance by an exact multiple of pixels, but they would always fall in the same spot between pixels. Also, if the speed is not a nice pixel multiple (ie. 4.5 pixels per frame) then the children would sometimes move 4 pixels, sometimes 5.
It's not truly a big issue, just something that may be unexpected.
There was a problem hiding this comment.
@kevinrushforth it probably isn't worth pursuing a change in the child/parent snapping logic right now, at least not without some compelling case. First I think snapping is rarely disabled (I never have, despite working on graphics heavy JavaFX applications). Second, one can already get the (possibly more) desired situation by taking care all children are unsnapped as well.
There was a problem hiding this comment.
I agree with the consensus here. And same as @hjohn, I actually also never disabled snapping. There was never usecase where I needed to do so (And I worked on a lot of UI related things, including creating custom components of all kind).
Right. If a non-snap-to-pixel ancestor lays out it children, then all bets are off. Absent a compelling reason to change it, this seems like a reasonable default (perhaps it should be documented better).
I agree here.The defaultis fine, and it's hard to say what would be a better default 'setting' here (if there really is a better way). Does a developer really expect that all children of a unsnapped node are unsnapped as well? Or is there a usecase where you don't want this? It's hard to answer, so as written above, the default seems fine and logicial.
…textfield-blurry
| @@ -355,15 +350,15 @@ private double computeChildHeight(Node child, Double topAnchor, Double bottomAnc | |||
| } | |||
|
|
|||
| if (leftAnchor != null) { | |||
| x = insets.getLeft() + leftAnchor; | |||
| x = snappedLeftInset() + leftAnchor; | |||
There was a problem hiding this comment.
This looks good to me, the snapping as explained by Andy is how it should be. A source of error can be still be that floating point calculations can introduce small deviations so sometimes it is needed to resnap after doing calculations that seem harmless (ie. snappedSize * 10 or snappedWidth - snappedLeftInset may need resnapping).
|
I think we should document the insights gained in this discussion (and others) somewhere in JavaFX, since snapping is such a difficult thing to get right, and the source of so many visual artifacts. |
There was a problem hiding this comment.
Thank you @hjohn for a detailed writeup!
I fully agree with the idea of explaining snapping in some form of "application note", let research the subject of where and how we can do that.
I suppose the next step would be to review other layouts to see if they fail with snapping, especially with fractional scale (hm, HBox).
|
@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: 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 6 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit 6aeaff3.
Your commit was automatically rebased without conflicts. |
The problem here is, that the
AnchorPanedoes not use its snapped insets.Therefore, the fix is to replace all
getInsets().getXXXcalls with their correspondingsnappedXXXInset()methods.Note: The reason the
AnchorPaneinside aTitledPaneis blurry in the first place is because aTitledPaneapplies padding to its content.Line 2995 in
modena.css:which translates to 9.6px.
EDIT: This is btw a good example of the JUnit 5 feature
@ParameterizedTestwith@ValueSourceProgress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/910/head:pull/910$ git checkout pull/910Update a local copy of the PR:
$ git checkout pull/910$ git pull https://git.openjdk.org/jfx.git pull/910/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 910View PR using the GUI difftool:
$ git pr show -t 910Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/910.diff
Webrev
Link to Webrev Comment