Skip to content
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

8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set #166

Closed
wants to merge 2 commits into from

Conversation

@ccavanaugh
Copy link

@ccavanaugh ccavanaugh commented Apr 9, 2020

https://bugs.openjdk.java.net/browse/JDK-8129123

This pull request fixes JDK-8129123. This bug also effects Windows and Linux platforms.
Also, I believe JDK-8196037 is a duplicate of this issue.

I've tested this against OpenJDK 11.0.6 on Ubuntu 18.04, Arch Linux and Windows 10.

Thanks!


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8129123: ComboBox popup list view does not scrollTo when ComboBox value/selection is set

Download

$ git fetch https://git.openjdk.java.net/jfx pull/166/head:pull/166
$ git checkout pull/166

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 9, 2020

👋 Welcome back ccavanaugh! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 10, 2020

was: #136

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Apr 14, 2020

I am taking a look at your changes now.
@ccavanaugh can you make jcheck pass while I review. Please see the details of the failing jcheck.

@ccavanaugh
Copy link
Author

@ccavanaugh ccavanaugh commented Apr 14, 2020

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 14, 2020

I think you will need to rebase and force push your branch.

@ccavanaugh
Copy link
Author

@ccavanaugh ccavanaugh commented Apr 16, 2020

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 16, 2020

The following should work:

$ git rebase -i master

If the rebase is successful, it will pop you into your EDITOR. You then select "pick" for the first commit (which is already selected by default) and "s"quash or "f"ixup for all subsequent commits. Then, if it looks good to you, do the following:

$ git push --force origin 8129123
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 16, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@ccavanaugh ccavanaugh force-pushed the ccavanaugh:8129123 branch from 3745e97 to 28b2537 Apr 17, 2020
@openjdk openjdk bot added the rfr label Apr 17, 2020
@ccavanaugh
Copy link
Author

@ccavanaugh ccavanaugh commented Apr 17, 2020

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 17, 2020

Webrevs

@aghaisas
Copy link
Collaborator

@aghaisas aghaisas commented Apr 17, 2020

I think, selection and scrolling are two separate operations. Here we use these two operations to achieve the desired result for ComboBox. It is nice to have behavior and I am OK with the fix.

Also, I believe JDK-8196037 is a duplicate of this issue.

Please Note that - JDK-8196037 is not a duplicate of this issue as described in PR description.

I have a minor comment regarding test.

int last = virtualFlow.getLastVisibleCell().getIndex();
assertTrue(" visible range [" + first + ", " + last + "] must include " + index,
first <= index && index <= last);

This comment has been minimized.

@aghaisas

aghaisas Apr 17, 2020
Collaborator

You have tested first and last index selection.
It will be nice to have a case where we select index = LIST_SIZE/2

This comment has been minimized.

@ccavanaugh

ccavanaugh Apr 18, 2020
Author

Agreed, mid point test added

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 17, 2020

repeating my comment from the previous pull request: I don't think this is yet ready for a technical review - there are some more basic questions that are not yet answered :)

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 18, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 18, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 18, 2020

This will need two reviewers. As part of this review, the questions raised by @kleopatra in this message from the original PR need to be answered.

@ccavanaugh
Copy link
Author

@ccavanaugh ccavanaugh commented Apr 19, 2020

