Support overlays in react portals in different document #12445

vividviolet · 2024-07-23T23:23:36Z

WHY are these changes introduced?

Fix issues related to overlays not working correct in App Bridge modals as outlined in this doc.

These specific issues will be fixed

WHAT is this pull request doing?

Currently Overlays are broken when rendered using React portals. This PR fixes the issue by doing the following:

ensuring all references to document are updated to be node.ownerDocument instead so the positioning can be calculated correctly inside of iframes
ensuring wherever we do instanceOf HTMLElement we instead just check to see if the node is present or try to call the related methods (getBoundingClientRect) with a fallback as the instanceof check will fail when using React portals for iframe content since iframes have their own globals

This PR only touches the related utils for Overlays but there are other references to document and instanceOf HTMLElement we should also update.

Expected usage with App Bridge v4, note that you have to wrap the modal's content with the AppProvider from Polaris so that the portals are created in the correct place.

For example, here's a modal with tooltips inside:

import {AppProvider} from '@shopify/polaris';
import {Modal} from '@shopify/app-bridge-react';

function App() {
  return (
    <Modal>
      <AppProvider i18n={{}}>
        <div style={{padding: '75px 0'}}>
          <Tooltip content="This order has shipping labels.">
            <Text fontWeight="bold" as="span">
              Order #1001
            </Text>
          </Tooltip>
          <ButtonGroup variant="segmented" fullWidth>
            <Tooltip content="Bold" dismissOnMouseOut>
              <Button>B</Button>
            </Tooltip>
            <Tooltip content="Italic" dismissOnMouseOut>
              <Button>I</Button>
            </Tooltip>
            <Tooltip content="Underline" dismissOnMouseOut>
              <Button>U</Button>
            </Tooltip>
            <Tooltip content="Strikethrough" dismissOnMouseOut>
              <Button>S</Button>
            </Tooltip>
          </ButtonGroup>
        </div>
      </AppProvider>
    </Modal>
  );
}

Before

Screen.Recording.2024-07-23.at.7.22.29.PM.mov

After
https://github.com/user-attachments/assets/6c1bf97a-e8f4-4715-9960-9f5709222f75

az_recorder_20240730_133807.mp4

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Open the App Bridge Playground: https://admin.shopify.com/store/trish-dev/apps/app-bridge-next-playground-4/react/index.html
Open all the custom content modals and do a smoke test to verify overlay components are rendered on top with correct sizing and click events are working

🎩 checklist

Tested a snapshot
Tested on mobile
Tested on multiple browsers
~~- [ ] Tested for accessibility~~
~~- [ ] Updated the component's README.md with documentation changes~~
~~- [ ] Tophatted documentation changes in the style guide~~

vividviolet · 2024-07-23T23:24:37Z

/snapit

vividviolet · 2024-07-23T23:47:51Z

/snapit

github-actions · 2024-07-23T23:49:35Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240723234825"

bakura10 · 2024-07-25T01:55:55Z

Hi @vividviolet ,

Thanks for working on this, this is a huge frustration of App Bridge v4. I have tried the snapshot and unfortunately for now it is still not working (at least for the color picker component, in max modals). They still appear below the actual modal.

vividviolet · 2024-07-25T19:09:33Z

Hi @vividviolet ,

Thanks for working on this, this is a huge frustration of App Bridge v4. I have tried the snapshot and unfortunately for now it is still not working (at least for the color picker component, in max modals). They still appear below the actual modal.

Sorry, I haven't actually worked on the color picker yet - I'll take a look now.

bakura10 · 2024-07-26T00:45:30Z

@vividviolet , unfortunately none of them work. Here are the ones I have tried that do not work:

Popover
Color picker
Tooltip
Date picker

It therefore seems something is missing from the PR, as I can't reproduce what you're having in the video, those components also still fail in modal.

bakura10 · 2024-07-26T00:50:32Z

I realized I forgot to wrap the modal by the AppProvider. Some components like Tooltip don't render at all, some like the Popover seems to appear but at a completely incorrect place.

vividviolet · 2024-07-26T14:04:03Z

@bakura10 You're wrapping the Modal's content with the App Provider correct? It's not the modal but the content that has to be wrapped. Can you try using this to test?

import {useState, useRef, useMemo, useCallback} from 'react';
import {createRoot} from 'react-dom/client';
import {Modal, TitleBar} from '@shopify/app-bridge-react';
import {
  AppProvider,
  Button,
  Popover,
  ActionList,
  Combobox,
  Listbox,
  Tooltip,
  Text,
  ButtonGroup,
  Page,
  Layout,
} from '@shopify/polaris';
import '@shopify/polaris/build/esm/styles.css';

const root = createRoot(document.querySelector('main')!);

function App() {
  return (
    <AppProvider i18n={{}}>
      <Page>
        <Layout>
          <Layout.Section>
            <Text as="h2" variant="headingMd">
              Custom content modals
            </Text>
            <ReactModal title="React Modal with action list">
              <ActionListInPopoverExample />
            </ReactModal>
            <ReactModal title="React Modal with combo box">
              <ComboboxExample />
            </ReactModal>
            <ReactModal title="React Modal with with tooltips">
              <TooltipExample />
            </ReactModal>
          </Layout.Section>
        </Layout>
      </Page>
    </AppProvider>
  );
}

root.render(<App />);

function ReactModal({children, title}: any) {
  const modalRef = useRef<UIModalElement>(null);
  return (
    <>
      <Modal ref={modalRef}>
        <TitleBar title={title}></TitleBar>
        <AppProvider i18n={{}}>
          <Page>{children}</Page>
        </AppProvider>
      </Modal>
      <button
        onClick={() => {
          modalRef.current?.show();
        }}
      >
        {title}
      </button>
    </>
  );
}

function ActionListInPopoverExample() {
  const [active, setActive] = useState(false);

  const toggleActive = useCallback(
    () => setActive((active) => !active),
    [],
  );

  const handleImportedAction = useCallback(
    () => console.log('Imported action'),
    [],
  );

  const handleExportedAction = useCallback(
    () => console.log('Exported action'),
    [],
  );

  const activator = (
    <Button id="button" onClick={toggleActive} disclosure>
      More actions
    </Button>
  );

  return (
    <div style={{minHeight: '250px'}}>
      <Popover
        active={active}
        activator={activator}
        onClose={toggleActive}
        preferredPosition="below"
        fullWidth
        fullHeight
        fluidContent
      >
        <ActionList
          actionRole="menuitem"
          items={[
            {
              content: 'Import file',
              onAction: handleImportedAction,
            },
            {
              content: 'Export file',
              onAction: handleExportedAction,
            },
          ]}
        />
      </Popover>
    </div>
  );
}

function ComboboxExample() {
  const deselectedOptions = useMemo(
    () => [
      {value: 'rustic', label: 'Rustic'},
      {value: 'antique', label: 'Antique'},
      {value: 'vinyl', label: 'Vinyl'},
      {value: 'vintage', label: 'Vintage'},
      {value: 'refurbished', label: 'Refurbished'},
    ],
    [],
  );

  const [selectedOption, setSelectedOption] = useState<string | undefined>();
  const [inputValue, setInputValue] = useState('');
  const [options, setOptions] = useState(deselectedOptions);

  const escapeSpecialRegExCharacters = useCallback(
    (value: string) => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'),
    [],
  );

  const updateText = useCallback(
    (value: string) => {
      setInputValue(value);

      if (value === '') {
        setOptions(deselectedOptions);
        return;
      }

      const filterRegex = new RegExp(escapeSpecialRegExCharacters(value), 'i');
      const resultOptions = deselectedOptions.filter((option) =>
        option.label.match(filterRegex),
      );
      setOptions(resultOptions);
    },
    [deselectedOptions, escapeSpecialRegExCharacters],
  );

  const updateSelection = useCallback(
    (selected: string) => {
      const matchedOption = options.find((option) => {
        return option.value.match(selected);
      });

      setSelectedOption(selected);
      setInputValue((matchedOption && matchedOption.label) || '');
    },
    [options],
  );

  const optionsMarkup =
    options.length > 0
      ? options.map((option) => {
          const {label, value} = option;

          return (
            <Listbox.Option
              key={`${value}`}
              value={value}
              selected={selectedOption === value}
              accessibilityLabel={label}
            >
              {label}
            </Listbox.Option>
          );
        })
      : null;

  return (
    <div style={{height: '400px'}}>
      <Combobox
        activator={
          <Combobox.TextField
            onChange={updateText}
            label="Search tags"
            labelHidden
            value={inputValue}
            placeholder="Search tags"
            autoComplete="off"
          />
        }
      >
        {options.length > 0 ? (
          <Listbox onSelect={updateSelection}>{optionsMarkup}</Listbox>
        ) : null}
      </Combobox>
    </div>
  );
}

