-
Notifications
You must be signed in to change notification settings - Fork 9
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
Layout shifts caused by App Bridge side bar navigation input events included in CLS scores #194
Comments
Hi @ascherkus ! I'm sorry but I'm not sure I understood what the issue here is. CLS measured by App Bridge is isolated from the rest of Shopify's Admin because your app lives in an iframe. The way Regarding your reproduce steps, where do you use your snippet? Is it in the app code (inside the Thanks! |
Hey @bratsos! Thanks for the reply -- I'll try and do my best to describe what we're seeing here. My understanding is the following:
I've also confirmed that if our own embedded app uses the same web-vitals library and callbacks (e.g., The above makes total sense and isn't the part where we're seeing anything problematic. The issue is with how web-vitals computes the CLS score based on events emitted by If you have an embedded app that causes layout shifts as a result of new PerformanceObserver((entryList) => {
for (const entry of entryList.getEntries()) {
console.log(entry);
}
}).observe({ type: "layout-shift", buffered: true }); ... you'll notice that If our application had used in-frame tabs instead of Shopify's navigation sidebar, I hope that clarifies the issue and if not I'm happy to provide more sample code, screenshots, logs, etc...! Thanks! |
Just to follow up here -- we ran an experiment on a subset of our users last week where they saw in-frame tab navigation instead of Shopify App Bridge navigation. The following are the CLS metrics we observed as a result:
It seems pretty conclusive to us that using Shopify App Bridge navigation for single page apps is negatively impacting CLS scores due to This is unfortunate for Shopify App Bridge developers that use React or similar Single Page App frameworks as it seems like it incentivizes developers to use a Multi Page App approach (or other approaches like intentionally breaking the Shopify style guide to use in-frame tabs) to "workaround" the CLS issue. However, doing so comes at the expense of overall worse user experience. For example, Shopify mandates a LCP of 2.5 seconds or less 75% of the time [1]. If we went with the Multi Page App approach to "workaround" our CLS score while maintaining a ~1-2 second LCP value we believe it'd be an overall worse user experience as navigating inside our app would take 1-2 seconds as opposed to the instant navigation users have today with our Single Page App approach. Would love to hear your thoughts and recommendations as we'd love to apply for the Built For Shopify program but we're hung up on this particular issue. Thanks! [1] https://shopify.dev/docs/apps/best-practices/performance/admin#largest-contentful-paint |
Hey @ascherkus! Thanks for following up, and I apologize for my delayed response. I'll do my best to set aside some time this week to go through this and attempt to reproduce it locally. Meanwhile, I'd appreciate it if you could provide more information on the following:
Looking forward to hearing from you! |
Thanks for looking into this @bratsos! We use web-vitals' Based on our debugging of App Bridge and using web-vitals inside our app, it seems like Regarding "per session" -- are you referring to per unique app load (i.e., the In any case, I re-ran our metrics by computing percentiles over all metrics, the first CLS metric per app load (based on
I'll message you regarding our app name on Slack. In terms of reproducing I find it's easiest to trigger CLS by switching tabs to hide the page, which as mentioned above [1] https://github.com/GoogleChrome/web-vitals/blob/main/src/onCLS.ts |
@ascherkus were you able to get around this issue and achieve “Built for Shopify” for your app? We are hitting the same issue with our SPA app on Shopify. Our LCP is over 2.5s and we believe this bug is the underlying issue as we have already implemented other best practices around pre loading, compressing JS/CSS, and CDN. |
Has this been solved with the CDN app bridge? |
@TheSecurityDev No, it hasn't yet. We hope to solve it this week. |
Hi @ascherkus @TheSecurityDev we just pushed a fix in App Bridge CDN. May I have your appId to try it out? Action items:
|
@henrytao-me |
@TheSecurityDev I turned it on for your app. Looks good so far. Let's observe in a week. 🙇 |
Thanks! 🚀 |
@TheSecurityDev from what I see, this fixes might not make much difference in your app. So, we could expect the metrics are still stable as-is. |
@TheSecurityDev It's because your app is not affected by CLS score issue |
I don't know what you're seeing on your end, but this is what I've been seeing: But the last one shown is from March 20, which was also when I upgraded to App Bridge v4. |
Your app always has CLS as 1 even with the raw |
I spent over a month last year investigating this issue before I just gave up. The app is randomly reporting that the entire iframe has shifted, which results in a CLS of 1. I'm using NextJS, and I know of one other person with the exact same issue on NextJS, so I don't think it's just me. Also I'm not exactly sure what a raw This could also just be an issue with SPA apps like NextJS. I see there's an open issue about a similar thing on the web-vitals library. It looks like there's a draft PR aimed at trying to solve this issue, although I don't know how relevant it is to this issue. |
I am taking a look of NextJS setup now. I will let you know if I find anything that you can apply to your app.
What I mean is that we are using this web-vitals library. It emits your CLS of 1, even for initial load. So, there is nothing App Bridge can do about it. It should be fixed within your NextJS setup. I can dig into it a bit as I said above.
I don't think this is the case because other apps don't have this issue. WebVitals between top frame and iframe are separated.
I looked into all of these too. The way the new fix works is that it will stop collecting and report WebVitals after a side nav is click (or Redirect App AB action is dispatched if you are using ABv3). Action items:
|
We have been experiencing similar issues with our React/Vue.js SPA, and for this reason, we have not been able to achieve the "Built for Shopify" badge for our app. Additionally, we are unable to meet the LCP requirement of 2.5 seconds. Our app ID is 3932641, and we have almost given up on resolving this issue. Sometimes, it feels as though preference is given to simpler apps that receive such badges, and developers who have implemented deep integrations and followed best practices, like single-page applications (SPA), are being penalized. Thanks for looking into it. |
@dev-coderise please add App Bridge CDN to your app for this fix to work. Ref https://shopify.dev/docs/api/app-bridge-library#getting-started |
@TheSecurityDev your LCP looks good now. We will rollout the fix for webvitals tomorrow morning at 9am EST. Note that: only apps using App Bridge CDN haves this fix. You just need to add App Bridge CDN script tag to your app. There is no need to migrate or remove existing App Bridge from npm (although we recommend to migrate your apps over to use CDN version). See https://shopify.dev/docs/api/app-bridge-library#getting-started I close this issue for now. 🙇 |
Describe the bug
Cumulative Layout Shift (CLS) scores measure visual stability and is used to grade Shopify apps.
According to https://web.dev/cls/ the
web-vitals
library (which App Bridge uses) filters out layout shift events that had recent user input. The code in onCLS.ts is pretty straight forward here as well:https://github.com/GoogleChrome/web-vitals/blob/main/src/onCLS.ts
For a single page app (SPA) that navigates between different sections of the app (e.g., by using the App Bridge navigation side bar), this can result in layout shifts due to navigation, but since the user input is in the parent iframe, to
web-vitals
it doesn't filter it out meaning it can negatively impact the CLS score.In a hypothetical app that (say) used tabs inside its iframe for app navigation as opposed to the App Bridge navigation side bar, the
hadRecentInput
flag would be true when a user clicked on a tab to navigate.To Reproduce
Steps to reproduce the behaviour:
hadRecentInput
set to falseWhen the
onCLS
callback fromweb-vitals
is fired, the entries included are ones that were generated by side bar navigation.Expected behaviour
Ideally, App Bridge's use of
web-vitals
could somehow indicate / filter out those layout shift events from being included in the CLS score.Contextual information
Packages and versions
List the relevant packages you’re using, and their versions. For example:
@shopify/app-bridge
@3.7.4
@shopify/polaris
@10.34.0
Platform
Additional context
I'm aware this uniquely affects SPAs as an MPA with sidebar navigation would potentially be a new page load, and hence a new set of web-vitals scores.
Here are some issues / articles I found regarding SPAs and measuring CLS:
GoogleChrome/web-vitals#119
https://web.dev/vitals-spa-faq/
The text was updated successfully, but these errors were encountered: