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

Incorrect forward padding when resizing the Viewport #213

Closed
AnnaKozlova opened this issue Sep 24, 2020 · 7 comments
Closed

Incorrect forward padding when resizing the Viewport #213

AnnaKozlova opened this issue Sep 24, 2020 · 7 comments
Labels

Comments

@AnnaKozlova
Copy link

AnnaKozlova commented Sep 24, 2020

Hello, @dhilt.
I agree with @brabenetz comment. It would be very convenient if we could have a parameter like an optional setting property "hideDataPaddingForwardIfLastItemVisible=true" or something else.
I have a similar problem issue only the actions to solve it need more: The viewport container has a dynamic height, the parameters have a custom template and can change at any time (by async request) and there is a lot of data.
I need to subscribe everywhere to size changes and it becomes inconvenient.

I wanted to clarify: if we change the size of the elements inside, we should use the check() function to check, and if we change the height of the container, then clip (), right?

In general, it would be good to have additional functionality so that we can disable data-padding-forward computations. Maybe you can change my mind and suggest a more elegant solution to my problems. Thank you.

@dhilt
Copy link
Owner

dhilt commented Sep 24, 2020

@AnnaKozlova Changing the Viewport's dimensions in real time is an interesting use case. Adapter.check can't help you here, because this method handles only items changes, not the viewport itself. Scroll event might help as it triggers the standard procedure of so called inner loop processes chaining (fetch-render-clip-adjust) on the ui-scroll end.

Basically, changing the dimension of the Viewport triggers the scroll event automatically, if there are items or non-empty padding in the backward direction out of the visible area (in short, if the scroll position is greater than 0). But a) it may not happen, b) the contents may jump down a bit depending on the resize speed. Instead, I would suggest to fire the scroll event manually, as it will guarantee running the inner loop stuff.

// need to call in response to the Viewport dimensions change
this.viewportRef.nativeElement.dispatchEvent(new Event('scroll'));

A good question is how to reduce the effect of the contents jumping when we rapidly increase the viewport size with scrollTop > 0. This is absolutely expected behavior, but it could worsen the user experience. I have no vision how to deal with it.

I made quick demo, ngx-ui-scroll-change-viewport-size-in-real-time, it uses ResizeObserver to handle the Viewport changes. It has fixed minIndex (scrollTop = 0), and if you remove the code of dispatching the scroll event, you will see that the Viewport resizing is not accompanied by new items appearance. From the other hand, if you unfix the minIndex, you will be able to experience the contents jumping regardless of auto and manual scroll event triggering.

Adapter.clip is not necessary in any case, I don't think the resize can make a lot of items clippable. You may use it to keep the DOM as small as possible, in this case please don't forget to sync it with the ui-scroll Workflow engine via await datasource.adapter.relax().

@dhilt
Copy link
Owner

dhilt commented Sep 24, 2020

@AnnaKozlova The previous comment didn't address the forward padding issue. I want to discuss it separately. Do you experience this problem on 100% scaling? The initial issue #111 was about non-100% scaling environments. And I would consider it as a library bug if you have it on 100% scaling (in this case I will definitely need the repro). If not, we'll think how to move forward...

@AnnaKozlova
Copy link
Author

AnnaKozlova commented Sep 25, 2020

Thank you for researching the problem. It is the useful notice about the scroll jump.
My general problem is that I shouldn't show a scroll if the elements are fit in the viewport. Unfortunately, scroll is displayed almost always, because viewport size may change or items size themselves too.

I'd like to use dispatchEvent with scroll event, but it doesn't work for me.
Moreover, I see strange behavior when subscribing to resize items: if I resize one item it sends events to all the other items too, even if they have not changed. Here 100% scaling. I don't know why this is happening, maybe it's the subscription library itself. Using it to call the check() function will be dangerous and I think it will cause performance problems. But I may be wrong.
image

Here is your demo with changes: A limited number of items, the initial size of viewport more, and added item component.
demo

This allows us to see that when the viewport size is reduced, the data-forward-padding is not recalculated, even when the scroll event is fired manually.

Steps to reproduce the case when the scroll is shown even when there is free space at the bottom:
[1 case]: change viewport, make it less

Steps to reproduce strange subscription behavior:
[2 case]: click on item, border will be added.

@dhilt dhilt changed the title Incorrect forward padding. Incorrect forward padding when resizing the Viewport Sep 28, 2020
@dhilt
Copy link
Owner

dhilt commented Sep 28, 2020

On items ResizeObserver triggering

The thing is that any change you apply to the layout does really update ResizeObserver params of all items. As a simplest check I made following:

Screenshot 2020-09-28 at 16 23 41

Log for item-19 was called twice, before bordering item-18 and after. This is not related to the scroller, and you need to develop an algorithm that would be specific to your App and your needs.

On Viewport params consistency during resize

I researched the problem and even released tiny patch (ngx-ui-scroll v1.8.3) related to it. I can confirm that currently we do not have stable solution on the lib end. The best fit approach is to use Adapter.clip after the resize is done, but it may fall into a race condition with scroll event triggering. Throttling and Adapter.relax do help, but not 100%, there are too much async processes.

In my current opinion, the Adapter.check method is the best candidate for handling resize, so I'll be thinking in that direction. I'm not sure whether this is a bug or enhancement (race conditions on clip cloud be treated as a bug, but expanding the Adapter.check functionality is definitely an enhancement), but I'll flag it as a bug because the issue affects the UX.

@dhilt dhilt added the bug label Sep 28, 2020
This was referenced Oct 5, 2020
@dhilt
Copy link
Owner

dhilt commented Oct 5, 2020

After some updates (ngx-ui-scroll v1.8.5), I believe the following workaround has become stable enough to be used in different circumstances:

async onViewportResize() {
  this.viewportRef.nativeElement.dispatchEvent(new Event('scroll'));
  await this.datasource.adapter.relax();
  this.datasource.adapter.clip();
}

The demo is available here: https://stackblitz.com/edit/ngx-ui-scroll-change-viewport-size-in-real-time.

Also, I created separate issue (#218) for handling the viewport size changes with the Adapter.check method.

@dhilt dhilt closed this as completed Oct 5, 2020
@AnnaKozlova
Copy link
Author

@dhilt I don't have enough time to check it out right now, but I think I'll check it out by the end of the week. I guess everything will be fine now. Thank you very much for your efforts and support!

@AnnaKozlova
Copy link
Author

AnnaKozlova commented Oct 21, 2020

@dhilt
I checked your approach and found a little problem: when datasource is empty scroll still appears. Padding isn't recalculated when the viewport is changed. But that's okay. I fixed this with an additional condition:

async onViewportResize() {
    if (this.uiDatasource.adapter.itemsCount > 0) {
      this.viewportRef.nativeElement.dispatchEvent(new Event('scroll'));
      await this.datasource.adapter.relax();
      await this.datasource.adapter.clip();
      return;
    }
    await this.datasource.adapter.relax();
    await this.datasource.adapter.reload();
}

Now everything works.
All I have to do is now to recalculate the padding when the elements are changed. But that's another story.
Thank you!

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

No branches or pull requests

2 participants