Neos-ui: Validation is no longer enforced for inline editable properties

[bwaidelich]

Description

Since #2538 validation could be activated for inline editable properties.
Due to some regression this no longer works with Neos 5.0+

Steps to Reproduce

Adjust the headline node type definition in the demo site:

'Neos.Demo:Content.Headline':
  properties:
    'title':
      # ...
      validation:
        'Neos.Neos/Validation/StringLengthValidator':
          minimum: 1
          maximum: 5

Open up the homepage (or any other page that contains a headline node) and enter a headline that exceeds 5 characters

Expected behavior

The inline editable should get a red border & display the validation error (see #2538).
It should be impossible to persist / publish the invalid state.

Actual behavior

The validation is completely ignored.

Affected Versions

Neos: 5.0+
UI: 5.1+ (probably also 5.0)

[markusguenther]

The neos.ui has a automated test for that and has the nodetype headline.

'Neos.TestNodeTypes:Content.Headline':
  superTypes:
    'Neos.Neos:Content': true
  ui:
    label: Headline_Test
    icon: icon-header
    position: 200
    inlineEditable: true
    help:
      message: 'Markdown **test**.'
  properties:
    title:
      type: string
      defaultValue: '<h1>Enter headline here</h1>'
      ui:
        inlineEditable: true
        inline:
          editorOptions:
            placeholder: '<h2>Enter headline here...</h2>'
            formatting:
              h1: true
              h2: true
              h3: true
              h4: true
              h5: true
              a: true
      validation:
        'Neos.Neos/Validation/NotEmptyValidator': []
        'Neos.Neos/Validation/StringLengthValidator':
          minimum: 1
          maximum: 255

This leads to the following result. So this test never fail because of the additional notEmpty validator.

[markusguenther]

The issue was bit tricky and the error can have multiple reasons.

1. The validators are stored for each node so when the Validators has been changed the existing nodes still use the old config.
2. When you have a validator with just stringLength and expect a minimum of 1 it does not work and you need additionally the NotEmpty validator.
3. The StringLength validator with a pretty short maximum will often fail because the inline elements have values with markup. So the HTML tags blow that up.

The PR now strips the tags off and that resolves the maximum validation, but for the minimum of 1 we need a NotEmpty validator as well.

[dimaip]

Thanks so much for digging into this Markus!

[bwaidelich]

Yes, thanks a ton, Marks. But I'm afraid this ain't solve it yet

The validators are stored for each node so when the Validators has been changed the existing nodes still use the old config.

What does that mean? The validation rules are persisted with the node? So I can't change the validators afterwards?

When you have a validator with just stringLength and expect a minimum of 1 it does not work and you need additionally the NotEmpty validator.

I know, that's expected and it works like this in Flow & the Form Framework, too

The StringLength validator with a pretty short maximum will often fail because the inline elements have values with markup. So the HTML tags blow that up.

Right. I had addressed that with https://github.com/neos/flow-development-collection/pull/1849 (see #2579) but that is not related to this bug really (I think at least)

[bwaidelich]

...another heisenbug. I can't reproduce it any longer. At least not with the Neos.Demo site o.O

[markusguenther]

:)

I could reproduce the HTML thing. But I maybe had a caching issue with the point 1. Checked the nodedata and the local-storage and redux store and did not find the old config.
Added a small video with the neos demo. Yesterday I worked with the testing distribution to be able to run the e2e test locally.

[markusguenther]

Arg 🤦‍♂️ while checking that the validators are implemented in the ui itself and not using the PHP validator code I saw that the StringLength validator has an option ignoreHtml.

So we don't need the strip html stuff ... you just need to add this config.
Sorrry did not see that before.

[bwaidelich]

I'm really sorry for all the fuss.. It turns out, this was just a bug in a customer project of mine that messed with z-index so that the inlineValidationTooltips were not visible.

I already expected that and reproduced the bug with a clean Neos.Demo installation but apparently I ran into the caching issue @markusguenther mentioned above, so that my new validation rules were not active immediately..

Closing this one.. Thanks for your help, Markus, I owe you some beers <3

[suffle]

There is a subtle change in Neos 5 which changes the behaviour of validation messages though: The <div id="neos-new-backend-container"></div> is rendered as the first element of the body in Neos ~5.0 while it was rendered as the last element in Neos ~4.0 which changes the layering of the elements and makes the described behaviour reproducible in the Neos.Demo Package. When you add a Validator to Neos.Demo:Content.Headline and edit the Headline on the KeyVisual you have the exact same problem and I guess there are more projects with the same issue:

Neos 4:

Neos 5:

[bwaidelich]

Turns out this was a regression after all:
This line is the culprit: https://github.com/neos/neos-ui/blob/ba3831edbdacf666cc1395be04d6c579a4e84d5c/Resources/Private/Fusion/Prototypes/Page.fusion#L86

doesn't work since there is no neosBackendContainer in the original Neos.Neos:Page fusion prototype since https://github.com/neos/neos-development-collection/pull/2672

Maybe we should take the chance and get rid of the "new" and "old" separation at this point. But that's probably something for master while the minimal invasive bugfix would be to change after neosBackendContainer to before closingBodyTag probably. What do you think @markusguenther @dimaip ?

[markusguenther]

The position has been adjusted an will be released for the next ui version for 5.0 and newer.
As the renaming the container is a bit breaking if someone rely on that I will change that only on master for 6.0.

Thanks for the help guys ❤️