Skip to content

Scroll position is not persisted with RouteReuseStrategy #8145

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

Closed
ymita opened this issue Sep 15, 2020 · 15 comments
Closed

Scroll position is not persisted with RouteReuseStrategy #8145

ymita opened this issue Sep 15, 2020 · 15 comments

Comments

@ymita
Copy link
Contributor

ymita commented Sep 15, 2020

Description

IgxGrid does not persist its scroll position when the grid is reused by RouteReuseStrategy.

  • igniteui-angular version: 10.1.3
  • browser: n/a

Steps to reproduce

  1. Run the sample
  2. Scroll vertically and horizontally
  3. Click the button at the bottom to move to other page
  4. Click the button to go back to the original page

Result

Both vertical and horizontal scrollbars are reset to the initial positions.

Expected result

Both vertical and horizontal scrollbars should remain the positions where these are scrolled.

Attachments

igx-grid-scroll-position.zip

@ymita ymita added 🐛 bug Any issue that describes a bug grid: general labels Sep 15, 2020
@ChronosSF ChronosSF self-assigned this Sep 16, 2020
@ChronosSF
Copy link
Member

@ymita ,

Discussed this with @rkaraivanov , and as RouteReuseStrategy is currently implemented, fixing this would require introducing yet another mutation observer of some sort. This is because angular doesn't provide hooks that would allow us to update stuff that are broken between detaches (such as the scroll position).

There is an angular issue: angular/angular#27290 that suggests adding such hooks so I would like to postpone fixing this to when angular decide to improve their RouteReuseStrategy.

@ChronosSF
Copy link
Member

As discussed in #8272, we'll add a mutation observer to resolve this. @ymita , do we need to target old versions with this fix or we can implement it for 10.2 and later only?

@ymita
Copy link
Contributor Author

ymita commented Oct 30, 2020

@norikois For the fix of this issue, are we fine with 10.2 and later?

@norikois
Copy link

norikois commented Nov 2, 2020

@ymita
Thank you for pinging me.

@ChronosSF
Could you fix this for 10.1 as well as 10.2, which are the versions in maintenance period?

@ChronosSF
Copy link
Member

ChronosSF commented Nov 2, 2020

We only plan one more release for 10.1 , @kdinev , @radomirchev , should we postpone it a little bit so that this fix can get in?

@kdinev
Copy link
Member

kdinev commented Nov 2, 2020

@ChronosSF We will release 10.1 until all the pending PRs to it are merged. No need to postpone a current tag, we will just continue tagging until there's deltas from the previous tag.

@ChronosSF
Copy link
Member

I've been testing the mutation observer for a while and it seems to work, however:

  • The issue in Which properties or features IgxGrid will not persist over RouteReuseStrategy? #8272 is unrelated and won't be fixed with this one. The columns get pixel widths after reattaching to the DOM because a separate observer detects this as a "resize" and switches them to pixels as we don't support resizing columns in % widths.

  • To make the new observer work on IE11 @MayaKirova had to add some debounce time which makes the scroll restoration noticeable (as it happens a certain amount of ms after the DOM reappears).

If @rkaraivanov , @kdinev , @radomirchev , @ymita are fine with this caveats we can proceed with merging the PR .

@ymita
Copy link
Contributor Author

ymita commented Nov 10, 2020

@ChronosSF
The issue in #8272 is unrelated and won't be fixed with this one. The columns get pixel widths after reattaching to the DOM because a separate observer detects this as a "resize" and switches them to pixels as we don't support resizing columns in % widths.
If available, would you help me find the documentation that explains this limitation? I found the below description but this is the opposite of what you are saying here...
https://github.com/IgniteUI/igniteui-angular/wiki/Column-Resizing#resizingauto-sizing-columns-with-percentage-width

To make the new observer work on IE11 @MayaKirova had to add some debounce time which makes the scroll restoration noticeable (as it happens a certain amount of ms after the DOM reappears).
Would it be possible to share a screenshot gif that demonstrates how the fix affects the scroll restoration?

@ChronosSF
Copy link
Member

We do allow setting widths of columns in %, as well as mixing % and pixels. All of this works correctly. However, once you try resizing one column, all columns get pixel widths. This is because we currently don't support resizing in % . This is something that can be resolved in a feature request.

We have a resize observer that fixes some stuff when a grid in % width gets resized and this observer is triggered on attach/detach because a detached container has 0 width. This observer handler functions similarly to resizing a column as these columns would change widths based on the new grid width triggering the limitation of resizing columns in % -> this turns them to px.

As for the scroll restoration - we already merged the solution. You can try your original sample with the latest 10.2 version. For 10.1 I think we haven't released a version that fixes it yet, we'll have to do it next week at this point.

@MayaKirova
Copy link
Contributor

MayaKirova commented Nov 10, 2020

Wait, we do support resize with % : #7662
And we documented it accordingly: https://www.infragistics.com/products/ignite-ui-angular/angular/components/grid/column-resizing#resizing-columns-in-pixelspercentages

This issue seems to be specific to a grid that has 100% and at some points becomes 0 width (in this case when detached). It seems that there is some additional handling for such scenarios to calculate and apply the total column width as width of the grid, which incorrectly applies that calculated px width to the original width property of a column. Which is a bug - I logged it separately here: #8555

Otherwise resizing of any combination of px,% or null widths should work as expected and should not modify the original width property measurement set by the user (if user sets 20% it will remain in % after resize).

@ymita
Copy link
Contributor Author

ymita commented Nov 11, 2020

@ChronosSF
With 10.2.3, I confirmed the scrollbar restoration on IE11. This looks good to me.

@ChronosSF
Copy link
Member

Glad to hear that. Scratch my info about column widths as well. Maya already PRed a fix that resolves the detach/attach transition for it to pixels. We'll release both the scroll restoration and this fix with next week's final 10.1 patch.

@ymita
Copy link
Contributor Author

ymita commented Nov 12, 2020

@ChronosSF
Thanks for fixing this issue. Let me double check in which version this issue will be fixed. I see there are two PRs, #8499 (master) and #8500 (10.2) . Does master mean 10.1 in this case?

CC: @norikois

@ChronosSF
Copy link
Member

There were 3 prs - #8499, #8500, #8505 addressing the scroll position and further 3- #8556, #8559, #8560 that address the column widths. Both issues have PRs for 10.1 as discussed with @norikois . Master refers to the yet unreleased 11.0 version.

There was some miscommunication regarding the 10.1 release on Monday so we didn't release a patch including the scroll position fix. We'll therefore release the fixes for both issues with next week's 10.1 patch.

@ChronosSF
Copy link
Member

ChronosSF commented Nov 17, 2020

@ymita , @norikois , the fixes for both the scroll restoration and the column widths issues are now available in the latest patches for 10.1, 10.2 and 11.0 .

Let us know if you are still experiencing problems with RouteReuseStrategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants