Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Layout issues around <wp-block> and display: contents #49

Closed
yscik opened this issue Aug 8, 2022 · 24 comments
Closed

Layout issues around <wp-block> and display: contents #49

yscik opened this issue Aug 8, 2022 · 24 comments

Comments

@yscik
Copy link
Collaborator

yscik commented Aug 8, 2022

Themes commonly style the top-level content elements, like .entry-content > *, for layout and spacing, eg to allow various alignments. Example:

.entry-content > :not(.alignwide):not(.alignfull):not(.alignleft):not(.alignright):not(.is-style-wide) {
  max-width: 58rem;
  width: calc(100% - 4rem);
	}
.entry-content > * {
  margin-left: auto;
  margin-right: auto;
  margin-bottom: 1.25em;
}

This doesn't really work with display: contents on the <wp-block> wrapper:

Current Expected
image image

One option is inheriting these spacing styles that would apply to the wrapper:

wp-block > * {
	margin: inherit;
	max-width: inherit;
	width: inherit;
}

I'm not sure that's enough for every theme, this would need wider testing and some strategy to keep compatibility with existing theme and possibly plugin styles.

@luisherranz
Copy link
Member

Should it inherit everything like this?

wp-block > * {
  all: inherit;
}

Or just a few rules?

It'd be great to have a CSS expert look at this.

@yscik
Copy link
Collaborator Author

yscik commented Aug 10, 2022

Bringing this in here from another conversation:

More background

When the theme uses selectors like .entry-content > *, the direct descendants of the .entry-content container would normally be the block wrappers.

With the current setup those descendants however are the <wp-block> elements. With display: contents, they won't have any layout box, but they are still present in the DOM and considered for descendant and other relationship selectors. Without display: contents, the basic styling could still work.

Alignments

What still won't work either with the above inherit rules nor with just display: block for wp-block is support for alignments, because the .alignfull and similar classes are still on the original block wrapper, which is inside the wp-block.

One option to solve all this is to take the block wrapper out of the React component, and treat wp-block as the wrapper, applying the blockProps directly to it via the regular JS DOM API.

Another one is doing an alternative hydrate implementation that can replace the target element instead of mounting the React app as a child node.

That is, of course, if we want to support existing theme styles. We can also require theme devs to do an update to support frontend blocks. I think with block themes the alignment and content width styling is provided by WordPress, so maybe it's only classic themes that'd need to work around this.

@luisherranz
Copy link
Member

luisherranz commented Aug 10, 2022

That is, of course, if we want to support existing theme styles. We can also require theme devs to do an update to support frontend blocks

I don't think that's an option.

One option to solve all this is to take the block wrapper out of the React component, and treat wp-block as the wrapper, applying the blockProps directly to it via the regular JS DOM API.

I don't think we can do anything during the hydration because that would cause a flash. This needs to come as is from the server.

We could try to move the blockProps to <wp-block>, but I don't like that. It's not how Gutenberg was designed (and works in the Editor), so even if it initially works, it can bit us later.

Another one is doing an alternative hydrate implementation that can replace the target element instead of mounting the React app as a child node.

I researched this a bit when we started, and React doesn't support wrapperless hydration. There are some hacky ways to do it, but I'd prefer to avoid those.

But Preact does support wrapperless hydration. It's not documented, though. I remember reading a tweet from @developit about it. I'll try to find it.


At this moment, I'm a bit disappointed with display: contents, to be honest. I thought its point was to make those nodes invisible in terms of CSS 😓😄 I'm not sure what it's useful for. Only flexbox?

@luisherranz
Copy link
Member

@luisherranz
Copy link
Member

Initially, it seems to be working fine: https://codesandbox.io/s/nice-mountain-su6zl1?file=/src/index.js

For this app:

const App = () => <div id="root">content</div>;

And these targets:

const root = document.getElementById("root");
const rootFragment = createRootFragment(
  document.body,
  document.getElementById("root")
);

Using root, I get:

<body>
  <div id="root">
    <div id="root">content</div>
  </div>
</body>

And using rootFragment, I get:

<body>
  <div id="root">content</div>
</body>

@luisherranz
Copy link
Member

Anyway, we should research first if using a custom element is not possible at all. Safari doesn't support custom built-in elements (<div is="..."), so if we can't use <wp-block>, we'd have to use a polyfill or choose a different hydration method.

@yscik
Copy link
Collaborator Author

yscik commented Aug 10, 2022

Nice! I played around using a DocumentFragment as the root with React, and that actually somewhat works too — except for event handlers, which React attaches to the root: https://codesandbox.io/s/upbeat-wozniak-wnfo39?file=/src/index.js

I've been thinking on an alternative to custom elements, and maybe just a wp-block attribute on the block wrapper could do the trick, with a MutationObserver looking for new blocks. (Not sure about the performance, but it has an attributeFilter for early culling.)

@luisherranz
Copy link
Member

I tried attributeFilter to avoid the wp-block wrapper in non-interactive blocks, but unfortunately it only reports changes to that attribute, not new nodes that appear in the DOM with that attribute. I didn't test how much of an impact it would be to use a MutationObserver to do the hydration, however. Maybe it's not too bad.

We can also take a look at how the custom-elements polyfill checks for new elements. Maybe there's another alternative.

@luisherranz
Copy link
Member

@jasmussen: We need some help here 🙂

We'd like some confirmation on whether using a <wp-block> wrapper for interactive blocks is viable in terms of CSS compatibility with current themes. We initially used display: contents, but that doesn't seem to cover all the cases. Peter described the problems he found here.

Would you mind taking a look or pinging someone that could take a look? Thank you very much! 🙏

@DAreRodz
Copy link
Collaborator

We can also take a look at how the custom-elements polyfill checks for new elements. Maybe there's another alternative.

Looking at the code, I see the polyfill looks for nodes being added/removed by subscribing the MutationObserver to childList and subtree changes and using the list of addedNodes and removedNodes. This is the callback it uses.

@jasmussen
Copy link

I've not yet found an excellent use of display: contents;, there's always a downside. In cases where I haven't been able to change the markup, for example to remove an extra wrapping div, I've found limited success in ignoring that wrapper using just the CSS property. But it's never a complete win, so I have a feeling it's not going to be a great solution in the longer run. Happy to be wrong here, though!

We'd like some confirmation on whether using a <wp-block> wrapper for interactive blocks is viable in terms of CSS compatibility with current themes.

I have a feeling you might get a better answer from @scruffian or @mtias. I'm assuming by "interactive blocks" you're referring to anything that uses JS on the frontend, or are there other cases? And by this wrapper, I assume you mean it would be an additional wrapper to what might exist already, right?

I.e. Navigation by default outputs this:

<nav class="is-layout-flex wp-block-navigation" aria-label="Navigation">
...
</nav>

So, essentially a fragment but for the frontend. It's tricky, as any wrapping div will default to being block level, but the block itself might be inline-block, or flex, or inline-flex. Additionally (and as suggested in the initial post), any direct-descendant CSS that assumes blocks are immediate children of the post list will target the wrong element.

Here's a wide image as an example:

<figure class="wp-block-image alignwide size-large">
	<img  ... >
</figure>

Purely in the name of exploring ideas (they might inspire better ones) would you be able to transplant the classes one level up? I.e. this:

<wp-block class="wp-block-image alignwide size-large">
	<figure>
		<img  ... >
	</figure>
</wp-block>

Another question: would limiting the features being explored to just block themes improve predictability? I.e. .wp-block-post-content > .wp-block-cover etc?

Is it impossible to accomplish what you're after without a wrapping element? Hopefully Ben or Matías can help shed some better light, and hope this was helpful.

@michalczaplinski
Copy link
Collaborator

michalczaplinski commented Aug 12, 2022

Anyway, we should research first if using a custom element is not possible at all. Safari doesn't support custom built-in elements (<div is="..."), so if we can't use <wp-block>, we'd have to use a polyfill or choose a different hydration method.

I think using the custom element built-in extends (<div is="wp-block" />) can be a viable alternative to <wp-block>. It's true that Safari does not support it (and looks like they are not going to anytime soon). But an argument could be made that only Apple devices run Safari and they have the fastest CPUs available on earth so adding a small polyfill would not hurt the UX much.

I was following Andrea's work recently (the author of the polyfill) and he's very adamant about his approach to custom element builtin extends being "THE way" 🤷‍♂️:

We used the custom element builtin extends in the earlier version of the BHE. Was there another reason why we removed them other than the lack of Safari compatibility?

@luisherranz
Copy link
Member

luisherranz commented Aug 26, 2022

I'd like to try this alternative wrapperless hydration based on a static virtual DOM: #60

It would not be as simple as switching to Preact to use createRootFragment and adding the <div is="wp-block"> polyfill, but it'll get us much closer to client-side navigations, and it'll remove the need for any inter-island wiring.

@luisherranz
Copy link
Member

I'm having an interesting conversation about the stability of Preact's internal properties in Preact's Slack: https://preact.slack.com/archives/C3M9NTD16/p1661766573099969 (you need to login first). They don't guarantee their stability, but createRootFragment depends on .__k.

Fresh is also using createRootFragment, so Fresh users shouldn't update preact without first looking at the mangle.json file.

For WordPress, if we finally want to access those internal properties, we would have to:

  • Fix the preact version.
  • Add it as a regular dependency in @wordpress/element (not as a peer dependency).
  • Always check the mangle.json file before updating the version.

@luisherranz
Copy link
Member

A bit more info about that conversation:

  • They consider createRootFragment internals quite stable.

    It's probably worth emphasizing that the linked gist (which fresh and preact-island use) replaces a deprecated parameter in v10. While yes, it does result in reliance upon __k, the alternative is using something that's deprecated and removed in v11. In that sense it's more stable.

  • They consider Option Hooks internals quite stable.

    This is Jason Miller's comment in Slack:

    we have historically considered backwards-incompatible changes to any options hook (mangled or otherwise) to be a breaking change and only done so in semver major releases. It's done because we have pieces of the ecosystem that rely on those hooks, and otherwise they'd be unversioned.

    A lot of the mangled names are mangled with an underscore prefix for size reasons and to prevent developers from reaching for those properties in cases where they're not actually needed. Rather than thinking of them as an undocumented/unversioned API, we generally think of them as "Preact's plugin API".

    Although this is what the Preact's docs say:

    Option Hooks are shipped in Preact, and as such are semantically versioned. However, they do not have the same deprecation policy, which means major versions can change the API without an extended announcement period leading up to release. This is also true for the structure of internal APIs exposed through Options Hooks, like VNode objects.

So I think this is the situation:

  • Mangled properties are very stable, although they don't have the full guarantee of semantic versioning.
  • We should be fine using createRootFragment.
  • If we want to use other internals, we should inform the Preact core team first so we can be in contact in case they have changes planned.
  • In any case, if we finally use Preact, we should fix preact version inside @wordpress/element and update it manually.

@mtias
Copy link
Member

mtias commented Aug 31, 2022

I'd rather stay away from <div is="wp-block"> and not encourage its proliferation

@tomalec
Copy link

tomalec commented Nov 5, 2022

I'd rather stay away from <div is="wp-block"> and not encourage its proliferation

Could you explain why?

@tomalec
Copy link

tomalec commented Nov 5, 2022

Sorry if that's a silly idea, but I kind of lack the context and overview of what we are trying to achieve and all the constraints.
(I'd appreciate if somebody would point me to a good read about with that)

  • Can't we make wp-block operate on its siblings rather children? Then it somewhat hacks the problem of "all children rendered"/"endTagParsedCallback" if we place it as the last sibling.
  • Can we make wp-block be a <template> then it obviously does not participate in CSS. As from what I understand what we try to achieve here, and the set of constraints is similar to what template shadowroot does.

@developit
Copy link

developit commented Nov 8, 2022

Regarding versioning - we could consider setting up an integration test in Preact core with a stripped-down version of WP's element base so that any potentially-breaking change will be flagged in CI.

One thing I wanted to suggest (not sure if it has already been suggested here) would be that a Custom Element could be used to initialize the Preact tree for a DOM subtree without necessarily wrapping that tree:

<!-- this div is as-rendered by the preact component: -->
<div class="wp-block-image alignwide size-large">
    <figure>
        <img ...>
    </figure>
    <input class="abc">
</div>
<wp-block name="image" props="{}"></wp-block>
<!-- ^ this CE hydrates its previous sibling(s), then hides/removes itself -->

Rough implementation:

import { h, hydrate } from 'preact';

class WpBlock extends HTMLElement {
  constructor() {
    this.style.display = 'none';
  }
  connectedCallback() {
    let root = this.previousElementSibling; // or createRootFragment w/ scan back to comment
    let block = this.getAttribute('name');
    let props = JSON.parse(this.getAttribute('props') || this.textContent || '{}');
    hydrate(h(components[block], props), root);
  }
}

@tomalec
Copy link

tomalec commented Nov 8, 2022

One thing I wanted to suggest (not sure if it has already been suggested here)

Just in the comment above ;) "Can't we make wp-block operate on its siblings rather children?"

Also, to be precise:

<wp-block name="image" props="{}"></wp-block>
<!-- ^ this CE hides itself, then hydrates its previous sibling(s), then removes itself -->
import { h, hydrate } from 'preact';

class WpBlock extends HTMLElement {
  constructor() {
    super();
    this.style.display = 'none'; // hide myself.
  }
  connectedCallback() {
    let root = this.previousElementSibling; // or createRootFragment w/ scan back to comment
    let block = this.getAttribute('name');
    let props = JSON.parse(this.getAttribute('props') || this.textContent || '{}');
    hydrate(h(components[block], props), root);
	this.remove(); // remove myself
  }
}

@mtias
Copy link
Member

mtias commented Nov 10, 2022

@tomalec sorry for the laconic phrasing. Mainly due to its semantic ambivalence, the contentious nature on Safari/Webkit, and the fact it doesn't seem necessary for this particular scenario.

@tomalec
Copy link

tomalec commented Nov 10, 2022

the contentious nature on Safari/Webkit

That's a definitely fair point if we aim to start polyfill free.

Mainly due to its semantic ambivalence,

"A customized built-in element inherits the semantics of the element that it extends."

I get that <div is="wp-block"> does not drive much semantic meaning, but other elements do.

the fact it doesn't seem necessary for this particular scenario.

I think it actually is useful, as it allows customizing elements with different parsing behavior.
For example <template is="wp-block">.

@luisherranz
Copy link
Member

Thanks for your suggestions, folks 🙂

We are going to start working on this issue. We'll try to do it so we can compare the performance of all the different approaches. I'll report back on what we find out, although if the performance differences are not significant, we'll prioritize the options that don't introduce additional elements in the DOM to keep the HTML as clean as possible (i.e., mutation observer for detection + createRootFragment for hydration).

@luisherranz
Copy link
Member

Closed as we're not actively working on this experiment anymore and this works fine in the Directives Hydration.

@luisherranz luisherranz closed this as not planned Won't fix, can't repro, duplicate, stale Jan 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants