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

[tree] fix scroll behavior when selecting or expanding a node #8154

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

Anasshahidd21
Copy link
Contributor

@Anasshahidd21 Anasshahidd21 commented Jul 8, 2020

What it does

Fixes: #3347, Fixes: #1593

The following commit updates the tree-widget to fix an issue when trees across the framework (ex: explorer, file dialog) are scrolled and nodes are selected which causes a jump and subsequently a poor user-experience.

The change includes updating the default value of the resize parameter in forceUpdate to false since we should only properly perform a cache clearAll only when an explicit resize has occurred.

clearAll ()
Reset cached measurements for all cells.

This method should be called when a Grid, List or Table needs to reflow content due to a resizing event for a responsive layout (eg. a window width resize may have an impact on the height of cells).

How to test

  1. start the application and verify the behavior with multiple trees (ex: explorer, problems, file dialogs, siw, scm)
  2. for a given tree-widget, perform a manual scroll and select a node
  3. determine that the tree does not jump as reported in the issues [file-tree] Undesired scrolling issue when toggling tree node expansion state #3347 & [navigator] disturbing scroll behavior on selection #1593
  4. determine that the debug console is properly rendered when resized

Review checklist

Reminder for reviewers

Signed-off-by: Anas Shahid muhammad.shahid@ericsson.com
Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com


Recordings:

Explorer Debug Console
scroll-yay debug-yay

@vince-fugnitto vince-fugnitto added core issues related to the core of the application tree issues related to the tree (ex: tree widget) bug bugs found in the application labels Jul 8, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really happy the annoying glitch was fixed 👍
I verified using multiple trees and it all works correctly when manually scrolling and selecting a node (no more annoying jump):

  • explorer
  • search-in-workspace
  • problems
  • npm scripts

Copy link
Contributor

@kaiyue0329 kaiyue0329 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes work well 👍 ! Tested the trees in Explorer, Search-in-workspace and Problems. The scroll no longer jumps up and scrolls the selected element away.

@Anasshahidd21 Anasshahidd21 force-pushed the disturbingScroll branch 3 times, most recently from 13b1492 to d67005f Compare July 9, 2020 15:56
@Anasshahidd21
Copy link
Contributor Author

@akosyakov Thank you for your previous review. You are right, my solution was not very optimal. I have updated the code, if you can review again that would be great. As mentioned in the PR description(updated), I made use of clear as mentioned here

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes work correctly when only clearing what is necessary 👍

@akosyakov
Copy link
Member

As mentioned in the PR description(updated), I made use of clear as mentioned here

please when you read something on the internet, think whether conditions are really applicable to our case

@vince-fugnitto
Copy link
Member

@akosyakov the code was updated to only resize (clearAll cache) when an explicit resize occus (onResize of the widget).
Is there another case that needs to be handled? With this approach the trees are correctly selected without a jump and the debug console still wraps correctly when resized.

@akosyakov
Copy link
Member

Is there another case that needs to be handled? With this approach the trees are correctly selected without a jump and the debug console still wraps correctly when resized.

Resizing was just my example of the case when the height of a row can change. I don't know what else can cause change of the height.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try.

It would be good if someone writes down what was the root cause of the issue and why such change fixes it. I don't understand it.

Fixes: eclipse-theia#3347
Fixes: eclipse-theia#1593

The following commit updates the tree-widget to fix an issue when trees are manually scrolled and nodes are selected causing a jump (poor user-experience).
The changes include:
- updating the default resize parameter for forceUpdate to false since we only want to clear the entire cache when a resize actually occurs
- updates the onResize to properly resize when performing a forceUpdate

Signed-off-by: Anas Shahid <muhammad.shahid@ericsson.com>
Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@Anasshahidd21
Copy link
Contributor Author

@kittaakos can you review this to see if it solves the issue for file-dialog for you?

@kittaakos
Copy link
Contributor

can you review this to see if it solves the issue for file-dialog for you?

I have tried it and quickly compared the behavior with the master: stunning. It works great, thank you for the fix!

@vince-fugnitto vince-fugnitto merged commit da73ae1 into eclipse-theia:master Jul 23, 2020
@vince-fugnitto vince-fugnitto deleted the disturbingScroll branch July 23, 2020 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application core issues related to the core of the application tree issues related to the tree (ex: tree widget)
Projects
None yet
5 participants