Views content rewrite text says "You may include HTML" but actually allows only a subset

As far as I can tell, this text is filtered using \Drupal\Component\Utility\Xss::filterAdmin(), which only allows these tags:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

Attached a patch to get the ball rolling, however I'm not sure linking to that page would be a good idea, so I have not included the link in the patch.

Also if you're interested in this subject, see #2776667: Review/update $adminTags variable for new html elements to be allowed

@Manuel Garcia++ for not adding a link to \Drupal\Component\Utility\Xss::filterAdmin()

My suggestion for adding a link about the sub set would be either:
- add it to the views help page inside the module and link to that, not crazy about that because it would depend on the help module being enabled and I think the target audience for the help module and this comment about the sub set of allowed HTML don't overlap much.
- edit the views documentation on this functionality https://www.drupal.org/node/1918474 to include the information and link to that . Well we should update the information on that page regardless of whether or not we link to it here.

I'd be for option 2, but any other ideas?

Setting to needs work for adding the link. Without a link to the sub set of HTML this change doesn't really help much.

Going to attempt to address the feedback in #7.

@Lendude my concern with adding the full list of permitted tags to the the documentation is that if the list of elements changed in the code then the documentation would be immediately out of date.

I have amended the documentation to indicate that a subset of HTML is permitted and have linked this text to: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

Does this go far enough?

@nlisgo I agree, adding a static list of tags would not be great in the docs pages, but casual users looking at the docs for Xss::$adminTags would probably not leave any wiser. Not into the do's and don'ts of docs to know what the right thing to do there is.

Another option would be to add a list of allowed tags to the #description, we could just use Xss::getAdminTagList() to generate the list, that would be nice and dynamic, but would also make the description very long. So again, not ideal.

I have a very similar problem... trying to include a iframe to insert a youtube video in "Override the output of this field with custom text"

You can find my question here...
http://drupal.stackexchange.com/questions/217351/iframe-in-rewrite-resul...

The page at https://www.drupal.org/node/1918474 currently strikes a good balance, linking to the list of permitted tags.

Indicating, as the patch does, that only a subset of html is permitted is a good improvement. It helps point users in the right direction.

So how about adding a second help fieldset containing the allowed tags. Bit too much?

It looks like this:

That would be a very nice enhancement.

It would be better imho yeah... however we're not doing this anywhere else in core, so I'm not sure this will get in...

I tried the patch on 8.3x and it applies fine.

Once the allowed tags are expanded the log list (which I am trying to make considerably longer in another issue) is a very bad use of space.

It would be more appropriate for the list to be comma separated.

Comma separated item lists do exist in classy at https://api.drupal.org/api/drupal/core%21themes%21classy%21css%21compone... but here we would need to add something like the following:

.item-list--comma-list li {
    display: inline;
}

.item-list--comma-list li:after {
    content: ", ";
}

.item-list--comma-list li:last-child:after {
    content: "";
}

That, combined with replacing '#theme' => 'item_list' with the '#theme' => 'item-list--comma-list' creates the following:

I am not sure how to check for current usage of the existing CSS which I am suggesting adding to.

however we're not doing this anywhere else in core

It would be more appropriate for the list to be comma separated

Both good points. To handle both would could just list all the tags, I was sorta against that in #11, but it might be the best way to go here. I modeled this after the HTML tag listing on the filter descriptions for formatted text areas.

Formatted text area:

Views rewrite text area after patch:

No interdiff because its a different approach

Updated issue summary (wasn't immediately obvious where this was...)

#18 is good.

As we have multiple solutions and it is a very simple bit of guidance we are trying to add. Maybe we should get some guidance from the committers about the direction they would like to go.

Brilliant, tagging as RTBC to get some feedback in case this isn't the right approach, but #18 looks great to me!

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -716,7 +716,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+        '#description' => $this->t('The text to display for this field. You may include a subset of HTML or <a href=":url">Twig</a>. You may enter data from this view as per the "Replacement patterns" below. Allowed HTML tags: :tags', array(':url' => CoreUrl::fromUri('http://twig.sensiolabs.org/documentation')->toString(), ':tags' => '<' . implode('> <', Xss::getAdminTagList()) . '>')),

pretty sure : is the wrong thing to be using here. :tags<code> should be <code>@tags.

Addressing #23 - good catch @alexpott

MH, can we show some limited list + a link or so for documentation what else might be allowed?

can we show some limited list + a link

The problem with a limited list would be that it wouldn't be dynamic, we have Xss::getAdminTagList() available for an up-to-date list, but no way to limit that to more common tags.

So we either have an up-to-date list that is inconveniently long, or a limited list that needs to be manually updated. Both not great options. My attempt in #14 to put it in a collapsed fieldset feels like overkill too.

As to a link, the documentation for Xss::getAdminTagList() isn't very readable for people not comfortable with reading API docs (the type of people that might benefit from this list of tags), so a link .... meh.

None of these option feel perfect. But all feel better then what we currently have.

So my +1 of the full tag list would be that at the very least it's dynamic, up-to-date and comprehensive.

Yeah what @Lendude said... they only way around this would be to use some kind of read more functionality so that the initial list is not so long, but I don't think we are doing that anywhere else in core.

Another possibility would be to create a documentation page for this purpose but then that doc page would have to be kept up to date manually, and that's bound to be inaccurate at times.

Agreed, imho it'd be more of an issue if you were adding a big chunk of text to, say, node editing or somewhere where it was always visible by default, but as it is you have to expand the fieldset to see it. Also the user is already in a modal window, so unless a link to an external page opens in a new browser tab, it's presumably going to make getting back to where they were afterwards and preserving any changes tricky.

I'm going to RTBC this…

- able to reproduce original problem
- fixed by patch
- added an 8.4.x test to testbot
- added 'String change in 8.4.0' tag (translatable text change)
- in scope and won't be any regressions
- doesn't require tests
- self explanatory without code comments
- only doubt I had was whether array at end of that already massively long line should be wrapped for readability, e.g.

          array(
            ':url' => CoreUrl::fromUri('http://twig.sensiolabs.org/documentation')
                             ->toString(),
            '@tags' => '<' . implode('> <', Xss::getAdminTagList()) . '>'
          )),

…but there's plenty of other places in the file that aren't, so...

On the UI, this looks perfect to me and will make life easier people building views. It does beg the question about what twig things are not allowed in this field though...

I just had a chat with xjm on this and we agreed on the following. First, the tags should be concatenated in a render array. Second, the tag list should be wrapped in <code> tags.

Thank you everyone.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -716,7 +716,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
-        '#description' => $this->t('The text to display for this field. You may include HTML or <a href=":url">Twig</a>. You may enter data from this view as per the "Replacement patterns" below.', array(':url' => CoreUrl::fromUri('http://twig.sensiolabs.org/documentation')->toString())),
+        '#description' => $this->t('The text to display for this field. You may include a subset of HTML or <a href=":url">Twig</a>. You may enter data from this view as per the "Replacement patterns" below. Allowed HTML tags: @tags', array(':url' => CoreUrl::fromUri('http://twig.sensiolabs.org/documentation')->toString(), '@tags' => '<' . implode('> <', Xss::getAdminTagList()) . '>')),

I think we could also improve the text slightly to say something like You may include <a href=":url">Twig</a> or the following allowed HTML tags: @tags (and then a render array for @tags if possible, as @cilefen mentioned). We want to have the translation context but also @tags as a markup object handled by the render system rather than implosion, if possible. Let's try a patch for that and then we can see if it works out.

On the <code>/code> tags, @cilefen and I weren't 100% sure because <code>node/add does not appear to have them, which would seem less semantic. So that merits further discussion.

Thanks everyone for patching and reviewing this Views usability issue.

Attached patch wraps the allowed html tags with the code tag, and changes the text string itself to what was sugesed on #32.

First, the tags should be concatenated in a render array.

As far as I can tell, FormattableMarkup::placeholderFormat expects a string or an object that implements \Drupal\Component\Render\MarkupInterface. Could you please elaborate / give an example on how to provide a render array to a placeholder?

@alexpott pointed out that this exists: \Drupal\Core\Field\FieldFilteredMarkup::displayAllowedTags(), which sets a precedent for "it's ok to implode these", and to me at least, suggests we could add the same to Xss. Also, taking a closer look at things like /filter/tips, it seems the things wrapped in code tags are intended to be working code. Put together, we could perhaps go back to #24 + improved warning, and I would entertain an addition to Xss.

@cilefen I’d be careful of the addition to the Xss class. We already have getAdminTagList and getHtmlTagList there and the whole class is about security. That class needs very careful consideration when we change it and so I'd do that in its own issue. Given that here is the only use-case in core I'd wait for the second one.

Furthermore if we are changing this string we should also fix Twig documentation URL. One it is a redirect and two we've changed our standards and now we inline the url in the translated text so if documentation is translated in another language they can use the specific language link.

Oops yeah, I did not manage to find \Drupal\Core\Field\FieldFilteredMarkup::displayAllowedTags(). I had been looking at Drupal\filter\Plugin\Filter\FilterHtml:

        $rows[] = array(
          array('data' => $tips[$tag][0], 'class' => array('description')),
          // The markup must be escaped because this is the example code for the                                                                               
          // user.                                                              
          array('data' =>
            array(
              '#prefix' => '< code>',
              '#plain_text' => $tips[$tag][1],
              '#suffix' => '</ code>'
            ),

That looked exactly like what I would have expected here for our best practices. But looks like that is not the right precedent. Sorry for the misdirection!

Also sorry about the conflation of render arrays and markup objects for the t() placeholders; apparently my brain had edited reality about what point we got to with that API before the release. Thanks @Manuel Garcia and @alexpott for catching these things. I agree with not expanding the scope to include any of them. We can discuss in broader-scoped followups if needed.

Finally, if we keep the implode, let's also add an inline comment about why we are doing the implode, why it is safe (plain-text escaped by @ from a known good list also), and maybe an @see to displayAllowedTags() for now. Really the main concern that made me hesitate on the original patch is the same thing I always worry about when we do creative things with translatable strings, which is the risk of someone copying a not-best-practice pattern and then changing it in a way that will open up XSS. An inline comment will mitigate that risk without expanding the scope here.

I think the improved text is easier to understand. Thanks @Manuel Garcia. Also good point about fixing the Twig link; let's mark NW for that.

Oh, CoreUrl::fromUri('http://twig.sensiolabs.org/documentation')->toString() is not necessary. Furthermore, to clarify @alexpott's point about link translation. A French developer might want to translate the text to link directly to the French Twig docs. So we can do:

$this->t('You may include <a href="http://current_url_here/">Twig</a> or the following HTML: @tags', ...);

Alright, thanks all for the kind explanations. Attached patch includes the link as part of the translatable string, and an inline comment explaining the implode. Please review the comment itself, not 100% convinced by it.

Here's how it'd look like:

The Twig link is still the one that has the redirect, which @alexpott suggested changing.

On the other hand, though, what it redirects to is http://twig.sensiolabs.org/doc/2.x/ -- which looks like it is specifically about Twig 2.x, when we support 1.x, and also presumably would no longer be the correct link for Twig 3.x when that is released some day. So maybe keeping the current link is better.

Get Twig version from constant and use to generate docs URL.
(This may be a terrible idea.)

EDIT: just realised the floor() is superfluous, will remove once Testbot is fixed.

Self code review.

I'm glad this is being addressed. I respect the usability that an auto-generated list in the UI would offer, however I'm leery of verbose help text. My recommendation is to link to a static D.O. handbook page which should clearly note it very may well be outdated and offer guidance (including images on how to expand the view source toggle) and a link to the xss.php documentation. People who are comfortable writing HTML will be able to recognize the allowed tags and a big benefit would be a very welcoming way of introducing sitebuilders to api.drupal.org.

Inadvertently changed status; sorry.

If you read all the way back through the issue, there are quite a few points made about verbosity and the merits of inline vs external docs.

I do wonder is if it's worth opening a separate issue to discuss moving the Configure Field modal to a "normal" page of it's own. With or without this change it's very long and the scrolling behaviour with opening various sections isn't great (nor I imagine, the experience trying to change anything on a mobile device). A quick search of past issues thew up this about accessibility: #1806308: Review Views JavaScript + generic modals for accessibility - and there are various others requiring fixes to the width, how modals behave after saving etc.

Also, would our conventions permit the Twig template link opening in a new window? Given that if you use the back button, you're sent back to the view edit page and any changes you made in the modal are lost.

It is good that this patch addresses that the Views message in question is also pointed to the wrong twig version. It should point to the twig 1.x version. This is misleading enough that in theory it should be patched on the 8.3.x branch. (since it is addressed here I won't file another bug). I also updated the documentation to reflect this here: https://www.drupal.org/docs/8/theming/twig

I would support more verbose help text in this instance in particular. I think for Admin UI particularly at the site builder level is better to err on the site of larger rather than shorter messages.

Reviewed and re-rolled. I don't believe the slight extra scrolling is an important issue since it is important information.
Help text only, so no tests and no API changes so should be back portable to 8.6.x as well.
+1 RTBC.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -720,11 +720,27 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+          '@tags' => '<' . implode('> <', Xss::getAdminTagList()) . '>'

Nitpick: should have a comma at the end of the array value.

Address the cs issue.

Moving back to RTBC since my change was so minor.

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -720,11 +720,27 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      if (defined('Twig_Environment::VERSION')) {
   ...
   +        $twig_ver = (int) constant('Twig_Environment::VERSION') . '.x';
   

   Why do we need the constant() function. Also I'm not convinced that casting to int is the best way to extract the major version since \Twig_Environment::MAJOR_VERSION exists. Also why do we think the constant might not exist?

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -720,11 +720,27 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +        // Imploding the admin tag list is safe here because it is escaped by @
   +        // and because it comes from a known good list.
   +        // @see \Drupal\Core\Field\FieldFilteredMarkup::displayAllowedTags()
   

   I'm not sure that these comments are that useful. The fact that @ is escaped is properly documented elsewhere and the @see is not to related code. If we want a comment about security then something closer to the code like

    // The tag list will be escaped.
    '@tags' => '<' . implode('> <', Xss::getAdminTagList()) . '>',
   

   is clearer.

Thanks @alexpott - good point about the doc link, we can just link directly to the correct version like this I believe.
Also changed the comment lines you mentioned.

Crediting @StuartJNCC for creating the issue and @xjm, @cilefen for issue reviews.

Committed and pushed 6c724f8faf to 8.7.x. As a string change only committed to 8.7.x.