jfx
jfx copied to clipboard
openjdk
8323511: Scrollbar Click jumps inconsistent amount of pixels
As seen in the unit test of the PR, when we click on the area above/below the scrollbar the position jumps - but the jump is now not always consistent. In the current version on the last cell - the UI always jumps to the top. In the other cases, the assumed default cell height is used.
With this PR, always the default cell height is used, to determine how much is scrolled. This makes the behavior more consistent.
Especially from the unit-test, it's clear that with this PR the behavior is much more consistent.
This is also related to the following PR: https://github.com/openjdk/jfx/pull/1194
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change requires CSR request JDK-8332041 to be approved
- [ ] Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Issues
- JDK-8323511: Scrollbar Click jumps inconsistent amount of pixels (Bug - P4)
- JDK-8332041: Scrollbar Click jumps inconsistent amount of pixels (CSR)
Reviewers
- Andy Goryachev (@andy-goryachev-oracle - Reviewer) 🔄 Re-review required (review applies to 023613d6)
Contributors
- Eduard Sedov
<[email protected]>
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1326/head:pull/1326
$ git checkout pull/1326
Update a local copy of the PR:
$ git checkout pull/1326
$ git pull https://git.openjdk.org/jfx.git pull/1326/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1326
View PR using the GUI difftool:
$ git pr show -t 1326
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1326.diff
Webrev
:wave: Welcome back fkirmaier! 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
- 08: Full - Incremental (adfc3ef2)
- 07: Full - Incremental (023613d6)
- 06: Full - Incremental (f16cff94)
- 05: Full - Incremental (645feee3)
- 04: Full - Incremental (b08aaa59)
- 03: Full - Incremental (3972d8ad)
- 02: Full - Incremental (6a02f682)
- 01: Full - Incremental (29d610fd)
- 00: Full (cbcaefec)
@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).
Thanks for the PR! Quick observations:
-
The unit test is indeed clear and at first sight, it looks to me that this is reasonable behavior. One of the hard parts with the VirtualFlow and related concepts, is that different use case scenarios assume different "reasonable behavior". Since we now have more tests than before, I am increasingly confident that a new change to the behavior is not causing regression.
-
Until we have the exact heights of all cells, the different measurable values (thumb location of scrollbar, position of the cell) can not be set simultaneously to desired values. (That is one of the other hard parts). Hence, changing the behavior of the scrollbar may affect the accuracy of the cell positioning in other scenario's. Again, since we have a growing amount of tests, I'm less worried about this.
I'll look into this more detailed.
For what it's worth, I just wanted to mention an alternative algorithm that was used in another project:
- keep a cache of at least one screenful of laid out cells above and below the view port (a sliding window). this enables correct scrolling when pixels (screen dimensions) are used, as in page up / page down, or when navigating close to the start/end
- do not update scroll bar min/max/thumb while the user is scrolling using the scroll bar. do update on MOUSE_RELEASED event. this eliminates flicker when cells with different sizes come into play
Please let me know if you want to see the code.
Agree, with all the tests added, especially in this area in https://github.com/openjdk/jfx/pull/1194 and in https://github.com/openjdk/jfx/pull/1246, it is much easier for us to catch regression. I will also have a look in the next days. I also noted that I got weird scrolling behaviour once before, but could never reproduce it.
Tested on macOS 14.1.2 with ListView, fixed and variable cell height, using MonkeyTester https://github.com/andy-goryachev-oracle/MonkeyTest
I do see two problems:
- With variable height cells: if I click below the scrollbar thumb, followed by a click above the thumb, I expect the view to go back to the same position exactly, but it does not (see the screenshots). I think I've mentioned this problem before in some other PR.
initial condition:
clicked below the thumb:
clicked above the thumb:
- this one is more serious. sometimes it gets into state when clicking below the thumb does not move the scroll bar at all, here is the screenshot:
It seems to happen when the cell height exceeds the viewport height AND the cell is positioned exactly at the top of the viewport.
Can you see it?
- With variable height cells: if I click below the scrollbar thumb, followed by a click above the thumb, I expect the view to go back to the same position exactly, but it does not (see the screenshots). I think I've mentioned this problem before in some other PR.
This is one of the major difficulties with the VirtualFlow. The word "expect" is very context-dependent. Different scenario's have different expectations. As long as there is nothing specified in the JavaDoc, I don't think one can expect anything. In this particular case, for example, I can think of a number of reasons why you will not go back to the exact same position.
- a cell has been added (outside the viewport).
- the contents of a cell (outside the viewport) have changed, leading to different sizes
- the estimates for cell dimensions have been updated
The location of the scrollbar thumb does not depend on previous actions, but on the current estimated position in the total sheet, which may vary for a number of reasons. We have the implicit restriction that even though the estimated position may change intermediately, we will not reposition the scrollbar thumb unless there is an explicit user action (scrolling or jumping etc).
For some time, we have been going back and forth to patch VirtualFlow in order to match particular expectations. By making a change, it is often very likely that the expectations in some scenario are violated -- although no contract is violated, hence no bug is introduced. With the changes in VirtualFlow over the past years, we implicitly specify the expectations by adding regression tests. In the end, it would be good to somehow have an exhaustive specification document.
@FlorianKirmaier 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!
@FlorianKirmaier I still think this would be a good addition. I believe there is only one open question (from Marius) so it would be great if you can answer that.
@johanvos Great to see you would like to see it merged! I've worked on this topic together with @eduardsdv , So consider his response to be my answer to the question.
@FlorianKirmaier 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:
8323511: Scrollbar Click jumps inconsistent amount of pixels
Co-authored-by: Eduard Sedov <[email protected]>
Reviewed-by: angorya, kcr, jvos
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:
- cf09d6f1b8e479e77683c91e271fac8716fe0791: 8332863: Crash in JPEG decoder if we enable MEM_STATS
- b08d7bbf83057c83cfde71592869db5aa4efff8e: 8322964: Optimize performance of CSS selector matching
- 2e1427e8c873c10c0929ffdf385bec305f13f41d: 8087444: CornerRadii with different horizontal and vertical values treated as uniform
- dedcf1d236b5429dcf3c42f5fd1095b28d5da063: 8332539: Update libxml2 to 2.12.7
- 31fe622c4e540df06ed727343ace16cba3942534: 8332732: Clean up non-standard use of /// comments in JavaFX
- 94aa2b68d01af6096a18316ab36c911fe8cec572: 8332313: Update code review guidelines
- b685db23f07df32a3caea7af36206c48b52bb6eb: 8313138: Scrollbar Keyboard enhancement
- b5fe362286056e516be1d26f5d1cdda12eb20a4c: 8330590: TextInputControl: previous word fails with Bhojpuri characters
- 9dc4aa2341581d730c9d721e91ac0da081ffddcc: 8324327: ColorPicker shows a white rectangle on clicking on picker
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.
Are there any open questions or objections? Can this pull request be merged?
Intuitively, the added test seems to be the right thing to do. It indeed fails before and succeeds after the patch. However, I'm not sure about the implementation.
The suggested patch changes the conceptual idea of VirtualFlow.scrollTo(int index) where a negative index is not specified (this is probably what @Maran23 asked at https://github.com/openjdk/jfx/pull/1326#discussion_r1530233902 .
The way the scrollTo(int index) is modified doesn't sound right to me. In case of the test, where the function is called with index = -1, it will first try to obtain the cell at index -2 (which will always return false), and then it will try to obtain the cell at index 0 (the one we need to have indeed), and then scroll X pixels where X is the height of the cell at index -1. In the test, all cells except the one at 0 have height 100, so the cell at -1 will have 100 as well, so it will use that.
This seems a complex way to deal with the issue. It would need a clear definition of "default size" and it changes the implicit assumption that scrollTo(index) should be called with a valid index (which does not have to be visible in the current viewport).
I believe the first thing that needs to be done, is to extend the "specification" that is currently in comments in the VirtualScrollBar:
/*
* Scroll one cell further in the direction the user has clicked if only one cell is shown.
* Otherwise, a click on the trough would have no effect when cell height > viewport height.
*/
What does this mean if there is no cell further in the direction the user has clicked? Should we them scroll to the top of the current cell (current behavior), or should we use a more gradual scroll (which is the new behavior, which I believe is better indeed)? If the latter is the preferred case, this looks to a behavior that is more similar to the Event that is received when the mousewheel is used (and which invokes VirtualFlow.scrollPixels(double delta))
The suggested patch changes the conceptual idea of
VirtualFlow.scrollTo(int index)where a negative index is not specified (this is probably what @Maran23 asked at #1326 (comment) .
Yes, this is exactly what I mean and where I do not know if this is the right approach.
The way the scrollTo(int index) is modified doesn't sound right to me
Agree, it sounds somewhat weird to me that when scrollTo is called with index = -1, that means we just scroll up more gradually (not to the top of the cell).
If the latter is the preferred case, this looks to a behavior that is more similar to the Event that is received when the mousewheel is used (and which invokes VirtualFlow.scrollPixels(double delta))
I completely agree. This sounds like we may should call scrollPixels directly instead.
As @johanvos mentioned, we also need to change the "specification" in the comment at least.
I understand your objections. I will create a new pull request. We can then discuss which solution we use.
But I already have a few questions about it.
In the new pull request, the VirtualScrollBar.adjustValue(double pos) method will then use the VirtualFlow.scrollPixels(double delta) method as follows.
public class VirtualScrollBar extends ScrollBar {
@Override
public void adjustValue(double pos) {
if (isVirtual()) {
IndexedCell<?> cell = flow.getCell(-1);
double pixels = flow.getFixedCellSize() > 0
? flow.getFixedCellSize()
: flow.isVertical()
? cell.getLayoutBounds().getHeight()
: cell.getLayoutBounds().getWidth();
if (pos > getValue()) {
flow.scrollPixels(pixels);
} else {
flow.scrollPixels(-pixels);
}
} else {
super.adjustValue(pos);
}
}
}
The main question is how far should we scroll when the user clicks on the scrollbar-track?
For backwards compatibility reasons, I have used the same logic to calculate the delta pixels as is currently used.
Therefore, to calculate the delta pixels, we probably need to change the visibility of the method VirtualFlow.getCellLength(T cell) to public. Currently I copied the content of it.
Furthermore, the method VirtualFlow.getCell(int index) is always called with the index -1 that always returns the accumCell.
Can we somehow shortcut this or should we use a different logic here?
Should we move the entire delta pixel calculation logic into a new method VirtualFlow.getBlockIncrement()?
That's a good question. Since there are a number of scenario's that change the current position (offset) of the VirtualFlow, we have to be careful that those scenario's are not conflicting, and are using the same code when appropriate. Hence, duplicating code between VirtualFlow and VirtualScrollBar sounds dangerous (what if code is changed on one place but not on the other?)
Rather than shifting some logic to VirtualScrollBar, I believe it would be safer to have VirtualScrollBar calling into VirtualFlow. For mouse-scrollwheel events, this is already the case so it might be a good idea to do something similar here?
I added a new method VirtualFlow.getBlockIncrement(), which returns the amount of pixels by which the position of the VirtualFlow should be adjusted when clicking on the scrollbar track.
Now, if only one cell is visible, the VirtualScrollBar changes the position of the VirtualFlow simply by calling the VirtualFlow.scrollPixels(double)``.
I might have raised this question before. In my opinion, the scrolling in these virtualized views should be more or less equivalent to the scrolling in the ScrollPane. In other words, using thumb track to page down and then page up (or using PgUp/PgDn keys) should get the control to its original state, unless it hit the rail.
This is how it's currently works in MS Word (which isn't exactly a pinnacle of usability, but something a lot of people across the globe use routinely). See here using Design -> Paragraph Spacing popup, notice how the initial state is not aligned exactly at the top of the cell, clicking on the track below the thumb to page down, then above the thumb to page up, we arrive at the same exact position:
diff 1 and 3:
I agree with you, at least with scrolling when clicking on the scrollbar track. I would like to change the amount of scroll pixels to the viewport length.
PageUp/PageDown not only scrolls the view, but also moves the selection/focus, so it makes sense to align the cells up/down.
PageUp/PageDown not only scrolls the view, but also moves the selection/focus, so it makes sense to align the cells up/down.
You are right about focus/selection!
This brings up another question: for accessibility reasons, do we want to add key bindings for the vertical scrolling in addition to the horizontal ones in #1393 ? For example, when the cell height exceeds the viewport height, there should be a way to scroll the content vertically using unit increment via keyboard only.
What do you think?
It would be nice if it possible to scroll with the keyboard without moving the selection/focus.
I followed @andy-goryachev-oracle's suggestion and changed the block increment to the viewport length. I also fixed the scroll behavior for the case described in my comment https://github.com/openjdk/jfx/pull/1326#discussion_r1591327052.
@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@FlorianKirmaier please create a CSR request for issue JDK-8323511 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
should the contributor info be updated to include @eduardsdv ?
@FlorianKirmaier @eduardsdv As @andy-goryachev-oracle mentioned, you will need to file a CSR for the new public method, Please include enough information in the Problem and Solution sections to justify why a new method is needed. The spec (javadoc API documentation) for that new method should indicate its purpose, and needs an @since 23 tag.
Also, please wait for @johanvos to review this.
/reviewers 2 reviewers