function TooltipExample() {
  return (
    <div style={{padding: '75px 0'}}>
      <Tooltip content="This order has shipping labels.">
        <Text fontWeight="bold" as="span">
          Order #1001
        </Text>
      </Tooltip>
      <ButtonGroup variant="segmented" fullWidth>
        <Tooltip content="Bold" dismissOnMouseOut>
          <Button>B</Button>
        </Tooltip>
        <Tooltip content="Italic" dismissOnMouseOut>
          <Button>I</Button>
        </Tooltip>
        <Tooltip content="Underline" dismissOnMouseOut>
          <Button>U</Button>
        </Tooltip>
        <Tooltip content="Strikethrough" dismissOnMouseOut>
          <Button>S</Button>
        </Tooltip>
      </ButtonGroup>
    </div>
  );
}

vividviolet · 2024-07-26T14:08:31Z

@bakura10 Oh I do see issues using the max Modal with the Tooltips and the the Combo Box starting at the Tablet breakpoint of 768px. I'll look into fixing these as well. The other Modal variants seem to be working fine.

Screen.Recording.2024-07-26.at.10.02.35.AM.mov

vividviolet · 2024-07-26T16:13:58Z

/snapit

github-actions · 2024-07-26T16:17:07Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240726161552"

vividviolet · 2024-07-26T21:37:56Z

/snapit

github-actions · 2024-07-26T21:39:52Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240726213840"

vividviolet · 2024-07-26T21:41:01Z

@bakura10 I just fixed the sizing issues when using a max modal. Also just fixed the Color Picker. Give it a try.

Screen.Recording.2024-07-26.at.5.38.45.PM.mov

bakura10 · 2024-07-27T09:39:26Z

@vividviolet this now works, thanks a lot! However i've noticed another problem. When you open a popover for instance (or a color picker) in a modal, it does not close if you click outside of it:

<Popover
   active={isPopoverActive}
   activator={button}
   onClose={() => setIsPopoverActive(false)}
>

Using the same code outside the modal properly works. So I think there might be another error where the click on outside is not properly detected in this context.

vividviolet · 2024-07-30T01:09:48Z

/snapit

github-actions · 2024-07-30T01:11:41Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240730011030"

vividviolet · 2024-07-30T01:15:27Z

/snapit

vividviolet · 2024-07-30T15:14:24Z

/snapit

github-actions · 2024-07-30T15:16:19Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240730151508"

bakura10 · 2024-07-30T15:22:45Z

I can confirm this fixes all the issues! Thanks for working on that @vividviolet . This should be coordinated with the App Bridge team to update the documentation and remove the limitations outlined in the doc ;)

vividviolet · 2024-07-30T16:22:14Z

/snapit

github-actions · 2024-07-30T16:24:14Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240730162303"

vividviolet · 2024-07-30T16:53:20Z

/snapit

github-actions · 2024-07-30T16:55:13Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240730165402"

vividviolet · 2024-07-30T19:56:47Z

/snapit

github-actions · 2024-07-30T19:58:46Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240730195732"

alex-page

Thank you @vividviolet. Appreciate the thorough tests with this as well.

vividviolet · 2024-07-31T16:22:09Z

polaris-react/src/components/ColorPicker/ColorPicker.tsx

@@ -135,7 +142,6 @@ export class ColorPicker extends PureComponent<ColorPickerProps, State> {
        </div>
        <HuePicker hue={hue} onChange={this.handleHueChange} />
        {alphaSliderMarkup}
-        <EventListener event="resize" handler={this.handleResize} />


Removed this because we're now using resize observer on the colorNode

vividviolet · 2024-07-31T16:23:10Z

polaris-react/src/components/DropZone/DropZone.tsx

@@ -294,12 +298,24 @@ export const DropZone: React.FunctionComponent<DropZoneProps> & {
  useEventListener('dragover', handleDragOver, dropNode);
  useEventListener('dragenter', handleDragEnter, dropNode);
  useEventListener('dragleave', handleDragLeave, dropNode);
-  useEventListener('resize', adjustSize, isServer ? null : window);


Removed this because we're now using resize observer on the node

kyledurand

This looks great to me. Thanks for all the test coverage. I think I maybe saw that a 3p dev tested this in the app? Is there any way we can 🎩 directly via staging?

.changeset/four-kids-nail.md

vividviolet · 2024-07-31T18:06:58Z

This looks great to me. Thanks for all the test coverage. I think I maybe saw that a 3p dev tested this in the app? Is there any way we can 🎩 directly via staging?

@kyledurand Let me see if there's an staging app we can use for testing

…odals for embedded apps - ensure references to document and window are updated to the component's ownerDocument so when elements are moved via React portal to an iframe the position can be calculated correctly - ensure wherever we do instanceOf HTMLElement|Element we instead just check to see if the node is present or try to call the related methods (getBoundingClientRect) with a fallback since the instanceof check doesn't work when elements are rendered inside of an iframe which has its own scoped HTMLElement|Element

vividviolet · 2024-07-31T18:52:40Z

/snapit

github-actions · 2024-07-31T18:54:42Z

🫰✨ Thanks @vividviolet! Your snapshot has been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240731185326"

kyledurand · 2024-08-01T00:28:14Z

Was able to 🎩 staging. Thanks @vividviolet I think this is good to go

@vividviolet

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@13.9.0 ### Minor Changes - [#12445](#12445) [`66acfb1c9`](66acfb1) Thanks [@vividviolet](https://github.com/vividviolet)! - Fixed issues with Popover, Tooltip, Color Picker and Drop Zone not working correctly when used inside Modals for embedded apps. ## polaris.shopify.com@1.0.13 ### Patch Changes - Updated dependencies \[[`66acfb1c9`](66acfb1)]: - @shopify/polaris@13.9.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

DeskmozUser · 2024-08-02T09:58:46Z

Hey can anyone provide me the code to use thin for popover with datePicker in shopify polaris

darrynten · 2024-08-03T05:55:51Z

Does this PR resolve the missing height values outlined in Shopify/shopify-app-bridge#405

bakura10 · 2024-08-08T12:57:28Z

@vividviolet , those changes work quite well, but I've noticed an edge case where it works unreliable. If you include a popover as part of a <details> element, clicking on it won't open the popover accurately:

<details>
  <summary>Your details</summary>

  <Popover activator={activator} active={active}>
    Something
  </Popover>
</details>

If you:

Click on the summary to open the details.
Click on the activator.
Most of the time, the popover won't open.

It is probably due to the fact that the popover is hidden completely initially, so some listeners might not be hook correctly. If the details element is pre-open by adding the open attribute, everything works as expected.

vividviolet · 2024-08-10T01:15:14Z

Does this PR resolve the missing height values outlined in Shopify/shopify-app-bridge#405

It doesn't fix that issue unfortunately. I'll reply in the other issue.

vividviolet · 2024-08-10T01:19:08Z

Hey can anyone provide me the code to use thin for popover with datePicker in shopify polaris

Do you mean something like this?

function DatePickerExample() {
  const [{month, year}, setDate] = useState({month: 1, year: 2018});
  const [selectedDates, setSelectedDates] = useState({
    start: new Date('Wed Feb 07 2018 00:00:00 GMT-0500 (EST)'),
    end: new Date('Mon Mar 12 2018 00:00:00 GMT-0500 (EST)'),
  });

  const handleMonthChange = useCallback(
    (month: number, year: number) => setDate({month, year}),
    [],
  );

  const [active, setActive] = useState(true);

  const toggleActive = useCallback(() => {
    setActive((active) => !active);
  }, []);

  const activator = (
    <Button id="button" onClick={toggleActive} icon={CalendarIcon} />
  );

  return (
    <div style={{minHeight: '250px'}}>
      <Popover
        active={active}
        activator={activator}
        onClose={toggleActive}
        preferredPosition="below"
        autofocusTarget="first-node"
        preferredAlignment="left"
      >
        <div style={{width: '300px', padding: '10px'}}>
          <DatePicker
            month={month}
            year={year}
            onChange={setSelectedDates}
            onMonthChange={handleMonthChange}
            selected={selectedDates}
            multiMonth
            allowRange
          />
        </div>
      </Popover>
    </div>
  );
}

vividviolet · 2024-08-10T02:31:27Z

@vividviolet , those changes work quite well, but I've noticed an edge case where it works unreliable. If you include a popover as part of a <details> element, clicking on it won't open the popover accurately:
<details>
  <summary>Your details</summary>

  <Popover activator={activator} active={active}>
    Something
  </Popover>
</details>
If you:

Click on the summary to open the details.

Click on the activator.

Most of the time, the popover won't open.

It is probably due to the fact that the popover is hidden completely initially, so some listeners might not be hook correctly. If the details element is pre-open by adding the open attribute, everything works as expected.

@bakura10 I'm working on a fix here

bakura10 mentioned this pull request Jul 25, 2024

Parent window cannot communicate accurately to child modals due to timing issues Shopify/shopify-app-bridge#404

Closed

bakura10 mentioned this pull request Jul 30, 2024

SaveBar not working inside a Modal with src Shopify/shopify-app-bridge#401

Closed

vividviolet force-pushed the fix-overlay-in-iframes branch 2 times, most recently from 0a77f4e to a9b4db1 Compare July 30, 2024 16:21

vividviolet force-pushed the fix-overlay-in-iframes branch from a9b4db1 to cb9164e Compare July 30, 2024 16:46

vividviolet marked this pull request as ready for review July 30, 2024 16:56

vividviolet force-pushed the fix-overlay-in-iframes branch from cb9164e to b5b89b1 Compare July 30, 2024 19:36

alex-page approved these changes Jul 31, 2024

View reviewed changes

vividviolet commented Jul 31, 2024

View reviewed changes

kyledurand approved these changes Jul 31, 2024

View reviewed changes

.changeset/four-kids-nail.md Outdated Show resolved Hide resolved

vividviolet force-pushed the fix-overlay-in-iframes branch from b5b89b1 to db66747 Compare July 31, 2024 18:48

vividviolet merged commit 66acfb1 into main Aug 1, 2024
13 checks passed

vividviolet deleted the fix-overlay-in-iframes branch August 1, 2024 14:15

shopify-github-actions-access bot mentioned this pull request Aug 1, 2024

Version Packages #12488

Merged

vividviolet mentioned this pull request Aug 10, 2024

Fix issue with popovers not working in embedded app modals when put inside of a details component #12534

Draft

6 tasks

Support overlays in react portals in different document #12445

Support overlays in react portals in different document #12445

Conversation

vividviolet commented Jul 23, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

vividviolet commented Jul 23, 2024

vividviolet commented Jul 23, 2024

github-actions bot commented Jul 23, 2024

bakura10 commented Jul 25, 2024

vividviolet commented Jul 25, 2024

bakura10 commented Jul 26, 2024

bakura10 commented Jul 26, 2024

vividviolet commented Jul 26, 2024 • edited Loading

vividviolet commented Jul 26, 2024

vividviolet commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

vividviolet commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

vividviolet commented Jul 26, 2024 • edited Loading

bakura10 commented Jul 27, 2024

vividviolet commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

vividviolet commented Jul 30, 2024

vividviolet commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

bakura10 commented Jul 30, 2024

vividviolet commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

vividviolet commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

vividviolet commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

alex-page left a comment

Choose a reason for hiding this comment

vividviolet Jul 31, 2024

Choose a reason for hiding this comment

vividviolet Jul 31, 2024

Choose a reason for hiding this comment

kyledurand left a comment

Choose a reason for hiding this comment

vividviolet commented Jul 31, 2024

vividviolet commented Jul 31, 2024

github-actions bot commented Jul 31, 2024

kyledurand commented Aug 1, 2024

DeskmozUser commented Aug 2, 2024

darrynten commented Aug 3, 2024

bakura10 commented Aug 8, 2024 • edited Loading

vividviolet commented Aug 10, 2024

vividviolet commented Aug 10, 2024

vividviolet commented Aug 10, 2024 • edited Loading

vividviolet commented Jul 23, 2024 •

edited

Loading

vividviolet commented Jul 26, 2024 •

edited

Loading

vividviolet commented Jul 26, 2024 •

edited

Loading

bakura10 commented Aug 8, 2024 •

edited

Loading

vividviolet commented Aug 10, 2024 •

edited

Loading