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

ColumnSet + Tree: Scroll position of collapsed rows will fall out of sync #1137

Closed
wants to merge 2 commits into from

Conversation

jdonaghue
Copy link
Member

These are the original steps to reproduce this bug:

  1. Have a grid with ColumnSets + Tree, with visible horizontal scrollbars
  2. Expand a parent row
  3. Collapse the parent row
  4. Scroll a column set horizontally
  5. Re-expand the same parent row
  6. Children will still have the previous scroll position

This commit fixes this issue and provides a test

@@ -182,6 +182,19 @@ define([
this.on('.dgrid-column-set:dgrid-cellfocusin', function (event) {
self._onColumnSetCellFocus(event, this);
});

if ('expand' in this) {
aspect.after(this, 'expand', this.afterRowExpand, true);
Copy link
Member

Choose a reason for hiding this comment

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

I would probably vote for just including the function internally/anonymously here rather than separating it. It shouldn't be public, and even if we underscore-prefixed it, we'd want to pick a name that is unlikely to collide with something else. In this case, I don't think it's worth exposing.

@kfranqueiro
Copy link
Member

We've been incrementally chipping away at issues with this PR, and while the actual fix works, the unit test doesn't fail when it should. Meanwhile, I've also now discovered that apparently the other ColumnSet unit tests previously written fail in IE/FF. These scroll-related unit tests are a real pain, and it seems we need to address them at a wider scale.

@kfranqueiro
Copy link
Member

To follow up on my previous comment, the other ColumnSet tests were only failing on master only, due to a naive implementation of a module to load the structural CSS for tests.

Meanwhile, the new test was not failing when it should because it was not running the test's steps in the correct order (it was scrolling after re-expanding, not before). I've pushed a squashed commit of the updated fix + tests to master in 1122212 and dev-0.4 in 3354df6.

@kfranqueiro kfranqueiro closed this Jun 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants