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

feat: custom ion-slides mutation observer #21739

Closed
pixelbucket-dev opened this issue Jul 13, 2020 · 17 comments
Closed

feat: custom ion-slides mutation observer #21739

pixelbucket-dev opened this issue Jul 13, 2020 · 17 comments
Labels
package: core @ionic/core package type: feature request a new feature, enhancement, or improvement

Comments

@pixelbucket-dev
Copy link

pixelbucket-dev commented Jul 13, 2020

Feature Request

Ionic version:

[x] 5.2.3

Describe the Feature Request

We're rendering dynamic content (graphs, D3) inside a caroussel (ion-slides) which repeatedly render streamed data (once a second). This, it seems, causes ion-slides' Mutation Observer to fire constantly and update each ion-slide. I observed that in the elements tree, each ion-slide's attributes are updated once per second. The consequence of this behaviour is, while slowly dragging the slides, that sometimes they can jump or flicker erratically. This is really unpleasant from a UX perspective.

To find the source of that issue I had to clone and build Ionic Core manually and link it to our app. I found that the default values of Mutation Observer don't work for us.

mut.observe(this.el, {
        childList: true,
        subtree: true
});

I mitigated the issue by changing the options object to restore the defaults.

mut.observe(this.el, { attributes: true });

Et voila, flickering/jumping disappear. A side benefit is the noticably smoother dragging performance.

Describe Preferred Solution

There are two options:

  1. Extend the ion-slides API to allow passing MutationObserverInit options from outside, e.g.
@Prop() mutationsOptions: MutationObserverInit = {
        childList: true,
        subtree: true
};
  1. Prop to tell whether have shallow observations or deep observations.
@Prop() observeChildrenDeep = false;



mut.observe(this.el, this.observeChildrenDeep ?  {
        childList: true,
        subtree: true
}): { attributes: true }

Feel free to find better names 😄.

Additional Context

Currently, I cannot get this to work w/o forking/linking Ionic Core because I don't have access to the internal Mutation Observer setup.

@ionitron-bot ionitron-bot bot added the triage label Jul 13, 2020
@liamdebeasi
Copy link
Contributor

Thanks for the feature request. Can you provide a GitHub repo of the current behavior? I spoke with the team, and we are a bit hesitant to expose some of the internals of ion-slides. If we can see an example of the issue, we might be able to come up with a good workaround.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 13, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 13, 2020
@pixelbucket-dev
Copy link
Author

pixelbucket-dev commented Jul 13, 2020

Hi @liamdebeasi thanks for the reply. This will be quite tricky because we we're having a lot of high-throughput live data and complex graphs, which won't be easy to mock.

All I am asking is a way to change the default from outside, which can stay as it is, but could give users a bit more flexibility. I did not report this as a bug because I imagined it is intentional. However, to me it feels like a bug, because the defaults you set make certain assumptions and seem to assume a best-case scenario. Correct me if I am wrong 😄 .

The Mutation Observer API has these props set to false for good reason, however, your component sets them to true.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jul 13, 2020
@pixelbucket-dev
Copy link
Author

I also just might have found an issue in one of our graphs. I removed it and the problem disappeared. I need to dig deeper to find the reason for it :).

@liamdebeasi
Copy link
Contributor

Our concern is that if users customize the Mutation Observer settings, other parts of ion-slides may no longer be reactive (causing other issues). If you are able to create a minimal reproduction of the issue, I am more than happy to take a look.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 13, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 13, 2020
@luboslav
Copy link

I had similar performance issues with ion-slides. I tried to use custom drag & drop library inside ion-slides component. The library needs to move dom elements very often and these changes trigger internal update of swiper inside ion-slides.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jul 14, 2020
@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 14, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 14, 2020
@pixelbucket-dev
Copy link
Author

Here is a screen grab of the DOM, which highlights the issue, showing continuous DOM node flashes (ion-slide elements). The issue is the second ion-slide element, which has some D3 magic going on and re-adding a new g element for the y-axis every second, which contains a CSS transition. This g element is very deeply nested. It has the same continuous DOM flash as both ion-slide elements. The first ion-slide does not have a nested element that flashes (re-renders). Since the mutation observer does deep DOM checks it affects all ion-slide elements within ion-slides and makes them flash/re-render. This is why, I believe, the dragging interfers with the mutation observer changes.

swiper1

Changing the ion-slides mutation observer config, as outlined above, prevents this DOM flasing behaviour and therefore now flickering during drag.

Some background: We're also using some hammerjs logic in the graph above. I first thought it's hammerjs events interferring with swiperjs. After deactivating all hammerjs logic, the problem persisted.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jul 14, 2020
@liamdebeasi
Copy link
Contributor

Thanks! Are you still able to provide a code reproduction? Without code it is unlikely I will be able to fix this issue.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 14, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 14, 2020
@pixelbucket-dev
Copy link
Author

As @luboslav mentioned, I also believe there are performance improvements when doing the config tweaks to the mutation observer. I wouldn't be surprised, because deep/recursive DOM checks can be heavy. Could benefit UX and battery life a lot if this could be configurable.

The reason I believe that works for us is that we have our on ResizeObserver inside the component that wraps ion-slides. That observer calls the ion-slides update method then the container resizes.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jul 14, 2020
@pixelbucket-dev
Copy link
Author

pixelbucket-dev commented Jul 14, 2020

@liamdebeasi I will have to have a look how I can create a simple and abstract version of our complex solution. Won't be easy. I will keep you posted.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 14, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 14, 2020
@pixelbucket-dev
Copy link
Author

pixelbucket-dev commented Jul 20, 2020

@liamdebeasi I've created a sandbox. It's not exactly the same behaviour we see, here you have to work harder for the bug to occur (i.e. drag more slowly), which is probably down to the fact there are fewer updated nodes and almost no nesting.

https://codesandbox.io/s/ion-slides-debug-test-case-ztgn1

Edit: For some reason, you have to open the second "Browser tab" inside the sandbox. I wished they had a functional Stencil template. Went through some length to get a container sandbox with Stencil/Ionic booted 😄 .

I also recommend opening the Browser in full view.

@ionitron-bot ionitron-bot bot removed the needs: reply the issue needs a response from the user label Jul 20, 2020
@ionitron-bot ionitron-bot bot added the triage label Jul 20, 2020
@liamdebeasi
Copy link
Contributor

Thanks! What device(s) are you seeing the performance degradation on?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 20, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 20, 2020
@pixelbucket-dev
Copy link
Author

A bit on Desktop/Electron (web) and iOS, not tested in Android yet.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jul 21, 2020
@liamdebeasi
Copy link
Contributor

What iOS device are you testing on?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Jul 21, 2020
@ionitron-bot ionitron-bot bot removed the triage label Jul 21, 2020
@pixelbucket-dev
Copy link
Author

pixelbucket-dev commented Jul 21, 2020

Testing on simulated iPad Pro 12.9 Inch 3rd generation.

But you can also see the behaviour (less stark) in the CodeSandbox example. My mitigation was to avoid deep checks with MutationObserver. Since we use D3.js inside the slides we delgate some DOM handling responsibility to D3. This must clash with swiper or your slides implementation. I guessed it has something to do with v-dom.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Jul 21, 2020
@pixelbucket-dev
Copy link
Author

Any updates on this?

@liamdebeasi liamdebeasi added package: core @ionic/core package type: feature request a new feature, enhancement, or improvement labels Feb 23, 2021
@liamdebeasi liamdebeasi changed the title feat: Ion-Slides Mutation Observer options prop - prevent deep mutations observation feat: custom ion-slides mutation observer Feb 23, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the issue! With the release of Ionic 6, we made the decision to deprecate ion-slides in favor of using Swiper.js directly.

Moving forward, ion-slides will only receive critical security fixes, and the component will be removed in Ionic 7. As a result, I am going to close this issue.

We have prepared migration guides for each of the 3 JavaScript frameworks we support, so developers can get started migrating right away.

Migration for Angular
Migration for React
Migration for Vue

We believe this change will lead to a healthier Ionic ecosystem as it frees up resources for the Ionic team to dedicate to other areas of this project while still giving developers options for carousels in their apps. Since ion-slides currently uses Swiper under the hood, the behavior of your carousels should remain the same even after you migrate.

For more information on the reasoning for and benefits of this change, please see the ion-slides documentation.

Thank you!

@ionitron-bot
Copy link

ionitron-bot bot commented Feb 19, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Feb 19, 2022
@liamdebeasi liamdebeasi closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: feature request a new feature, enhancement, or improvement
Projects
None yet
Development

No branches or pull requests

3 participants