surveyjs / survey-library

Free JavaScript form builder library with integration for React, Angular, Vue, jQuery, and Knockout.
https://surveyjs.io/form-library
MIT License
4.11k stars 801 forks source link

Don't render strings in `h5` or `span` tags. #6841

Open SamMousa opened 1 year ago

SamMousa commented 1 year ago

Are you requesting a feature, reporting a bug or asking a question?

Feature

What is the current behavior?

Many strings are rendered somewhere in the DOM below an h1-h6 node and inside a span tag.

What is the expected behavior?

While adding support to render HTML is easy, this will in many cases result in bad markup. For example if a question description contains a list the markup is technically invalid.

I propose using an element that can contain elements from the flow content category for rendering texts.

For example you could use <section> or <fieldset>, both allow flow content inside.

Note that currently in most browser it will work because browsers are very lenient and try their best to make sense of badly structure HTML. Nonetheless this change should improve the document structure and make it more easy to parse for screenreading software.

JaneSjs commented 1 year ago

Hi @SamMousa, Thank you for your inputs. In SurveyJS forms, we use different elements to render texts. The flow content category of HTML elements list many elements which are used inside SurveyJS. For instance, heading HTML elements (h1...h6), span, div, fieldset (which is used by Radiogroup/Checkboxes).

To help us introduce changes that would enhance our library, would you please share a particular example of a badly-structured/invalid HTML form produced by a SurveyJS Form Library?

I appreciate your cooperation. Look forward to hearing from you.

SamMousa commented 1 year ago

Imagine you have HTML rendering enabled, so my onMarkdown just puts the raw text in the HTML field. This is my JSON:

{
 "logoPosition": "right",
 "pages": [
  {
   "name": "page1",
   "elements": [
    {
     "type": "radiogroup",
     "name": "question1",
     "title": "<p>Dit is&nbsp;<strong>HTML tekst</strong></p>\n<p>&nbsp;</p>\n<ol>\n<li><strong>A</strong></li>\n<li><strong>B</strong></li>\n<li><strong>C</strong></li>\n</ol>",
     "choices": [
      "Item 1",
      "Item 2",
      "Item 3"
     ]
    }
   ]
  }
 ],
 "name": "test"
}

The rendered HTML looks like this. This is invalid becausee a span cannot contain a p. While in this example the p could be ommitted, the ol has a clear purpose.

<span class="sv-string-viewer" data-bind="html: locString.koRenderedHtml">
    <p>Dit is&nbsp;
        <strong>HTML tekst</strong>
    </p>
    <p>&nbsp;</p>
    <ol>
        <li>
            <strong>A</strong>
        </li>
        <li>
            <strong>B</strong>
        </li>
        <li>
            <strong>C</strong>
        </li>
    </ol>
</span>

This is invalid HTML.

A simple solution (for knockout) would be to render the string-viewer class as a div instead.

JaneSjs commented 11 months ago

Thank you for the clarification, @SamMousa. I'll discuss this task with our developers and update you shortly.

JaneSjs commented 11 months ago

Hello @SamMousa, Thank you for your patience. I discussed this usage scenario with our developers. We may not consider changing the HTML markup of question titles at the moment. Question titles/descriptions are used to display captions. They are not generally designed to embed complex block HTML elements such as <p> elements.

If you wish to display complex HTML markup as a descriptive text, consider using a separate HTML survey element rather than embedding HTML within question titles/descriptions.

Thanks

SamMousa commented 11 months ago

Hi @JaneSjs thanks for your reply. I'm aware that I can use the HTML survey element as a workaround, but that's suboptimal.

You mention titles are used as captions, which seems reasonable. But descriptions by definition are not captions... The reason I created this issue is not because something doesn't work, it already fully works as desired. The issue is that the markup becomes technically invalid, so it works but it is incorrect if you look at the specs.

Changing the tag that's used is trivial and requires only a few lines changed. This could even be a configuration so as not to break backwards compatibility.

For example, this is the string viewer for knockout:

<!-- ko ifnot: locString.koHasHtml -->
<span class="sv-string-viewer" data-bind="text: locString.koRenderedHtml"></span>
<!-- /ko -->
<!-- ko if: locString.koHasHtml -->
<span class="sv-string-viewer" data-bind="html: locString.koRenderedHtml"></span>
<!-- /ko -->

This is the one for react:

  protected renderString(): JSX.Element {
    if (this.locStr.hasHtml) {
      let htmlValue = { __html: this.locStr.renderedHtml };
      return <span ref={this.rootRef} className="sv-string-viewer" style={this.style} dangerouslySetInnerHTML={htmlValue} />;
    }
    return <span ref={this.rootRef} className="sv-string-viewer" style={this.style}>{this.locStr.renderedHtml}</span>;
  }

As you can see these are all very simple and very few lines and making the tag configurable should be trivial and open up this functionality.