[2.0.0-rc1] Should StringPropType normalize and strip tag?

Created on 27 November 2024, about 2 months ago

Problem/Motivation

Discussed in 🐛 Side effect in links prop type normalization casting Active with @pdureau, we will not touch StringPropType in the related issue.

But it raised the question if a normalize should be added in it and in this case should it strip tag?

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇫🇷France Grimreaper France 🇫🇷

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Grimreaper
  • 🇫🇷France just_like_good_vibes PARIS

    hello,
    oups i think i already added things in 🐛 LinkPropType normalization...suite Active

  • 🇫🇷France pdureau Paris

    I have mixed feelings (so an open mind) on this subject:

    • in a pure JSON Schema point of view, strings with markup are still strings, so no stripping
    • in a SDC (or any similar UI component technology) point of view, markup (already rendered or not) belongs to slots, and string props may be considered as plain text only
  • 🇫🇷France Grimreaper France 🇫🇷

    Hi,

    I have discussed this issue with @pdureau, if JSON Schema string allows markup, do not strip tags.

    But recommend to use a slot if it should contain markup.

  • 🇫🇷France pdureau Paris

    Let's imagine we are leveraging contentMediaType and create a new PropType plugin to make the distinction between Markup and PlainText:

    • we can create a Markup prop type with type: string; contentMediaType: "text/html" and keep type: string for plain text
    • or we can create a PlainText prop type with type: string; contentMediaType: "text/plain" and keep type: string for markup

    What would be the usage of this Markup prop type? It will be very similar to SlotPropType :

    • accept the same source plugins as SlotPropType: WYSIWYG, Component, Block, Field Formatter...
    • do the same data normalization as SlotPropType, with only one step more: the renderable is renderer

    Would it make sense? I don't think so.

  • 🇫🇷France just_like_good_vibes PARIS

    i find the remark interesting, but still we need to decide what to do :

    - Before 🐛 LinkPropType normalization...suite Active , string prop type was unfiltered. String prop types are supposed to have only string and not tags, but it seems some people were abusing the original intent of prop types and (mistakenly) used them to convey html markup. We were like authorizing this (through twig usage or through weird sources like token ? )

    - After 🐛 LinkPropType normalization...suite Active , which is now : we are stripping tags. Is-it a mistake? should we re-allow tags inside strings? (by the way, tags from forms won't work, like input for example, twig will filter them i guess) ?

    Option 1) : we keep the strip tags. I would say then "work as expected". People are not supposed to free-ride string props to put markup in it. They should re-think their SDC component model and use slots for that. The benefit of that strip tags filtering is the ability to have a predictable behavior of string prop types. from a twig usage perspective, people can inject "stuff" (e.g. html) that may be the result of a rendering, and that stuff is cleaned by our normalization process to become nice string.

    Option 2) : We remove the strip tags. We open the gate to strange things. but some people would be happy to not rewrite their SDC component, and ui patterns will support their strange usage. still this is not a real html support yet, would we allow also form elements ? (in that case a specific treatment in preprocess should be added).

    Option 3) : We keep Option1), but we introduce the new prop type Pierre was talking about. Then, weird usage of string props with tags, would work if and only if SDC component definition would be patched with contentMediaType: "text/html" . That would be a compliance requirement of ui patterns 2. more elegant solution with a nasty new prop type.

    Honestly, i like option 3) but it is still an open door to strange things with props. But strange things are going into a specific prop type at least instead of free-riding the nice String prop type.

    we need to "vote", for the best option.

  • 🇫🇷France pdureau Paris

    I will have a look

  • 🇫🇷France just_like_good_vibes PARIS

    hey hey, what about Option 4) we keep the strip tags behavior in string prop types and we do not introduce a new prop type to inject HTML into props. BUT, we implement this : if the JSON schema definition has contentMediaType: "text/html" , then we allow HTML to be placed into the string prop, and the normalization and preprocess on our side behaves in a different manner.

    now i vote for option 4

  • 🇫🇷France pdureau Paris

    option 4 looks great: no new prop types, no confusion with slots, just an adaptation related to the prop schema.

    I keep the issue on my side to check if no contentMediaType is stricter than contentMediaType: "text/html" in JSON schema.

  • 🇫🇷France pdureau Paris

    I am now hesitating between option 4

    A single StringPropType with type: string JSON Schema, if the JSON schema definition has contentMediaType: "text/html" , then we allow HTML to be placed into the string prop.

    And option 5:

    A single StringPropType with type: string JSON Schema, if the JSON schema definition has contentMediaType: "text/plain" , then we strip the HTML.

    Anyway, with both options, we are going in the right direction.

    What do you think?

  • Pipeline finished with Failed
    about 1 month ago
    Total: 545s
    #375004
  • Pipeline finished with Success
    about 1 month ago
    Total: 633s
    #375122
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 459s
    #375151
  • Pipeline finished with Success
    about 1 month ago
    Total: 557s
    #375155
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024