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

Stabilize Block Editor APIs used by Isolated Block Editor #57672

Open
jsnajdr opened this issue Jan 9, 2024 · 19 comments
Open

Stabilize Block Editor APIs used by Isolated Block Editor #57672

jsnajdr opened this issue Jan 9, 2024 · 19 comments
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Enhancement A suggestion for improvement.

Comments

@jsnajdr
Copy link
Member

jsnajdr commented Jan 9, 2024

The isolated block editor uses several Block Editor private APIs and that has become an ever bigger liability since Core started intentionally breaking private API usages on every major release. In this issue I'm writing down what exactly is being used and what we need to do to stabilize all these APIs and make them public.

<BlockCanvas>: has two versions. One is exported by @wordpress/block-editor as public API and supports only a limited set of props. The full component is exported only as an ExperimentalBlockCanvas in the private API. The props that would need to be added to the public version are:

  • shouldIframe is by default true, but the isolated editor wants to set this to false. Alternatively this could be solved by making the isolated editor always work in iframed mode, which it currently doesn't support.
  • iframeProps are props for the iframe element, and one thing we need to pass here are the mobile styles from useResizeCanvas.
  • contentRef is widely used across the editor to access the scrolling element of the editor. See the useTypewriter() and usePopoverScroll() description below.

useTypewriter(): watches the content scrollable element in order to scroll the caret into view when user starts typing (@ellatrix did I guess the function of the hook correctly?). This useTypewriter hook should be make public (it's now the old-fashioned private API with an __unstable prefix) and the various __unstableContentRef props should be also made public.

The scrollable content element (usually passed as __unstableContentRef) is also used by the usePopoverScroll() hook which ensures that we can scroll smoothly even when the mouse is over a popover element. IMO this hook currently doesn't work, as it wasn't updated for the iframed editor.

<RecursionProvider> is used to prevent infinite rendering loops, like when a post-content template would contain an inner post-content block that renders the same post. This API (RecursionProvider and useHasRecursion) could easily be stabilized by removing the __experimental prefix. Or maybe it could be integrated into BlockList. And instead of using the provider and the hook explicitly, make it triggered by some block.json attribute, like preventRecursion: { id: 'ref' } (pointing to the name of attribute that serves as unique ID).

useResizeCanvas(): provides extra styles for mobile views. Maybe could be integrated into BlockCanvas.

<LayoutStyle>, useLayoutClasses() and useLayoutStyles(): used to render theme layout styles for the top-level post content. In isolated block editor this code is identical to what EditorCanvas does. So maybe using EditorCanvas directly in the isolated editor could also be possible, eliminating these dependencies, and also others like useResizeCanvas() or useTypewriter().

<Library> is a wrapper around InserterMenu, exported as __experimentalLibrary, is used by everyone who wants to implement an inserter sidebar menu. Let's review its API and stabilize it.

<ListView> is a similar component used to implement block list view in a sidebar. Let's review API and stabilize, too.

useDialog() is a hook in @wordpress/compose, exported with __experimental prefix, used to manage focus on modal elements and to close modals on Esc key press. We should either stabilize it, or replace it with something in Ariakit or in the components package. It feels like an a11y thing that these packages work with all the time (cc @ciampo).

<Text> is exported from @wordpress/components, and used by isolated editor to render list view outline. It's basically a div styled with styles fetched from the component system context. Maybe we can replace it with regular <div> everywhere. I'm not sure if all the extra features are really used.

reusableBlocksStore: the isolated block editor reimplements this store with its own version, and most of this store's actions have the __experimental prefix. And looking at the codebase, I have some doubts whether this store is still used at all (?)

And that's all 🙂 FYI @youknowriad as there is a very big overlap between this and the GaaF project in #53874.

@jsnajdr jsnajdr added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jan 9, 2024
@youknowriad
Copy link
Contributor

This is a quick message as I can't reply entirely right now. But my opinion is that we should deprecate the isolated block editor. Using raw block-editor should be easy enough that the isolated block editor shouldn't be needed. That's what I tried to convey with my several playground stories.

That said, I'll circle back here and try to reply to each of the APIs proposed here.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 9, 2024

But my opinion is that we should deprecate the isolated block editor. Using raw block-editor should be easy enough

That's what the isolated block editor essentially is. It imports various pieces from @wordpress packages and composes them into something like this:

<InterfaceSkeleton
  secondarySidebar={
    isList ? <ListView /> : <Library />
  }
  content={ 
    <KeyboardShortcuts>
      <BlockTools>
        <BlockCanvas shouldIframe={ false } contentRef={ useTypewriter() }>
          <LayoutStyle />
          <LayoutStyle />
          <RecursionProvider>
            <BlockList />
          </RecursionProvider>
        </BlockCanvas>
      </BlockTools>
    </KeyboardShortcuts>
  }
/>

The problem is that half of these pieces are private/experimental APIs.

With the recent GaaF work taken into account, is this editor code going to look fundamentally different? Or at least greatly simplified?

That's what I tried to convey with my several playground stories.

What does "playground stories" mean? 🙂

@spencerfinnell
Copy link

spencerfinnell commented Jan 9, 2024

@jsnajdr I believe @youknowriad is referring to https://github.com/WordPress/gutenberg/blob/trunk/storybook/stories/playground/with-undo-redo/index.js which shows the ongoing efforts of #53874

I think the important distinction between the Isolated Block Editor and "Block Canvas" work done in #53874 is the additional UI provided by the Isolated Block Editor and own internal stores for managing its own sidebars, etc.

I have personally moved away from the Isolated Block Editor with the progress made in <BlockCanvas /> and found it be a pretty nice transition away from the Isolated Block Editor (but did require implementing @wordpress/interface, etc). Custom undo/redo doesn't seem to work 100% with @wordpress/core-data but for me (and for now) it's a reasonable trade-off for not being tied to the other package.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 9, 2024

Thanks for the pointer! Did you have to import any private/experimental APIs from the @wordpress packages to make things work?

I don't mind deprecating the isolated block editor package and building the editor from the "raw" block editor packages. Namely for the P2 editor. But so far it seems to me that it's going to be essentially the same code, just in different repo. Even if it gets to be much simpler, but that simplification can be done in the original isolated block editor package, too.

@spencerfinnell
Copy link

spencerfinnell commented Jan 9, 2024

@jsnajdr Yes, there are still a few components that could be stabilized. I ended up with:

import {
	BlockCanvas,
	BlockEditorProvider,
	BlockList,
	store as blockEditorStore,
	__experimentalListView as ListView,
	__experimentalLibrary as InserterView,
} from '@wordpress/block-editor';

Edit: To to be fair, that is to build the main block editor area. Other things like building a sidebar with new <Tabs /> component still require unlocking private APIs.

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Jan 9, 2024
@ciampo
Copy link
Contributor

ciampo commented Jan 11, 2024

useDialog() is a hook in @wordpress/compose, exported with __experimental prefix, used to manage focus on modal elements and to close modals on Esc key press. We should either stabilize it, or replace it with something in Ariakit or in the components package. It feels like an a11y thing that these packages work with all the time

I took a look at useDialog and, from what I can see, it does a bunch of things:

  • handles constrained tabbing
  • handles moving focus within the dialog on mount,
  • handles returning the focus (if possible) to the previously selected element when closing the dialog
  • causes the dialog to close when:
    • Escape key is pressed
    • focus leaves the dialog (eg. when clicking on the backdrop)

Regarding its APIs:

  • The hook takes one options object as its argument. The available options are: focusOnMount (used to specify what focus strategy to adopt when mounting the dialog) and onClose (a callback used to effectively stop rendering the React component representing the dialog).
  • The hook returns a tuple. The first element is a ref, and the second element is an object of props that are supposed to be applied to the DOM element representing the dialog.

A few considerations:

  • useDialog is currently used in our Popover component (and, as a consequence, in all our popover-based components that haven't been rewritten to Ariakit), although it's notably not used in the Modal component. That is probably because we had found issues with how useDialog checks if the focus leaves the modal element — we had users reporting bugs that the modal would closed unexpectedly if focus was moved to an iframe inside the dialog.
  • it looks like useDialog is also (sparingly) used in other parts of the editor

I'm not sure if Ariakit offers via its public APIs an easy drop-in replacement for the logic of the editor (cc @diegohaz ).
We could stabilize it, and that probably would be the most conservative/safe option.

@diegohaz
Copy link
Member

I'm not sure if Ariakit offers via its public APIs an easy drop-in replacement for the logic of the editor (cc @diegohaz ).
We could stabilize it, and that probably would be the most conservative/safe option.

There's a useDialog hook exported by @ariakit/react-core/dialog/dialog, which is a package that doesn't follow semver. The hook returns props to be passed to a Role component. And this is essentially how the Ariakit Dialog component is implemented:

import { forwardRef } from "react";
import { useDialog } from "@ariakit/react-core/dialog/dialog";
import { Role } from "@ariakit/react";

const Dialog = forwardRef(function Dialog(props, ref) {
  const roleProps = useDialog({ ref, ...props });
  return <Role.div {...roleProps} />;
});

Besides not following semver, I don't believe it can work as a drop-in substitute because the hook might yield custom props that Role understands, but a native div wouldn't.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 22, 2024

I'll circle back here and try to reply to each of the APIs proposed here.

Hi @youknowriad 👋 Pinging you here as a reminder if you still want to share your feedback. Especially after the Drupal workshop which was also about integrating the editor in an "unusual" way.

@youknowriad
Copy link
Contributor

Hi @jsnajdr Sorry for the late response.

BlockCanvas shouldIframe, iframeProps contentRef:

I intentionally kept these props private. They were needed because of our weird implementations and backward compatibility needs in WordPress but from a third-party developer perspective, I would love for the iframe to be default (no need to provide a choice), and for the other props to be not needed. I didn't want to commit to them as API because they felt very unclear from an implementer's perspective.

useTypewriter

This for me also shouldn't be something the implementor thinks about. So maybe this one could be a prop of BlockCanvas or a setting in BlockEditorProvider

RecursionProvider

This feels like an API that blocks should be using rather than the editor. I know that we have one exception to this (the root level post content) but I guess that's fine too. I agree that it feels like a stable enough API at this point. cc @mcsf

useResizeCanvas

I'm torn here. I don't think it should be integrated to BlockCanvas because it would mean that the block editor gains knowledge of devices/breakpoints... which has a lot of complexities to it. Right now I see it as an API to be used in the @wordpress/editor abstraction instead. For third-party block editors that are not "entity editors", maybe it's fine if this is implemented adhoc (by them) for now.

, useLayoutClasses() and useLayoutStyles()

I think passing a "layout" to BlockList and having BlockList render the styles would make sense here. I don't like how these APIs evolved over time because LayoutStyle is just the combination of the two others IMO so not sure why we need the two alternatives. That said, I don't really see EditorCanvas used in the isolated block editor.

Library

Yes, this one is an Inserter without "toggle". I think it's fine to stabilize. Some of the __ex props need to be checked and potentially stabilized too.

ListView

Yes, same but it does feel like it has way too much props. Probably some of these are very specific and don't make sense in a good reusable component.


I hope this helps, I do think we should shift and consider the IsolatedBlockEditor as deprecated though. If there's too much boilerplate to use BlockEditor directly, we should fix these instead and provide examples in our storybook directly. (And update the platform docs as well)

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 23, 2024

I hope this helps, I do think we should shift and consider the IsolatedBlockEditor as deprecated though.

Currently the isolated block editor is used by the P2 theme, to support inline posting and commenting. If we deprecate it, the equivalent P2 code will have exactly the same problems, they will not disappear.

Another similar integration project is "Verbum", a WordPress.com feature to provide inline Gutenberg commenting on site frontends. It's interesting that they are able to almost completely avoid unstable API, for a very similar functionality. There are a few prefixed fields in the editor settings object and that's all.

@youknowriad
Copy link
Contributor

Currently the isolated block editor is used by the P2 theme, to support inline posting and commenting. If we deprecate it, the equivalent P2 code will have exactly the same problems, they will not disappear.

I'm just saying that P2 should be using block-editor directly in this case. There's no need for middle solution. It doesn't negate the fact that we need to stabilize stuff...

@mcsf
Copy link
Contributor

mcsf commented Jan 23, 2024

RecursionProvider

This feels like an API that blocks should be using rather than the editor. I know that we have one exception to this (the root level post content) but I guess that's fine too. I agree that it feels like a stable enough API at this point. cc @mcsf

Are you suggesting that it live in @wordpress/blocks? Otherwise, if it's just about promotion to stable, I have a PR at #58120.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 23, 2024

Are you suggesting that it live in @wordpress/blocks?

Probably not, it's completely normal that 3rd party blocks import APIs for block authors from @wordpress/block-editor. See InspectorControls for example, among many others.

@fjorgemota
Copy link

fjorgemota commented Feb 15, 2024

I'm just saying that P2 should be using block-editor directly in this case. There's no need for middle solution. It doesn't negate the fact that we need to stabilize stuff...

As someone who is working on upgrading Gutenberg on P2 (for the first time), and also having to fiddle with isolated-block-editor to update it fully (because we have weird issues on the frontend editor, where the isolated block editor is used in P2). I want to ask: Are you sure about this, @youknowriad?

I'm asking because, despite the progress on #53874, I'm not sure how easy it would be to make P2 use the block editor directly or if maybe we should still have the isolated block editor somewhere working as a "middleware" to integrate with Gutenberg (but maybe in a more sustainable way, using the components more directly while not relying so much on the private APIs).

Also, I do not know much about Gutenberg internals to confirm that using it directly in P2 is, in fact, feasible, to start with.

Finally, the fact that we have even more teams adopting Isolated Block Editor in Gutenberg makes me wonder how easy this, in fact, is. :)

@youknowriad
Copy link
Contributor

I want to ask: Are you sure about this, @youknowriad?

Yes, did you look at the examples in the storybook. I'm not saying that everything is perfect but IMO, isolated block editor shouldn't have to exist, so if there are things that you still think are unclear or too hard to do, please share.

@fjorgemota
Copy link

Nope, I didn't. And to be honest the more I work on this Gutenberg upgrade in P2, the more I agree with this:

isolated block editor shouldn't have to exist

Isolated Block Editor has quite some code copied directly from Gutenberg, which makes the upgrade process way tougher than it should be.

IMHO, it should be possible to use Gutenberg directly, and that's why I'm kinda interested in learning more about what's possible to do with Gutenberg directly.

However, I'm a bit worried about bringing the idea and suddenly finding that it is not feasible right now, and then we end up with Isolated Block Editor 2.0. :)

@jsnajdr
Copy link
Member Author

jsnajdr commented Feb 16, 2024

@fjorgemota You could start by simply copying the Isolated Block Editor sources into the P2 repository, and then you can start updating them there.

There should be many opportunities to simplify the integration code. The "Gutenberg as a Framework" project has shipped many improvements in the integration API, and the Storybook examples, and also the integration in the Verbum project, they all look much less complex than the current IBE.

@fjorgemota
Copy link

Hey @jsnajdr

You could start by simply copying the Isolated Block Editor sources into the P2 repository, and then you can start updating them there.

I'm not sure I'm following what you mean here. What's your idea? Do this to upgrade Gutenberg? Or to replace it with Gutenberg directly?

Given the current status of P2, I do not think it is a good idea to copy the IBE files into the P2 repository. Also, right now I was mostly researching how feasible is to use Gutenberg directly instead of IBE. We do not have any plans right now to do that migration. Our current plan is to just upgrade P2 and IBE to run on latest Gutenberg as of now.

@jsnajdr
Copy link
Member Author

jsnajdr commented Feb 26, 2024

right now I was mostly researching how feasible is to use Gutenberg directly instead of IBE.

My idea is to do exactly what you describe here: to use Gutenberg directly instead of IBE. If you want to do this, a good way to proceed is to first copy the existing IBE files into P2, and then modify the code there. The modifications would be hopefully mostly simplifying and streamlining the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

8 participants