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 #136

Closed
wants to merge 0 commits into from

Conversation

@ccavanaugh
Copy link

@ccavanaugh ccavanaugh commented Mar 4, 2020

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

Error

 ⚠️ For contributors who are not existing OpenJDK Authors, commit attribution will be taken from the commits in the PR. However, the commits in this PR have inconsistent user names and/or email addresses. Please amend the commits.

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/136/head:pull/136
$ git checkout pull/136

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 4, 2020

Hi ccavanaugh, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user ccavanaugh" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@bridgekeeper bridgekeeper bot added the oca label Mar 4, 2020
@ccavanaugh
Copy link
Author

@ccavanaugh ccavanaugh commented Mar 4, 2020

/signed

@bridgekeeper bridgekeeper bot added the oca-verify label Mar 4, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 4, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@bridgekeeper bridgekeeper bot removed oca oca-verify labels Apr 2, 2020
@openjdk openjdk bot added the rfr label Apr 2, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 2, 2020

Webrevs

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 2, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 2, 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 2, 2020

Can you please provide a unit test? One that fails before your fix and passes after your fix.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 2, 2020

@aghaisas can you be one of the reviewers?

@ccavanaugh
Copy link
Author

@ccavanaugh ccavanaugh commented Apr 2, 2020

Can you please provide a unit test? One that fails before your fix and passes after your fix.

I can provide a manual test the next couple of days that demonstrates it before and after, but I'm not sure how to create an automated unit test because the issue is visual.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 3, 2020

A quick snippet of how-to write a unit test (for setup see other tests in controls) that will fail before and pass after the change:

@Test
public void testScrollInSkin() {
    int index = 50;
    comboBox.getSelectionModel().select(index);
    comboBox.show();
    VirtualFlow<IndexedCell<?>> virtualFlow = getFlow(); 
    int first = virtualFlow.getFirstVisibleCell().getIndex();
    int last = virtualFlow.getLastVisibleCell().getIndex();
    assertTrue(" visible range [" + first + ", " + last + "] must include " + index,
            first <= index  && index <= last);
}

protected VirtualFlow<IndexedCell<?>> getFlow() {
    VirtualFlow<IndexedCell<?>> virtualFlow = (VirtualFlow<IndexedCell<?>>) 
            VirtualFlowTestUtils.getVirtualFlow(comboBox);
    return virtualFlow;
}
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 3, 2020

A couple of comments to the bug (and fix) itself:

  • 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
@openjdk openjdk bot removed the rfr label Apr 4, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 4, 2020

Mailing list message from Craig Cavanaugh on openjfx-dev:

My use case which is driving the fix for this bug is user expectation in an
open source application (https://github.com/ccavanaugh/jgnash).

When a user makes an edit to an existing transaction, the expectation is
the prior selected ComboBox value is visible when the list is displayed,
otherwise it gives the impression they made an error or something is
wrong. I think the root cause of the poor experience is seeing the correct
value when collapsed, and then not seeing it or it's selection when
expanded.

Also, scrolling automatically was the default behavior for Swing.

My current work around is an extended ComboBox that listens for a mouse
click or key events and then scrolls the list view.

On Fri, Apr 3, 2020 at 8:19 AM Jeanette Winzenburg <
fastegal at openjdk.java.net> wrote:

On Fri, 3 Apr 2020 12:03:33 GMT, Jeanette Winzenburg <fastegal at openjdk.org>
wrote:

Can you please provide a unit test? One that fails before your fix and
passes after your fix.

I can provide a manual test the next couple of days that demonstrates
it before and after, but I'm not sure how to
create an automated unit test because the issue is visual.

A quick snippet of how-to write a unit test (for setup see other tests
in controls) that will fail before and pass
after the change:
@test
public void testScrollInSkin() {
int index = 50;
comboBox.getSelectionModel().select(index);
comboBox.show();
VirtualFlow<IndexedCell<?>> virtualFlow = getFlow();
int first = virtualFlow.getFirstVisibleCell().getIndex();
int last = virtualFlow.getLastVisibleCell().getIndex();
assertTrue(" visible range [" + first + ", " + last + "] must
include " + index,
first <= index && index <= last);
}

protected VirtualFlow\<IndexedCell\<\?>> getFlow\(\) \{
    VirtualFlow\<IndexedCell\<\?>> virtualFlow \=

(VirtualFlow<IndexedCell<?>>)

            VirtualFlowTestUtils\.getVirtualFlow\(comboBox\)\;
    return virtualFlow\;
\}

A couple of comments to the bug (and fix) itself:

- 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

-------------

PR: https://git.openjdk.java.net/jfx/pull/136

@openjdk
Copy link

@openjdk openjdk bot commented Apr 9, 2020

⚠️ @ccavanaugh a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master 159f6516879ca1d176223b2e28ee19a0ad1e3e19
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

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

3 participants