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

Regression in <replace> #54

Open
fulv opened this issue Dec 2, 2015 · 9 comments
Open

Regression in <replace> #54

fulv opened this issue Dec 2, 2015 · 9 comments

Comments

@fulv
Copy link
Member

fulv commented Dec 2, 2015

Between Diazo v 1.0.6 and v 1.2.1 (which ship with Plone 4.3.6 and Plone 5.0, respectively), something broke in the way <replace> works.
The simplest test case I can think of to reproduce this issue is this: <replace content="//a/text()" />.

Using diazocompiler I compared the xslt produced by the two versions.

1.0.6

The output of the rule results in all <a> elements being stripped of their content, though the element and its attributes are untouched. For example, <a>home</a> becomes <a></a>.

    <!--RULE: <diazo:replace content="//a/text()" xml:id="r25"/>-->
    <xsl:template match="//a/text()"/>
1.2.1

In Plone 5 all <a> elements have their content duplicated, i.e. <a>home</a> becomes <a>homehome</a>.

<xsl:template match="//a/text()">
    <xsl:apply-templates select="." mode="before-content"/>
    <xsl:apply-templates select="." mode="replace-content"/>
    <xsl:apply-templates select="." mode="after-content"/>
</xsl:template>

<xsl:template match="//a/text()" mode="replace-content"/>

(By the way, note how the comment explaining where these transforms originated from is gone in diazocompiler's output for most rules.)
I'm not sure what the before-content, after-content, replace-content are supposed to do, but it seems they are at the root of the problem.

The reason why I stumbled onto this issue is because I was looking for a way to wrap the content of an element with another element, e.g. <a>home</a> ---> <a><span>home</span></a>. I was not able to do it with plain Diazo rules, but had to revert to straight xsl, which is a shame:

  <xsl:template
    match="//ul[@id='portal-globalnav']/li/a">
    <a>
      <xsl:apply-templates select="./@*"/>
      <span>
        <xsl:value-of select="." />
      </span>
    </a>
  </xsl:template>
@davidjb
Copy link
Contributor

davidjb commented Apr 20, 2016

I'm seeing the same issue with <replace> between the same versions of Diazo (tested with 1.0.6 and the more-recently released 1.2.2). One original rule in 1.0.6 allowed me to replace just an attribute with a given value (or in this case, an empty value):

<replace content="//input[@class='searchButton']/@value">
    <xsl:attribute name="value"></xsl:attribute>
</replace>

which becomes

    <xsl:template match="//input[@class='searchButton']/@value">
                <xsl:attribute xmlns:diazo="http://namespaces.plone.org/diazo" name="value"/>
            </xsl:template>

in XSL and works, clearing the value attribute (but not deleting the attribute itself).

However, in versions of Diazo after 1.1.2 (that feature commit 1f411eb), the result is an error from Diazo (in Plone) when applying the theme:

    runtime error, element 'attribute' [183:0]
    xsl:attribute: Cannot add attributes to an element if children have been already added to the element. [0:0]

because the the resulting XSL looks like what @fulv has shown:

<xsl:template match="//input[@class='searchButton']/@value">
    <xsl:apply-templates select="." mode="before-content"/>
    <xsl:apply-templates select="." mode="replace-content"/>
    <xsl:apply-templates select="." mode="after-content"/>
</xsl:template>

<xsl:template match="//input[@class='searchButton']/@value" mode="replace-content">
                <xsl:attribute xmlns:diazo="http://namespaces.plone.org/diazo" name="value"/>
            </xsl:template>

The reason I was using <replace> was so I could manipulate the content; an <xsl:template> rule doesn't work as I have another <replace> rule for #portal-searchbox (eg so something like this doesn't work).

Suggestions on a replacement syntax?

@lrowe
Copy link
Member

lrowe commented Apr 20, 2016

The changes involved were to support easier modification of content by supporting <before/replace/after content-children=...> similar to <before/replace/after theme-children=...>

For the text case you should probably use:

<replace content-children="//a">
    ...
</replace>

For the attributes case I think this should work:

