dhilt / ngx-ui-scroll

Virtual/infinite scroll for Angular
https://dhilt.github.io/ngx-ui-scroll/
MIT License
224 stars 19 forks source link

Issue: adaptor.reload(reloadIndex) not functioning correctly on iOS #344

Closed klscmms closed 1 year ago

klscmms commented 1 year ago

Description The IDatasource.adaptor.reload(reloadIndex) function in the ngx-ui-scroll library is not working as expected on iOS. The intended behavior of this function is to reload the scroll and bring the user to the target index. However, on iOS devices, it fails to navigate to the correct index.

Steps to Reproduce

  1. Set up a project using the ngx-ui-scroll library.
  2. Use the following code snippet to reload the scroll and navigate to the target index: const index = this.getTargetIndex(); this.serviceOrdersSource.adapter.reload(index);

Expected Behavior The adaptor.reload(reloadIndex) function should reload the scroll and accurately bring the user to the specified reloadIndex.

Actual Behavior On iOS devices, the scroll fails to navigate to the correct index after invoking adaptor.reload(reloadIndex).

Additional Information The reason why I have to use the reload index is that the card height in the scroll will change dynamically, and I need to ensure that the scroll remains positioned at the same card even after the height change. Currently, I am relying on the adaptor.reload(reloadIndex) function to bring me to the target index, but it does not work correctly on iOS. An alternative solution that can achieve the same outcome is welcome.

dhilt commented 1 year ago

@klscmms Did you try to run it in other but iOS environment? The Adapter-reload functionality is super-basic and any issues with it would be critical. Unfortunately I don't have a iOS device, but I'd expect the simplest case (like this demo) to work properly... could you check it?

Going back to your case, what is index? Are you sure it's correct before calling the adapter-reload method? What exactly happens after you call the reload, how the UI responds? You also can enable the Dev logger and send me the log, it may highlight something. To enable it follow the doc: https://github.com/dhilt/vscroll/wiki/Dev-Log.

Summing up, I would ask you to reply on the following questions


From the other hand, the scenario you described in the end of the issue description reminds me of the Adapter-check method that was designed to handle elements mutations at runtime. You may check the demo: https://dhilt.github.io/ngx-ui-scroll/#/adapter#check-size. Perhaps the Adapter-check API will give you a more consistent and user-friendly way to deal with dynamic changes.

klscmms commented 1 year ago

Yes, it works on Android. During the investigation, I found that scrolling actually works in Safari on iOS, but not in the iOS App. The iOS and Andriod setup is the same,
this.serviceOrdersSource = new Datasource<ServiceOrder>({ get: (index, count, success) => { success(this.displayedOrders.slice(index, index + count)); }, settings: { startIndex: 0, infinite: true } }); Please check the log from the file: log.txt

I also took a video with the log and the emulator. The emulator behaves the same as my iOS device.

I attempted to include this.serviceOrdersSource.adapter.check(); before the reload, but unfortunately, it did not resolve the issue.

dhilt commented 1 year ago

@klscmms Looks like an environment specific issue. This is a dark area, I mean how scrolling is implemented on this or that device/os/browser. I spent a lot of time in previous years making scroll handling more or less unified on the lib's end. In particular, this line (551) is not good in your log:

[Log] ⤵ – 191 – "0ms" – "" (cordova.js, line 1413)

It's a scroll event trigger in-between "6-120" and "6-121" WF Cycles that runs additional fetching, rendering and scroll adjustment, which results in a shift on the UI. I believe there should not be 191px scroll-down event, but the log shows it... The other shifts follow this one. Currently I have no idea how to debug it without an appropriate environment. We may try to workaround your approach or develop a new one.

1. As a workaround I would suggest to fix a default item size. As I see, you have 2 states for each item: collapsed with fixed height and expanded with non-fixed height. Can you update the scroller's settings as follows?

this.serviceOrdersSource = new Datasource<ServiceOrder>({
    get: ...,
    settings: {
      sizeStrategy: SizeStrategy.Constant, // default height is fixed
      itemSize: 40, // height of collapsed item
      ...
    }
});

Then you may try to hard-reset instead of reload using Adapter.reset API. At last, it may be important to make sure that there no pending processes when you reload/reset the scroller.

await adapter.relax();
await adapter.reset(...);

We don't normally need this, but what if the device remembers there was a scroll before reload/reset, so let's give that black box a slightly bigger chance to reset its internal state.

2. Another approach is connected with Adapter.check API. As I see, you collapse items and do a reload. Instead, you may collapse and do a check (without reload). The Adapter.check API needs to be used when currently rendered items change their size at runtime. You only need to know the exact moment of that change. For example,

await adapter.relax();
await collapseVisibleItems(); // must return a promise resolving when the UI got all changes
// await adapter.relax(); // maybe it's not a mistake to put one more protection from pending processes on the scroller's end
await adapter.check();

You may use MutationObserver API to make sure the items are collapsed. Or just put some timeout for the proof-of-concept purpose. Then would like to see how it works (video + log), because from what I see this approach should cover your scenario.

klscmms commented 1 year ago

@dhilt, I tried to use relax and reset. Here is my new setup of the source: this.serviceOrdersSource = new Datasource<ServiceOrder>({ get: (index, count, success) => { success(this.displayedOrders.slice(index, index + count)); }, settings: { startIndex: 0, infinite: true, sizeStrategy: SizeStrategy.Constant, itemSize: 20 } });

The code to reload the scroll: await this.serviceOrdersSource.adapter.relax(); this.toggleAllDetails(); // function hide details await this.serviceOrdersSource.adapter.relax(); await this.serviceOrdersSource.adapter.reset(this.serviceOrdersSource); await this.serviceOrdersSource.adapter.check(); this.scrollToCard(serviceOrder);

scrollToCard function, I set a 5 second timeout, I think it should be enough scrollToCard(serviceOrder: ServiceOrder): void { const index = this.getIndex(); // function gets the index if (index !== -1) { setTimeout(() => { console.log('==============================================='); console.log('============= reload the scroll =============='); console.log('==============================================='); this.serviceOrdersSource.adapter.reload(index); }, 5000); } }

I have one successful case, but sometimes it doesn't work Here is the works_log.txt, and the video demonstrating it in action. On the other hand, here is the not_work_log.txt and the video showing when it doesn't work. Additionally, I encountered a situation where the selected card became the first element of the list. Unfortunately, I didn't keep a log of this one, but you can check out the video to see what happened.

For the second approach: await this.serviceOrdersSource.adapter.relax(); this.toggleAllDetails(); // function hide detail await this.serviceOrdersSource.adapter.check();

The selected card did not remain in the same position on the screen. For further details and insights, please refer to the log file approach_2_log.txt and the video for more insight.

dhilt commented 1 year ago

@klscmms Okay, let's fix the 2nd approach, as it seems more appropriate in the end. First, toggleAllDetails must be async (MutationObserver can help, but let's emulate it with timeout). Second, after check is done, we need to update scroll position to match the previous state. Below is an approximate code

// make sure the scroller relaxes
await adapter.relax();
// save first visible item index
const index = adapter.firstVisible.$index;
// update the DOM
this.toggleAllDetails();
// make sure that updates are applied
await new Promise(r => setTimeout(r, 250));
// run size checker on the scroller's end
await adapter.check();

// scroll to the item that was first visible before we updated the DOM
const viewportElement = document.getElementById('viewport'); // your html should be considered here
const element = viewportElement?.querySelector?.(`[data-sid="${index}"]`);
if (viewportElement && element) {
  const elementTop = element.getBoundingClientRect().top;
  const viewportTop = viewportElement.getBoundingClientRect().top;
  const toScroll = viewportTop - elementTop;
  const diff = viewportElement.scrollTop - toScroll;
  if (viewportElement.scrollTop !== diff) {
    adapter.fix({ scrollPosition: diff });
  }
}

Manual scrolling procedure could be more accurate (for this we'll need to persist index and offset, not only index), but let's see how it works.

dhilt commented 1 year ago

The first approach is not correct, it should be much shorter, like the following

// make sure the scroller relaxes
await adapter.relax();
// save first visible item index
const index = adapter.firstVisible.$index;
// update the DOM
this.toggleAllDetails();
// make sure that updates are applied
await new Promise(r => setTimeout(r, 250));
// reset the scroller with new startIndex
await adapter.reset({
  ...this.serviceOrdersSource.settings,
  startIndex: index
});

Nothing more, reset should do the job. Also, when you set up the initial settings, you need to make itemSize to be like height of collapsed item, not 20:

      ...
      sizeStrategy: SizeStrategy.Constant,
      itemSize: ??? // should be height of collapsed item (px)
dhilt commented 1 year ago

@klscmms After you try both fixed approaches, I'll be itching to get the new logs.

klscmms commented 1 year ago

@dhilt, thank you for your solutions!

For the manual scrolling, this was my setup:

const index = this.serviceOrdersSource.adapter.firstVisible.$index;
this.toggleAllDetails();
await new Promise(r => setTimeout(r, 250));
this.serviceOrdersSource.adapter.check();
const viewportElement = document.getElementById('viewport'); // your html should be considered here
const element = viewportElement?.querySelector?.(`[data-sid="${index}"]`);
console.log({ viewportElement })
console.log({ element })
if (viewportElement && element) {
  const elementTop = element.getBoundingClientRect().top;
  const viewportTop = viewportElement.getBoundingClientRect().top;
  const toScroll = viewportTop - elementTop;
  const diff = viewportElement.scrollTop - toScroll;
  console.log({ diff });
  if (viewportElement.scrollTop !== diff) {
    this.serviceOrdersSource.adapter.fix({ scrollPosition: diff });
  }
}

It works perfectly with the browser. However, on the native iOS app, it cannot get the element.

iOS manual scroll

For the adapter.reset, this was my setup:

const index = this.serviceOrdersSource.adapter.firstVisible.$index;
console.log({ index });
await this.serviceOrdersSource.adapter.relax();
this.toggleAllDetails();
await new Promise(r => setTimeout(r, 250));
await this.serviceOrdersSource.adapter.reset({
      settings: {
            ...this.serviceOrdersSource.settings,
            startIndex: index
      }
});

It works on the emulator(iOS 16.2) sometimes, but it doesn't work on the actual device (iOS 16.5.1(c)). Please check the native app.txt. The "index" log is at line 654. You can also view the native app video.

Not sure if it can help, but there is a log and video from when it worked on the emulator emulator log.txt, and the emulator video.

I use Ionic to build the iOS app, and the versions I'm using are as follows: Angular: 15.2.9 Ionic: 7.1.1 cordova-ios: 6.3.0 ngx-ui-scroll: 3.1.0

dhilt commented 1 year ago

@klscmms Well, I think we are close to completing this.

It works perfectly with the browser. However, on the native iOS app, it cannot get the element.

You just need to find a consistent way to get the viewport element in your native-iOS environment. You may try the standard Angular approach:

<div class="viewport" #myViewport">
    <div *uiScroll="let item of myDatasource">
        ...
    </div>
</div>
@Component({ ... })
export class MyComponent {
  @ViewChild('myViewport', { static: true })
  viewportRef: ElementRef<HTMLElement>;

  ...

  doJob(...) {
    const viewportElement = this.viewportRef.nativeElement;
    ...
  }
}

In case it's not there, I'd say you have a non-scroller issue.

klscmms commented 1 year ago

@dhilt, thank you!

We are very close to closing this. I think the calculation of the scrollPosition has something wrong. Especially when selecting the last item on the list. It scrolls upward which is not indented.

Scroll Setup:

this.serviceOrdersSource = new Datasource<ServiceOrder>({
  get: (index, count, success) => {
    success(this.displayedOrders.slice(index, index + count));
  },
  settings: {
    startIndex: 0,
    infinite: true,
    sizeStrategy: SizeStrategy.Constant,
    itemSize: 74
  },
  devSettings: {
    debug: true
  }
});

When DOM changes:

const index = this.serviceOrdersSource.adapter.firstVisible.$index;
this.toggleAllDetails();
await new Promise(r => setTimeout(r, 250));
this.serviceOrdersSource.adapter.check();
const viewportElement = this.serviceOrderList.nativeElement;
const element = viewportElement?.querySelector?.(`[data-sid="${index}"]`);
console.log({ index });
if (viewportElement && element) {
  const elementTop = element.getBoundingClientRect().top;
  const elementHeight = element.getBoundingClientRect().height;
  console.log({ elementHeight });
  const viewportTop = viewportElement.getBoundingClientRect().top;
  const toScroll = viewportTop - elementTop;
  console.log({ viewportTop });
  console.log({ elementTop });
  console.log({ toScroll });
  console.log('viewportElement.scrollTop: ', viewportElement.scrollTop);
  const diff = viewportElement.scrollTop - toScroll;
  console.log({ diff });
  if (viewportElement.scrollTop !== diff) {
    this.serviceOrdersSource.adapter.fix({ scrollPosition: diff });
  }

Please check the scroll_log.txt (the DOM changed log start from line 653) and the video, I am not sure if it is a scroller issue.

dhilt commented 1 year ago

@klscmms I believe you forgot to wait for the Scroller to relax after check. Without it, scroll position calculation becomes messy.

await this.serviceOrdersSource.adapter.check(); // await !