Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.43k stars 1.99k forks source link

Unable to Create New Page In Mobile Browser #93852

Closed cat-og closed 2 weeks ago

cat-og commented 2 months ago

Quick summary

I am unable to create a new page in my mobile browser. Tested in both Safari and Chrome; when I create a new page from the Dashboard, it takes a moment to load. The patterns appear briefly, and then the page refreshes itself. The patterns appear again for a short time, and then the entire page closes and I see a browser error.

iPhone 15 iOS 17.5.1 Tested in Chrome and Safari

Steps to reproduce

  1. Start on mobile Dashboard in a browser, not the Jetpack app
  2. Use mobile Dashboard navigation to Create a New Page
  3. The new page will not load, and the browser will eventually return an error message.

What you expected to happen

A new post will be created which I can then edit and publish.

What actually happened

The browser returns an error message. If I hit back from there, it brings me to the last post I was working on, not back to the Dashboard.

https://github.com/user-attachments/assets/5210d62f-f34f-4259-8716-feda0d03e549

Impact

Most (> 50%)

Available workarounds?

No but the platform is still usable

If the above answer is "Yes...", outline the workaround.

No response

Platform (Simple and/or Atomic)

Simple, Atomic

Logs or notes

No response

TimBroddin commented 2 months ago

📌 REPRODUCTION RESULTS

I can reproduce.

📌 FINDINGS/SCREENSHOTS/VIDEO Same results as above.

xavier-lc commented 2 months ago

Unassigning myself, I only own Android devices and the flow works fine on them.

TimBroddin commented 2 months ago

I'm having a hard time remote debugging this on my iPhone, but my hunch is that this is an OOM error, caused by rendering the block previews.

The heap sizes of Calypso + editor iframe + block previews probably overflows the max memory allowed (which depending on source is somewhere between 384 and 500 MB).

The<BlockPreview /> component lives in Gutenberg: https://github.com/WordPress/gutenberg/tree/trunk/packages/block-editor/src/components/block-preview.

The modal lives here: https://github.com/Automattic/wp-calypso/tree/trunk/packages/page-pattern-modal

@p-jackson (don't bother reading this until you get back from leave): as the author of the pattern modal, do you have any idea on how to best resolve this?

Imran92 commented 2 months ago

I'm having a hard time remote debugging this on my iPhone, but my hunch is that this is an OOM error, caused by rendering the block previews.

The heap sizes of Calypso + editor iframe + block previews probably overflows the max memory allowed (which depending on source is somewhere between 384 and 500 MB).

The<BlockPreview /> component lives in Gutenberg: https://github.com/WordPress/gutenberg/tree/trunk/packages/block-editor/src/components/block-preview.

The modal lives here: https://github.com/Automattic/wp-calypso/tree/trunk/packages/page-pattern-modal

@p-jackson (don't bother reading this until you get back from leave): as the author of the pattern modal, do you have any idea on how to best resolve this?

Wondering if https://github.com/Automattic/wp-calypso/issues/83472 has fixed this issue as well

Imran92 commented 2 months ago

Removing the "Needs Triage" label as I was able to reproduce it in my phone as well following the instructions

https://github.com/user-attachments/assets/6039da4e-2590-4501-a33a-8afdf970c1d1

miksansegundo commented 1 month ago

I can also reproduce it on a real iOS device. It looks like a memory issue. It doesn't happen all the times and seems to be triggered by the scroll.

Note this modal has been migrated from the ETK plugin to the Jetpack-mu-wpcom plugin and uses the npm package page-pattern-modal. See /features/starter-page-templates/

I haven't found any clue on bottlenecks or memory leaks when doing a performance and a memory analysis in Chrome Dev tools. I'm going to use my sandbox to test some changes to try to reduce the scope of the code causing this bug.

miksansegundo commented 1 month ago

This looks like a rendering issue with a memory leak. I haven't reproduced the page crash when the page loads but I can reproduce it by just doing clicks on the screen, no need to scroll.

https://github.com/user-attachments/assets/e1720ef1-9c7c-4c50-aa28-6f72ae606ed3

I'll continue debugging the rendering in Chrome Dev Tools next week. I expect to find a React memo that is not working as expected or it's missing to prevent unnecessary renders.

miksansegundo commented 1 month ago

This issue is affecting the pattern rendering in all the editors. Safari mobile crashes when exploring patterns in the page, post and site editors.

I have created an issue in Gutenberg to address the pattern rendering performance issue there.

Edit: The issue could be in some plugin in the Dotcom side because I cannot reproduce it on a core site. Maybe it's because of the number of patterns because a Dotcom site has many more patterns from our library.

miksansegundo commented 1 month ago

@TimBroddin I have found that reverting the diff D109323-code to use the map provider mapbox rather than mapkit (which uses Apple maps in iOS) improves the pattern rendering performance a lot and helps prevent the browser crash. Do you know the context of using mapkit?

Using Mapbox won't fix the issue totally but it helps a lot. Otherwise, Safari crashes every time a pattern with an Apple map is rendered. Sometimes on the first load, others on the second load but it always crashes.

I believe the root cause is multiple issues because of the features Dotcom adds to the editor. I'm still debugging the features in the plugin jetpack-mu-wpcom one by one to find more issues.

The modal itself can also use more memoizations. I'll follow up with more later with a PR.

This GB performance issue can affect here too.

TimBroddin commented 1 month ago

@miksansegundo Thanks for digging into this.

The main reason for switching the map block to Mapkit was savings. We spent a ton on Mapbox, and Mapkit is free.

We could use Mapbox or a static image when rendering the pattern selector if we can somehow determine that we're being rendered there. The downside would be that the map would look different on insertion. I can look into that if you want.

p-jackson commented 1 month ago

Would a quick fix simply be to bypass the modal on mobile devices and just start with a blank page? It feels warranted given this currently blocks the "create new pager" flow on mobile.

@p-jackson (don't bother reading this until you get back from leave): as the author of the pattern modal, do you have any idea on how to best resolve this?

Things have definitely changed in this modal since it was first created. A lot about patterns has changed. It originally showed jpeg screenshots of the patterns which is much more memory friendly. Check out this page 359fb-pb/. But this was all before patterns were in the core editor.

Now the block inserter menu shows live previews of patterns, and using mShots isn't an approach that would work for Core. By using dynamic previews it means the previews are more easily translatable and can use the correct styling based on theme.json. Using an mShot approach would mean taking jpeg screenshots of many many permutations of each pattern.

But yeah, all those iframes are pretty heavy 😬

I was just testing the block inserter menu on my phone, and even some of those pattern previews cause the site to crash for me!

TimBroddin commented 1 month ago

I did some digging and using something like this works:

useEffect(() => {
        if ( document.querySelectorAll('body.modal-open').length > 0 ) {
            setIsInsidePatternList(true);
        }
    }, []);

if ( preview ||  isInsidePatternList ) {
        const mapStyleObject = styles.find( styleObject => styleObject.name === mapStyle );

        content = (
            <div className="wp-block-jetpack-map__map_wrapper">
                <img
                    alt={__('Map Preview', 'jetpack')}
                    src={mapStyleObject ? mapStyleObject.preview : previewPlaceholder}
                    style={{ width: '100%', height: 'auto' }}
                />
            </div>
        );
...
}

(Note that there's already a preview attribute that triggers the display of a jpg instead of a full map here).

However, it would be great if there was a hook useIsInsideBlockPreview that would get this value from a provider higher up. Checking the existence of a class name feels hacky.

obenland commented 1 month ago

@TimBroddin Any update on this?

annezazu commented 3 weeks ago

Noting that in the future, Core is headed towards removing the modal and instead rely on adding patterns directly from an Inserter: https://github.com/WordPress/gutenberg/issues/63865 In the meantime though, this needs a fix.

TimBroddin commented 3 weeks ago

@TimBroddin Any update on this?

If this pattern picker is going to go away in the future, the hacky way I suggested above might be a viable option 👍

davemart-in commented 2 weeks ago

Removing this from The One Board and removing the Groundskeeper label since Quake team took ownership.