Different/wrong html when there is only one image

Thanks for the issue report! I agree. 👍

@Grevil: Let's please fix this and prepare a MR. I think it's important to solve this as early as possible, before many people are using / styling the structures.

Is this something we need to solve in this module or is this from the pswp library? Eventually let @thomas.frobieter also take a look for the right solution.

This is probably linked to the JS setting the fallback wrapper, have a more concise look at the wrapper:

<span class="photoswipe-gallery photoswipe-gallery--fallback-wrapper" data-once="photoswipe">

vs.

<div class="photoswipe-gallery" data-once="photoswipe">

I'll get to it, as soon, as I can!

This isn't easily done. Inside "/src/Plugin/Field/FieldFormatter/PhotoswipeFieldFormatter.php", we have the following code, which causes this issue:

    if (!empty($items) && count($items) > 1) {
      // If there are more than 1 elements, add the gallery wrapper.
      // Otherwise this is done in javascript for more flexibility.
      $elements['#prefix'] = '<div class="photoswipe-gallery">';
      $elements['#suffix'] = '</div>';
    }

Prefixing "elements" unfortunately means prefixing the entire field, for some reason...

Meaning we probably need to move the entire logic inside our js, which ACTUALLY makes way more sense, since if we are using multiple images on a template using our twig method or the global photoswipe option, and no wrapper is manually set, each image will be wrapped independently!

So very nice find, which is quite important actually! :)

Yeah this was a very very large topic in the past... we shouldn't make too quick changes here. Now I also see the key difference. It's the fallback gallery wrapper vs. the intended gallery wrapper.

As this is expected behavior for reasons, I don't rate this as bug anymore, instead we should discuss how to unify the functionality, by for example only setting classes, but not adding wrappers. We need to discuss this personally in the team.

Switching to feature request for further discussions.

Why don't we just add the class to the field attributes then?
If the field wrapper is removed by the theme for whatever reason, the JS fallback kicks in.

It's not that simply. Many edge cases need to be considered, we shouldn't glitch back into the tales we were in before the current solution. No quick shots please, but of course suggestions like this are welcome for discussion.

I tinkered around a bit in the formatter and the reason why the whole field is prefixed, is because inside FormatterBase's "view()" method, the items from "viewElements" are simply merged with the "field" theme defintion:

$elements = $this->viewElements($items, $langcode);

    // If there are actual renderable children, use #theme => field, otherwise,
    // let access cacheability metadata pass through for correct bubbling.
    if (Element::children($elements)) {
      $entity = $items->getEntity();
      $entity_type = $entity->getEntityTypeId();
      $field_name = $this->fieldDefinition->getName();
      $info = [
        '#theme' => 'field',
        '#title' => $this->fieldDefinition->getLabel(),
        '#label_display' => $this->label,
        '#view_mode' => $this->viewMode,
        '#language' => $items->getLangcode(),
        '#field_name' => $field_name,
        '#field_type' => $this->fieldDefinition->getType(),
        '#field_translatable' => $this->fieldDefinition->isTranslatable(),
        '#entity_type' => $entity_type,
        '#bundle' => $entity->bundle(),
        '#object' => $entity,
        '#items' => $items,
        '#formatter' => $this->getPluginId(),
        '#is_multiple' => $this->fieldDefinition->getFieldStorageDefinition()->isMultiple(),
        '#third_party_settings' => $this->getThirdPartySettings(),
      ];

      $elements = array_merge($info, $elements);

Leading to an incorrect hierarchy codewise:

(The array entries 0 and 1 contain our "photoswipe_image_formatter" theme items and are therfore on the same level with the "field" theme)

I guess, this could be seen as a core bug, but on average this isn't a problem in 99% of all cases.

So my solution would be to create a separate 'photoswipe-image-formatter-gallery-wrapper' template, in which we define the gallery wrapper span / div and pass our "photoswipe-image-formatter" items into and display them.

This way we have a formatter specific solution which is really nice and clean and people can also hook into the gallery wrapper template and inject their own HTML if needed!

I created a first MR, to show my approach. It's not perfect yet, currently I can only add the wrapper under the field-item or around the field. Not in between, so this still needs work (and discussion).

All done, please review!

I'll adjust the tests tomorrow.

Thanks @Grevil, Code changes LGTM. For each broken test please manually check the new result is *really* correct. There's still a risk left that we forgot something or special things happen. We should use this chance :)

@thomas.frobieter should afterwards test the dev version, to be sure nothing is broken, before we tag a new release.

@Grevil and @thomas.frobieter I think we should try to get this fixed ASAP as the code changes will be kind of BC. This means we need at least a 5.1.0 version then. Better to fix it earlier than leter.

Might take a bit, since I can not run photoswipe js tests locally.

All adjusted accordingly! Tests with two media/image fields vs. two one field with multiple pictures COULD be refined, but they are fine as is (there is a line checking for two gallery wrappers [2 fields] and for one gallery wrapper [1 field 2 pictures]).

This can be reviewed!

Changes LGTM. BC notes above. Think there's no way around it.

I can confirm that the html structure is better and consistant now with the new version. Appreciate the work guys, thanks a lot ;)

Automatically closed - issue fixed for 2 weeks with no activity.