repeating my comment from the previous pull request: I don't think this is yet ready for a technical review - there are some more basic questions that are not yet answered :)

  • is it really a bug or a nice-to-have enhancement? couldn't find an example in win, didn't try too hard and nowadays, such plain combos are not a really frequent
  • while none of the virtualized controls (nor ChoiceBox) combines selection with scrolling to the selected item. For combo and choice, I see no reason not make it the default behavior. We need to make certain it behaves "naturally" when navigating in the open popup.
  • instead of catching every list.select (and not forget the unselect) we might consider doing it in a showing handler
  • alternatively, we might consider to go deeper and support it directly in the listView
  • For me / my users / and the open bug, it is a bug due to the current behavior being unexpected. It creates the illusion of a preselected value not actually being selected because it's not visible if the list is large and has been shown. It creates doubt and the user has to scroll to reconfirm their selection which takes extra unnecessary effort and time.

  • With my testing, for the ComboBox, behavior was smooth and natural. I've not made any attempt to change selection with it shown and I'm not certain it can happen unless done programmatically. User selection within the list requires scrolling, so the selected value is already visible.

  • I'm not opposed to taking this approach. My current work around by extending ComboBox is based on scrolling when the value is set (restored) programmatically. I could see how it may be more efficient if multiple selections were being performed programmatically, but not sure why someone would write code this way. I would think it's a one shot process to select the final value.

  • Implementing the change in ListView would not change/improve ChoiceBox simply because it does not use an underlying ListView. My search on uses of ListView only reviled ComboBox other than itself. Since ListView by itself is not collapsed/hidden for typical uses, would automatic scrolling within ListView create a confusing experience?

@kevinrushforth kevinrushforth self-requested a review Apr 21, 2020
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 26, 2020

For me / my users / and the open bug, it is a bug due to the current behavior being unexpected. It creates the illusion of a preselected value not actually being selected because it's not visible if the list is large and has been shown. It creates doubt and the user has to scroll to reconfirm their selection which takes extra unnecessary effort and time.

Yes, I understand the perspective of the users (would be unhappy with it as well) - but from the perspective of the fx, it's the job of the application developer to keep a selected item visible. Which s/he could do easily enough, even for a comboBox. Also I agree, that in the case of the combo it feels unexpected to not the selected item on opening the popup. So we could do something about it - but we have to specify exactly what that something should be.

With my testing, for the ComboBox, behavior was smooth and natural. I've not made any attempt to change selection with it shown and I'm not certain it can happen unless done programmatically. User selection within the list requires scrolling, so the selected value is already visible.

Hmm, probably wasn't clear enough: have a combo with many items, open the popup, navigate (via keys like page/up/down ..) and compare the behavior before/after the fix. I see considerable differences in behavior. F.i. keyDown does move the selection one-down without scrolling before and scrolls down by one after (making the selected item visually "stick" at the top). Do you see that as well or did I goof somehow (also a possibility :) ?

Navigation behavior should be the same before/after, I think. There might be other behavior changes introduced (didn't check further, it's your issue :), that we should actively look out for.

I'm not opposed to taking this approach. My current work around by extending ComboBox is based on scrolling when the value is set (restored) programmatically. I could see how it may be more efficient if multiple selections were being performed programmatically, but not sure why someone would write code this way. I would think it's a one shot process to select the final value.

not sure what you mean by "one shot process" and "final value": the fix scrolls the list to top whenever the value changes, and the value is changed whenever the selectedItem is changed which happens on navigation (that is user interaction) as well as changing it programatically.

Maybe we should really go back to square one and exactly specify when we want the value be scrolled into view. The current fix does so for each selection (which has rather broad effects). If it could be bounded a bit more, the effects would be narrower. That's why I suggested to limit it f.i. to the time of opening the popup (which is rather rare compared to selection change).

Implementing the change in ListView would not change/improve ChoiceBox simply because it does not use an underlying ListView.

yeah, forget about ChoiceBox, I was wrong - whoever needs to scroll to the selected value is using the wrong control ..

My search on uses of ListView only reviled ComboBox other than itself. Since ListView by itself is not collapsed/hidden for typical uses, would automatic scrolling within ListView create a confusing experience?

Ahh, guilty of having been unclear again, sry - what I meant is to add support of doing a well-behaved scrolling in ListView (and the other virtualized controls): scrollTo is not overly well-behaved. And I agree, doing it always and unconditionally, might introduce a bad user experience.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 13, 2021

I meant to close this PR out a while ago, but it kept getting put on the back burner. I don't think this PR is the right approach to the problem.

I can confirm that this doesn't behave naturally at all when using the arrow or page up / page down keys when the combo is open. Before we make any changes in FX itself we need to specify what the behavior should be for various cases, including multiple selections, scrolling while the combo is open, changing the selection while the combo is open, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants