Widgets overwrite zone tag helper.

[sarahelsaig]

Describe the bug

I can't use both <zone> tag helpers and widgets for the same zone. If you have a widget in the zone, the content in zone tag helpers are ignored.

To Reproduce

Steps to reproduce the behavior:

1. Create a new Orchard Core CMS Web App solution.
2. Add a new Orchard Core Theme project
3. Replace the Views/Layout.liquid with Views/Layout.cshtml having the following content to keep it simple:

@inherits OrchardCore.DisplayManagement.Razor.RazorPage<TModel>

@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers
@addTagHelper *, OrchardCore.DisplayManagement
@addTagHelper *, OrchardCore.ResourceManagement

<zone name="Footer">
<p>Hello from zone tag helper!</p>
</zone>

@await RenderSectionAsync("Content", required: false)
@await RenderSectionAsync("Footer", required: false)

3. Launch and set up the webapp.
4. Go to /Admin/Themes, and select the newly created theme.
5. Confirm "Hello from zone tag helper!" is visible on the home page.
6. Create a zone "Footer" and a layer "Always" (boolean true condition).
7. Create a widget content type with Html Body and add a new item with "Hello from Widget!" to the Footer zone.
8. Go back to the home page. You only see Hello from Widget! and any tag helpers are ignored.

Expected behavior

You should see

Hello from zone tag helper!
Hello from Widget!

in the footer.

[sarahelsaig]

Upon further investigation the widget doesn't overwrite the zone tag helpers (the widget filter runs before the tag helpers), but it breaks them. It's because the zone shape (layout.Zones["Footer"]) becomes PositionWrapper type. The ZoneTagHelper only works with Shape and nothing else so it gets skipped.

It seems to me, that the PositionWrapper should be wrapped in a Shape that represents the zone or it should implement the IShape.AddAsync(object, string) method. Currently that throws NotImplementedException. Is that intentional?

[jtkech]

Layout zones are pre-rendered before executing the layout itself, so using the zone tag helper here is too late.

The zone tag helper can be used while rendering any layout zone, for example through a part / field shape while rendering the layout Content zone, through a widget in any zone using the Liquid zone tag targetting another zone, and so on.

The only constraint is that the targeted layout zone should not be pre-rendered before the layout zone in which the zone tag helper is used, for info the first pre-rendered layout zone is the layout Content zone.

[jtkech]

Good to take a look at ThemeLayout.cs in OrchardCore.DisplayManagement

[jtkech]

Zones are pre-rendered in IHtmlContent which are not Shapes and then on which we can't add shapes anymore.

[sarahelsaig]

I don't think it should be too late at this point, this should work up until the RenderSectionAsync call. I have made a copy of the ZoneTagHelper and modified it like below and it that works how I expect. (Relevant change is the if (zone is PositionWrapper positionWrapper... block.)
Even if you think the current behavior is not a bug I think it's very counterintuitive. Do you see any reason against the change below?

using Microsoft.AspNetCore.Razor.TagHelpers;
using Microsoft.Extensions.Logging;
using OrchardCore.DisplayManagement;
using OrchardCore.DisplayManagement.Layout;
using OrchardCore.DisplayManagement.Shapes;
using OrchardCore.DisplayManagement.Zones;
using System;
using System.Threading.Tasks;

namespace Lombiq.HelpfulLibraries.OrchardCore.Shapes;

[HtmlTargetElement("zone2", Attributes = NameAttribute)]
public sealed class Zone2TagHelper : TagHelper
{
    private const string PositionAttribute = "position";
    private const string NameAttribute = "name";

    private readonly ILayoutAccessor _layoutAccessor;
    private readonly Func<ValueTask<IShape>> _zoneFactory;
    private readonly ILogger<Zone2TagHelper> _logger;

    public Zone2TagHelper(
        ILayoutAccessor layoutAccessor,
        IShapeFactory shapeFactory,
        ILogger<Zone2TagHelper> logger)
    {
        _layoutAccessor = layoutAccessor;
        _zoneFactory = () => shapeFactory.CreateAsync("Zone");
        _logger = logger;
    }

    [HtmlAttributeName(PositionAttribute)]
    public string Position { get; set; }

    [HtmlAttributeName(NameAttribute)]
    public string Name { get; set; }

    public override Task ProcessAsync(TagHelperContext context, TagHelperOutput output)
    {
        if (string.IsNullOrEmpty(Name))
        {
            throw new ArgumentException("The name attribute can't be empty");
        }

        return ProcessInnerAsync(output);
    }

    private async Task ProcessInnerAsync(TagHelperOutput output)
    {
        var childContent = await output.GetChildContentAsync();
        var layout = await _layoutAccessor.GetLayoutAsync();

        var zone = layout.Zones[Name];

        if (zone is PositionWrapper positionWrapper && layout is ZoneHolding zoneHolding)
        {
            zone = new ZoneOnDemand(_zoneFactory, zoneHolding, Name);
            await zone.AddAsync(positionWrapper, positionWrapper.Position);
        }

        if (zone is Shape shape)
        {
            await shape.AddAsync(childContent, Position);
        }
        else
        {
            _logger.LogWarning(
                "Unable to add shape to the zone using the <zone2> tag helper because the zone's type is " +
                "\"{ActualType}\" instead of the expected {ExpectedType}",
                zone.GetType().FullName,
                nameof(Shape));
        }

        // Don't render the zone tag or the inner content
        output.SuppressOutput();
    }
}

[jtkech]

Yes, the result of a pre-rendering is a PositionWrapper embedding an IHtmlContent

ThemeLayout.Zones[zone.Key] = new PositionWrapper(await DisplayAsync(shape), "");

Then good idea to wrap it but as I remember we do pre-rendering for perf, hmm but here we re-build a zone on demand, but only on demand ;) So looks like a good solution allowing to add items to a zone later on.

[jtkech]

If you suggest a PR, would need to also update the Liquid ZoneTag accordingly.

[jtkech]

For info zones are pre-rendered since #5436, not for perf but for the following (and not only I think).

Rendering zones before Layout allow zones to register resources to ResourceManager.

So okay, your suggested version allows to add items to an already pre-rendered zone, e.g. from a given layout (we can have different layouts) or from any other zone.

Just one thing to be aware of, the positioning may not be relative to the positions of other zone items if the whole is already pre-rendered, the PositionWrapper having no specific position in itself.

[ns8482e]

Technically you can use<zone> tag helper to any anywhere as long as the given zone is not rendered yet.
That means you can define zone tag helper in Layout before it has been called for rendering.

However, IMHO zone tag helper should not be defined within Layout or in any other zone and should be discouraged.

The reason its prerendered is not just perf but also to collect resource info defined in zones which rendered in head section of layout before actual zone rendering

[sarahelsaig]

I'm fine with discouraging this use case, but currently it's not communicated to the developer at all. So at first it works anyway, and when it breaks silently the developer is left confused. At least a _logger.LogWarning should be there like in the else part in my above snippet.

[ns8482e]

I'm fine with discouraging this use case, but currently it's not communicated to the developer at all. So at first it works anyway, and when it breaks silently the developer is left confused. At least a _logger.LogWarning should be there like in the else part in my above snippet.

Isn't it already throwing an Not implemented exception ?

[sarahelsaig]

It doesn't throw.
If you tried to call await zone.AddAsync(childContent, Position); directly then it would throw, but that never happens. Thanks to the type checking here if the zone isn't a Shape then the tag helper won't do anything (fail silently). I think it should either:

• throw (remove the type checking)
• log a warning like in my example (I'd prefer this as it's helpful in debugging).