<drop attribute="value" content="//input[@class='searchButton']" />

(see: http://docs.diazo.org/en/latest/basic.html#drop)

@davidjb
Copy link
Contributor

davidjb commented Apr 21, 2016

@lrowe For the attribute, that works if I want to drop it, but I replace the attribute with a value, in this case, empty, overriding the default Submit text that would appear if the value attribute isn't present. I've solved this specific case with CSS, but I have another system built with lots and lots of rules like this:

<replace content="//*[@class = 'portlet toc']/@class">
  <xsl:attribute name="class"><xsl:value-of select="."/> pull-right well well-large</xsl:attribute>
 </replace>

which is extending the classes of a given element, adding Bootstrap-compatible classes. As above, an <xsl:template> doesn't work because of another <replace> rule in effect for the portlet column. Any other suggestions? Could the <replace> syntax for attributes be made to work with inline content, eg `` <xsl:value-of select="."/> pull-right

In addition to adding to or replacing attributes, there's also rules written for Diazo 1.0.6 which involve <xsl:copy>, such as this https://github.com/collective/diazotheme.frameworks/blob/8b37bdd222d10eea9cadb1f56090ed42ecc6bb9e/diazotheme/frameworks/bootstrap/rules/forms.xml#L62 for shoehorning Plone's markup for form fields into what Boostrap requires (add attributes, wrap/reorganise elements). These rules also fail:

runtime error, element 'copy' [408:0]
Attribute nodes must be added before any child nodes to an element.

Thanks for your help on how make this work with the latest Diazo.

@keul
Copy link
Member

keul commented Aug 25, 2016

Any news or official direction about this? We already found two websites that can't be upgraded from 4.3.7 to 4.3.10 without pinning the old diazo.

@sauzher
Copy link

sauzher commented Aug 21, 2017

after one year since last keul's comment, working on a plone 5.0.6 and latest versions of diazo (1.2.5) and plone.app.theming (2.0.1) the issue, it seems to me, is still there.
Still retro-pinning diazo at 1.0.6.

@frisi
Copy link

frisi commented Jun 25, 2018

same here, still using diazo 1.0.6.

@lrowe maybe you can point me and others to the right direction.

how can the same as the following be achieved with diazo > 1.0.6?

the aim is to copy all body class attributes from content to theme and add additional classes
(to re-use the same index.html and most of the rules for different themes that are slight variations of the base theme (body.brand1, body.brand2)

<copy attributes="class" css:theme="body" css:content="body" />
<replace content="/html/body/@class">
   <xsl:attribute name="class"><xsl:value-of select="."/> brand1</xsl:attribute>
</replace>

@lrowe, @ebrehault is there a way to solve this problem with a current version of diazo?
or did you decide to scarify the support for the usecase above intentionally when adding support for modifying content on the fly using before and after in #43?

maybe you can give me (and other having this issue) any pointers how to solve this and not having to pin diazo to 1.0.6

the official docs did not help mw:
https://plone-theming-with-diazo.readthedocs.io/en/latest/snippets_diazo/recipes/index.html#add-attributes-on-the-fly states that you'll run into the error described in this issue if your node already has child nodes.
and the example here http://docs.diazo.org/en/latest/recipes/adding-an-attribute/index.html adds a new attribute instead of extending the value of an existing one.

@ebrehault
Copy link
Member

Hi @frisi,

This regression was not intentional, and I think the described use case makes sense, but apparently it was not part of the existing unit tests, hence I broke it without being aware.

I do not remember a lot about how it works, but I do remember it is super difficult and pretty painful to debug (xslt is difficult per se, but here we use xslt to generate xslt :) ).

I imagine the fix is about to add a proper xsl:when/xsl:otherwise in lib/diazo/emit-stylesheet.xsl to manage the case where the attribute already exists, but I have idea where it should be done.

Sorry I cannot be more helpful.

@frisi
Copy link

frisi commented Jun 26, 2018

thanks @ebrehault.
@lrowe could you have a look at this or know somebody else who can fix this?

@streaps
Copy link

streaps commented Jul 22, 2020

Is this unfixable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants