WordPress / block-interactivity-experiments

โš ๏ธ WARNING: This repository is no longer active. The development and discussions have been moved to the Gutenberg repository.
https://github.com/WordPress/gutenberg/discussions/categories/interactivity-api
109 stars 11 forks source link

What should be the HTML structure returned when `wp-show` is false? #159

Closed SantosGuillamot closed 1 year ago

SantosGuillamot commented 1 year ago

While working on creating the wp-show directive attribute, I wasn't sure the HTML that should be returned when wp-show evaluates as false. I'm opening this issue to discuss the implications of the different alternatives and decide on an approach.

Given the following HTML:

<div data-wp-show="false" class="my-class">
  <p>Children</p>
</div>

I considered two alternatives:

Option 1 - Wrapped everything inside the <template> tag

<template>
  <div data-wp-show="false" class="my-class">
    <p>Children</p>
  </div>
</template>

Option 2 - Wrapped children inside the <template> tag

<div data-wp-show="false" class="my-class">
  <template>
    <p>Children</p>
  </template>
</div>

As I mentioned, I don't know the pros and cons of each alternative, so I'd love to hear your thoughts. Whatever we decide, should probably be applied to similar directives like wp-each to keep consistency.

Alpinejs is using the second approach. For directives like x-if or x-for, they used them directly in the <template> tags, and it requires that <template> MUST contain only one root element.

On the other hand, if not explained properly, I feel that Option 2 could cause unexpected layout issues.

dmsnell commented 1 year ago

wrapping in a template pulls the contents and the directive out of the DOM and out of the page. this effectively hides it in a pocket dimension, so calls to querySelectorAll('div') will only find the one living outside of the <template>

that also means we can't apply styling to them, not that we would likely want to, but for example, some debug border to show the location of the directives wouldn't be possible.

since I'm less aware, possibly forgetful, is the requirement to wrap in a <template> manadatory, vs. hiding with CSS?

Option 2 could cause unexpected layout issues.

the main thing is probably that Option 1 simply doesn't participate in layout, as it's not in the page, whereas Option 2 lives in the DOM as an empty <div>

Screenshot 2023-02-22 at 1 24 26 PM
luisherranz commented 1 year ago

Imagine these blocks. Each inner block can be hidden or shown when clicked, and they can start hidden or shown depending on an attribute.

<!-- wp:superlist/list -->
<ul class="wp-block-superlist-list">
  <!-- wp:superlist/list-item -->
  <li class="wp-block-superlist-list-item">Item 1</li>
  <!-- /wp:superlist/list-item -->

  <!-- wp:superlist/list-item -->
  <li class="wp-block-superlist-list-item">Item 2</li>
  <!-- /wp:superlist/list-item -->

  <!-- wp:superlist/list-item -->
  <li class="wp-block-superlist-list-item">Item 3</li>
  <!-- /wp:superlist/list-item -->
</ul>
<!-- /wp:superlist/list -->
<?php
// render.php - superlist/list-item
?>

<li
  <?php echo get_block_wrapper_attributes(); ?>
  data-wp-context='{ "superlist": { "isShown": <?php echo $attributes[ "isShown" ]; ?> } }'
  data-wp-show="context.superlist.isShown"
  data-wp-on:click="actions.superlist.toggleItem"
>
  <?php echo $attributes[ 'text' ]; ?>
</li>

If we use Option 2 and the second element is hidden, the resulting HTML would be (I'm omitting the directives for simplicity):

<!-- wp:superlist/list -->
<ul class="wp-block-superlist-list">
  <!-- wp:superlist/list-item -->
  <li class="wp-block-superlist-list-item">Item 1</li>
  <!-- /wp:superlist/list-item -->

  <!-- wp:superlist/list-item -->
  <li class="wp-block-superlist-list-item">
    <template>Item 2</template>
  </li>
  <!-- /wp:superlist/list-item -->

  <!-- wp:superlist/list-item -->
  <li class="wp-block-superlist-list-item">Item 3</li>
  <!-- /wp:superlist/list-item -->
</ul>
<!-- /wp:superlist/list -->

That is not correct because it's rendered like this:

Screenshot 2023-03-02 at 19 26 38

This specific case seems to be resolved by adding style="display: contents" to the <li> node when it's hidden, but that overcomplicates the developer experience because they would need to add:

<?php
// render.php - superlist/list-item
wp_store( array( 
  'selectors' => array(
    'superlist' => array(
      'listItemDisplay' => $attributes[ 'isShow' ] ? 'list-item' : 'contents'
    ),
  ),
) );
?>

<li
  ...
  data-wp-style:display="selectors.superlist.listItemDisplay"
>
  ...
</li>
// view.js
store({
  selectors: {
    superlist: {
      listItemDisplay: ({ context }) =>
        context.superlist.isShown ? "list-item" : "contents",
    },
  },
});

There are also some accessibility problems related to display: contents.

So:

luisherranz commented 1 year ago

is the requirement to wrap in a template manadatory, vs. hiding with CSS?

I believe it's the correct one because the browser doesn't load the assets.

For example, a 100-image slideshow using this for each image:

<div data-wp-show="false" style="display: none;">
  <img src="...">
</div>

would download 99 unnecessary images on page load.

But as everything, it's open for discussion, of course ๐Ÿ™‚

dmsnell commented 1 year ago

the browser doesn't load the assets.

interesting. have we confirmed this with any tests? seems like browsers today might optimize that away. if not, I can run a test and check

luisherranz commented 1 year ago

have we confirmed this with any tests? seems like browsers today might optimize that away.

I did a quick test in Stackblitz and it seems like Chrome is indeed honoring that behavior. Although I wouldn't mind if it wouldn't in certain circumstances, like a desktop computer with a fiber connection (similar to what it does with loading="lazy").

https://user-images.githubusercontent.com/3305402/222701653-0831055a-3c40-439f-b276-5631065b5986.mp4

adamziel commented 1 year ago

Option 2 combined with tables would result in

  <tr>
    <template><td>Col 1</td><td>Col 2</td></template>
  </tr>

An extra table row also sounds problematic for all sorts of reasons: borders, paddings, rowspans and colspans.

Perhaps if the outer tag was rendered with an inline style="display: none"?

SantosGuillamot commented 1 year ago

Maybe there's a third option I didn't share in the opening post: Restrict the wp-show directive to be used inside <template>. So users would be required to write something like this:

<template data-wp-show="false">
  <div class="my-class">
    <p>Children</p>
  </div>
</template>

I'm not convinced of this approach, I don't see clear benefits of this one over option 1, but sharing it here in case it makes sense somehow. I'm still not familiar with how this would work internally, especially in the SSR.

luisherranz commented 1 year ago

I'm not convinced of this approach

Yeah, my neither... but it's the best option if we finally can't use set_outer_html. Thanks for sharing.

Perhaps if the outer tag was rendered with an inline style="display: none"?

The problem is that if we use style="display: none;" in the outer tag and that tag is an image/iframe/script, it will still download the asset.

Alpinejs is using the second approach. For directives like x-if or x-for, they used them directly in the <template> tags, and it requires that <template> MUST contain only one root element.

I think it was me who told you this, but I was wrong. Alpine is using style="display: none" for x-show and the template approach (option 3) for x-if. I don't like either.


I think the best option in terms of developer experience is 1.

Actually, I would even use a slightly simplified variation of 1. When wp-show is false:

In the client, having the DOM elements wrapped by <template> is not useful. I mean, we already have the static virtual nodes that represent that HTML in memory, we don't need them in the DOM anymore.

Stackblitz example

SantosGuillamot commented 1 year ago

I think it was me who told you this, but I was wrong. Alpine is using style="display: none" for x-show and the template approach (option 3) for x-if. I don't like either.

Okay, you're right! Got it ๐Ÿ™‚

I think the best option in terms of developer experience is 1.

I agree with this. I guess I would just go with option 3 if we can't use set_outer_html then.

Actually, I would even use a slightly simplified variation of 1.

I like it ๐Ÿ™‚ I have one question, though: Would this mean that, if you inspect the HTML in the browser, you won't be able to see the hidden elements? Could this cause some issues with debugging?

luisherranz commented 1 year ago

Could this cause some issues with debugging?

Well, wrapping them in the <template> is incidental to the fact that we are using HTML as our templating language. This React component won't put anything in the DOM either and I have never seen people complaining about it:

const Comp = ({ children }) => {
  const [show, setShow] = useState(false);
  return show ? children : null;
}
SantosGuillamot commented 1 year ago

That makes sense ๐Ÿ™‚ Thanks for clarifying!

westonruter commented 1 year ago

For an alternative to wrapping hidden elements in <template> there is also the hidden attribute. While currently hidden is just a shortcut to doing display:none ([hidden]{display:none}), it is becoming more powerful. In particular, instead of being just a boolean attribute, it now can take a until-found value. This allows for the hidden element to automatically be expanded when doing find-in-browser or when the URL fragment is linking to an anchor inside the hidden element. This is an extremely useful feature. In relation to wp-show, there is a beforematch event that fires when the element is found, which could be used to ensure the WP state is correctly updated to match.

It is currently supported by Chrome and Edge. It is spec'ed in the HTML5 living standard.

Therefore, if the primary concern is about the browser downloading images that are in hidden containers, I suggest the solution be to ensure that loading=lazy is added to such images and just use the hidden attribute, rather than to wrap the entire hidden tree in <template>.

luisherranz commented 1 year ago

if the primary concern is about the browser downloading images that are in hidden containers, I suggest the solution be to ensure that loading=lazy is added to such images and just use the hidden attribute

Oh, I didn't know loading=lazy would help in that case. WordPress now ships all images with loading=lazy by default, so that's good news. It seems to work great: Stackblitz.

It doesn't cover scripts, does it? For example, in this case, the Twitter JavaScript will be loaded before the tweet is shown on the screen:

<blockquote class="twitter-tweet">Tweet content...</blockquote>
<script async src="https://platform.twitter.com/widgets.js"></script>

I guess sometimes that could be a problem, although sometimes that could be the desired behavior.

So maybe the best option is to do the same as Alpine and ship two ways to do conditionals, one for hidden and one for template. And let developers decide when they need to use one or the other.

From what I read (and unlike display: none), elements with the hidden attribute are removed from the accessibility tree and screen readers will ignore the element, which is what we want. So it seems like we could promote the usage of the hidden attribute as the default way to do conditionals.

Oh, and we don't need a special directive for hidden:

<div data-wp-bind.hidden="context.someValue" class="my-class">
  <p>Children</p>
</div>

@alexstine, @joedolson: what do you think?

alexstine commented 1 year ago

@luisherranz Seems to work fine. Checked the example here to verify.

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden

Thanks.

westonruter commented 1 year ago

So if both methods are supported, and if data-wp-show is unnecessary since it can be supported with data-wp-bind.hidden, then would the current data-wp-show be replaced with data-wp-if to be aligned with Alpine's x-if?

Speaking of a11y, note also @mrwweb's excellent point about how the accessibility in the data-wp-show demo in the blog post can be improved with data-wp-bind.ariaExpanded and aria-controls.

felixarntz commented 1 year ago

+1 for relying on the hidden attribute to show/hide elements conditionally.

That said, seeing the example of data-wp-bind.hidden and data-wp-bind.ariaExpanded which typically will need to be used together, I wonder whether a custom directive would still work best. Accessibility best practices like taking care of the latter are often overlooked by developers, and if we can provide a custom directive that automatically handles it (potentially through an extra requirement), I think that would be a great win.

Or maybe a custom directive is not the right way here, but we may be able to automate the aria-expanded attribute usage through another means, e.g. if we find an aria-controls attribute on a button, we can automatically inspect the target element and set the button's aria-expanded attribute correctly. Just thinking out loud :)

mrwweb commented 1 year ago

if we find an aria-controls attribute on a button, we can automatically inspect the target element and set the button's aria-expanded attribute correctly.

I've taken that approach in scripts I write and generally like it. One advantage is that it fails if your IDs aren't unique. Generally speaking, anything like that to enforce accessible (otherwise it's broken) seems like a good idea.

+1 for relying on the hidden attribute to show/hide elements conditionally.

I suspect this won't work out very well. From Monica Dinculescu (h/t CSS-Tricks):

the hidden rule is a User Agent style, which means it's less specific than a moderate sneeze [ref]. This means that your favourite display style will override it:

 <style>
    div { display: block; }
  </style>
  <div hidden>
      lol guess who's not hidden anymore
      hint: it's this thing
  </div>
luisherranz commented 1 year ago

we may be able to automate the aria-expanded attribute usage through another means, e.g. if we find an aria-controls attribute on a button

Sure. If there is a deductible and unambiguous behavior, we can automate it with directives.

+1 for relying on the hidden attribute to show/hide elements conditionally. I suspect this won't work out very well.

That's a shame. Would something like [hidden] { display: none !important; } solve the issue, or will it cause other issues?

alexstine commented 1 year ago

The CSS rule display: none; will work for sure.