Shopify / shopify-app-bridge

https://shopify.dev/docs/api/app-bridge
82 stars 9 forks source link

Modal variant="max" (Full screen) breaks other UI components #264

Open michaelhaessig opened 6 months ago

michaelhaessig commented 6 months ago

Describe the bug

We are in the process of migrating from app bridge 3.x to 4.x since now the Full-screen mode seems to be supported.

Issue: we are now seeing that the Modal variant="max" is creating a completely new iframe, which seems to contain a copy of the previous page with the Modal element.

Apps that have other UI elements, for example a custom dropdown component, will still have the reference to the original body, which then causes the dropdown contents to be added to the original document body and not the copied one inside the modal. (Very common that dropdowns add the contents to the body). This copy behavior breaks a lot of UI components.

Is there any way to prevent the creation of a second iframe? The previous 3x version Full-screen mode also did not create a new iframe and everything is working fine.

To Reproduce

Original embeded app: CleanShot 2023-12-21 at 16 40 22@2x

Second Iframe for modal: CleanShot 2023-12-21 at 16 39 33@2x

Expected behaviour

The current page is kept and the modal is opened with the contents provided. App bridge v3 Fullscreen mode works without any issues.

Contextual information

Packages and versions

App bridge v4

oxxn commented 6 months ago

Seconding this. Many UI components inside this modal iframe are broken. For example, I can't get a button to work inside it. (I might be doing something else wrong, but it seems to be the same issue).

You can reproduce this with a Remix route:


import { Button } from "@shopify/polaris";
import { useEffect } from "react";

export default function Modal() {
  function handleOnClick(e) {
    console.log("received click event"); // does not log
  }

  useEffect(() => {
    document.getElementById("test-modal").show();
  }, []);

  return (
    <ui-modal id="test-modal" variant="max">
      <Button onClick={handleOnClick}>A Button</Button>
    </ui-modal>
  );
}
henrytao-me commented 5 months ago

Hi @michaelhaessig

Apps that have other UI elements, for example a custom dropdown component, will still have the reference to the original body, which then causes the dropdown contents to be added to the original document body and not the copied one inside the modal. (Very common that dropdowns add the contents to the body). This copy behavior breaks a lot of UI components.

When the 2nd iframe is loaded, it will suppress all the DOM element on the page. None of the existing elements are shown if you have App Bridge CDN defined on the tag.

Hi @oxxn

Seconding this. Many UI components inside this modal iframe are broken. For example, I can't get a button to work inside it. (I might be doing something else wrong, but it seems to be the same issue).

For React usage of this ui-modal content, you need to setup React Portal https://react.dev/reference/react-dom/createPortal for the ui-modal children. We are working on a new App Bridge React library that makes it easier for React consumer.

michaelhaessig commented 5 months ago

@henrytao-me i'm not sure i understand your comment or how this can be helpful in making other components work again.

For example, the page can have top level components with their JS logic attached, when app bridge extracts to full Modal content into a new iframe, all logic around the Modal is lost. (There are many JS frameworks that have top level controllers - breaking all these frameworks does not seem like a good dev experience).

Furthermore, components inside the modal still seem to have the references to the main document node, and for example a dropdown component that adds new elements to the DOM is not working. (Many dropdown components work like this - child elements are added to the <body> and are absolutely positioned.

May I ask why this decision of a new iframe was made? In app bridge v3 everything works well without the need for a new iframe.

vincaslt commented 5 months ago

Hi @michaelhaessig

Apps that have other UI elements, for example a custom dropdown component, will still have the reference to the original body, which then causes the dropdown contents to be added to the original document body and not the copied one inside the modal. (Very common that dropdowns add the contents to the body). This copy behavior breaks a lot of UI components.

When the 2nd iframe is loaded, it will suppress all the DOM element on the page. None of the existing elements are shown if you have App Bridge CDN defined on the tag.

Hi @oxxn

Seconding this. Many UI components inside this modal iframe are broken. For example, I can't get a button to work inside it. (I might be doing something else wrong, but it seems to be the same issue).

For React usage of this ui-modal content, you need to setup React Portal react.dev/reference/react-dom/createPortal for the ui-modal children. We are working on a new App Bridge React library that makes it easier for React consumer.

What are we supposed to do with the portals though? I tried rendering the whole ui-modal in the portal, doesn't change anything (and breaks SSR), I tried rendering the content outside, no clue what the target element should even be, can't render into the modal's content by id because it's an iframe.

nullndr commented 5 months ago

Any news about this? Has been already a month...

irahulranjan commented 4 months ago

Any updates on this?

jzazove commented 4 months ago

We are working on fix right now.

irahulranjan commented 4 months ago

Thanks for the update @jzazove. This fix would help us migrate the WYSIWYG editor into the max modal.

darrynten commented 4 months ago

@henrytao-me how do we do this when we are not using React? Is there a way to gain back the expected document context for the modal and pass it to scripts that need to run inside the modal?

forsbergplustwo commented 4 months ago

Unfortunately due to the design of the new fullscreen modals in App Bridge v4 we've decided to simply not use them. It results in a slightly inferior experience for users, but this library requires too much bending over backwards to make them worth it in it's current design.

CleanShot 2024-03-07 at 11 27 06@2x

I know the team are working on trying to improve this (and sorry if this is de-motivating 💚), but IMHO the fundamental design of how these modals work is neither intuitive, stable or documented. Hopefully we can revisit this in the future.

Zetxus commented 4 months ago

We're in the same boat as @irahulranjan , I feel most apps that have a WYSIWYG editor will be impacted by this. We modeled ours to be as close as possible to the Shopify theme editor which is why now we're facing this problem - we can't move away from the full screen as we need the space and we can't use it, as the DOM loses its context.

We either need reliable access to the new iframe's document+window so we can propagate those to our code or just get rid of the iframe altogether.

irahulranjan commented 3 months ago

Latest App bridge is now mandatory @jzazove. Do we have to move away from full screen to avoid being de-listed? https://shopify.dev/changelog/new-shopify-app-store-apps-require-the-latest-app-bridge Can you please help?

jzazove commented 3 months ago

Nope. While the latest version of App Bridge is required for new app submissions, we made it interoperable with App Bridge 3 to allow app developers to migrate on their timetable.

We now have a src attribute for the modal as documented here: https://shopify.dev/docs/api/app-bridge-library/web-components/ui-modal#uimodalelement-propertydetail-src. We expect this should solve the above issues.

rockposdev commented 3 months ago

Is there any news on this issue? we can't make it works even with src

jzazove commented 3 months ago

@rockposdev could describe what isn't working? Using the src attribute or even using an iframe inside the content should allow developers to really build anything now.

rockposdev commented 3 months ago

@rockposdev could describe what isn't working? Using the src attribute or even using an iframe inside the content should allow developers to really build anything now.

We are using src in the ui-modal, but always got this error image

If we use ReactJS components in the ui-modal, then all the event seems doesn't work, like onClick,... Is there any example which show that it works? I don't see much example on Shopify Polaris document. @jzazove

rockposdev commented 3 months ago

@jzazove I've wonder if we use this component is ok for the newest require from Shopify https://shopify.dev/docs/api/app-bridge-library/react-components/modal?

jzazove commented 3 months ago

@rockposdev could you share the code snippet that resulted in that error?

henrytao-me commented 3 months ago

@rockposdev Modal src should have same origin as your main app origin.

irahulranjan commented 3 months ago

any updates on this?

Zetxus commented 3 months ago

The addition of the src is great, but in our case getting the document and the window of the new iframe is what we're after.

We're using some libraries that require those so we can propagate preloaded styles and clean up some of the existing ones from the original window/document.

Without them we run into issues like unloaded fonts and styles interfering with our previews, as the max-modal will basically contain a preview along with some controls.

gmays commented 1 month ago

Another example: In our app the calendar using DatePicker within a Popover is displayed BEHIND the max modal, thus making it unusable.

Screenshot 2024-05-24 at 9 15 23 AM

This is when I delete the max modal from the dom while having focus on the date field.

Screenshot 2024-05-24 at 9 15 03 AM

jzazove commented 1 month ago

@gmays are you using the src attribute with the modal API?

gmays commented 1 month ago

@gmays are you using the src attribute with the modal API?

Thanks, will try that!

danyn commented 4 weeks ago

This works like here: friendlier explanation

IDK it was confusing or stressful in the docs which are very terse. Most people are using react remix so why not give an example of how to use it there?

TemurbekRuziyev commented 4 weeks ago

Hello @jzazove Can you please provide the example of src attribute usage? It's very poor documented in the Shopify docs