Updated: Comment #N

Problem/Motivation

Add nid field using views ui.
See double escaped dialog title.
dbl esc

This also affects standard core dialogs like e.g. the following issue that wants to use dialogs for delete confirmation forms: #2253257: Use a modal for entity delete operation links or any other dialog that e.g. contains em tags, characters that are encoded when displayed (like ') and so on.

Proposed resolution

The solution in the last patch #44 is to convert the HTML into simple plain text using PlainTextOutput::renderFromHtml() for the title. It returns a string with HTML tags stripped and HTML entities decoded, which is what we need.

Remaining tasks

User interface changes

Dialog titles that contain HTML tags or characters that would be escaped like < or ' are displayed correctly.

API changes

None.

CommentFileSizeAuthor
#64 2207247-64.patch6.94 KBjoelpittet
#61 2207247-8.2-modal-dialog-dbl.61.patch9.94 KBlarowlan
#61 interdiff.txt2.63 KBlarowlan
#58 2207247-8.2-modal-dialog-dbl.58.patch9.62 KBlarowlan
#58 interdiff.txt2.73 KBlarowlan
#55 2207247-8.2-modal-dialog-dbl.54.patch7.28 KBlarowlan
#55 interdiff.txt618 byteslarowlan
#52 2207247-modal-dialog-dbl.52.patch6.68 KBlarowlan
#52 interdiff.txt4.61 KBlarowlan
#45 Popup title displayed correctly.png62.13 KBsasanikolic
#44 dialog_titles_double-2207247-44-interdiff.txt2.03 KBsasanikolic
#44 dialog_titles_double-2207247-44.patch8.17 KBsasanikolic
#42 Error_snapshot.JPG183.62 KBTruptti
#35 dialog_titles_double-2207247-35-interdiff.txt4.67 KBsasanikolic
#35 dialog_titles_double-2207247-35.patch7.95 KBsasanikolic
#29 dialog_titles_double-2207247-29-tests-only.patch2.33 KBsasanikolic
#28 dialog_titles_double-2207247-28-interdiff.txt1.35 KBsasanikolic
#28 dialog_titles_double-2207247-28.patch4.79 KBsasanikolic
#27 dialog_titles_double-2207247-27-interdiff.txt1.6 KBsasanikolic
#27 dialog_titles_double-2207247-27.patch4.68 KBsasanikolic
#26 dialog-title-escaped-2207247-26.patch3.08 KBs_leu
#26 dialog-title-escaped-2207247-26.interdiff.txt3.41 KBs_leu
#25 dialog-title-escaped-2207247-25.patch3.17 KBmrjmd
#21 dialog-title-escaped-2207247-21.patch3.59 KBBerdir
#19 2207247-19.patch2.84 KBrpayanm
#16 2207247-16.patch2.67 KBolli
#16 2207247-interdiff.txt1.04 KBolli
#15 2207247-interdiff.txt1.05 KBolli
#15 2207247-15.patch1.64 KBolli
#12 8.x.pass_.patch1.42 KBlarowlan
#12 8.x.fail_.patch616 byteslarowlan
#4 dialog.png14.43 KBdawehner
#2 result.png16.14 KBdawehner
Screenshot 2014-02-28 12.07.31.png36.08 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Thank you for posting. I will have a look at it.

dawehner’s picture

Status: Active » Needs review
FileSize
16.14 KB

Ha, I debugged that until it went to javascript.

Are we sure that the dialog API does not try to do shit here?

nod_’s picture

If it is, it's jQuery UI Dialog, we're not doing anything special with the title on our end.

dawehner’s picture

FileSize
14.43 KB

Wrong screenshot ...

nod_’s picture

Issue tags: +JavaScript

Checked, jQuery UI does that.

nod_’s picture

Status: Needs review » Active

Using this solves the problem.

  $.widget('ui.dialog', $.ui.dialog, {
    _title: function (title) {
      if (!this.options.title) {
        title.html("&#160;");
      }
      title.html(this.options.title);
  });

Since we're already extending the dialog in #2113911: Modal style update it'd make sense to wrap that up in there.

dawehner’s picture

As written in IRC I think it would be totally fine to remove the HTML for the views title, though I consider it as a bad to deal with sanitization in javascript instead of PHP.

olli’s picture

#2126983-8: ConfigItemGroup form double escapes the form #title would remove the html in views, but something like #6 might be better. Is it safe to use options.title or should we add options.drupalTitle?

nod_’s picture

I'd rather strip out the tags. Page titles can't have HTML in them.

dawehner’s picture

But dialog titles can ... For such long titles as views UI has it is at least a little bit helpful.

larowlan’s picture

Title: Dialog titles double escaped for views handlers » Dialog titles double escaped for views handlers and delete forms
larowlan’s picture

Status: Active » Needs review
FileSize
616 bytes
1.42 KB

should be red/green

The last submitted patch, 12: 8.x.fail_.patch, failed testing.

dawehner’s picture

@larowlan
I would pretty much prefer an actual fix on the JS side but yeah if this is not possibe, let's wait on the other issue and write a dedicated test?

olli’s picture

FileSize
1.64 KB
1.05 KB

While waiting, added decodeEntities().

olli’s picture

FileSize
1.04 KB
2.67 KB

And here is #6 with drupalTitle for views.

LewisNyman’s picture

Issue tags: +frontend
Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch needs reroll.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
jhedstrom’s picture

Priority: Minor » Normal
Status: Needs review » Needs work

Needs another reroll. Bumping to at least normal. Double-escaping on the UI might even qualify as major?

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +Needs issue summary update
FileSize
3.59 KB

Had to create the patch from scratch, everything changed ;)

- Also added it to the modal renderer.
- Moved the strip_tags() call down to $title so that it runs for both #title and page title resolver calls.

I'm not exactly sure what else needs to be done here. The patch adds support for unescaped titles, but only views uses it, should we use that by default, combined with an Xss::filter() or so?

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/misc/dialog/dialog.ajax.js
    @@ -168,4 +168,18 @@
    +  $.widget('ui.dialog', $.ui.dialog, {
    +    _title: function (title) {
    +      if (this.options.drupalTitle) {
    +        title.html(this.options.drupalTitle);
    +      }
    +      else if (!this.options.title) {
    +        title.html("&#160;");
    +      }
    +      else {
    +        title.text(this.options.title);
    +      }
    +    }
    +  });
    

    Definitely needs docs to explain what it's doing and especially why.

  2. +++ b/core/modules/views/includes/ajax.inc
    @@ -53,6 +53,7 @@ function views_ajax_form_wrapper($form_class, FormStateInterface &$form_state) {
    +      'drupalTitle' => $title,
    

    Wouldn't something like rawTitle be more appropriate?

  3. +++ b/core/modules/system/tests/modules/ajax_test/ajax_test.routing.yml
    @@ -1,7 +1,7 @@
    -    _title: 'AJAX Dialog contents routing'
    +    _title: '<em>AJAX Dialog contents routing</em>'
    

    We don't yet have test coverage for this?

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: dialog-title-escaped-2207247-21.patch, failed testing.

mrjmd’s picture

Status: Needs work » Needs review
FileSize
3.17 KB

This is a straight reroll of #21, so the feedback from #22 still need to be sorted.

s_leu’s picture

The patch in #25 still applied but didn't work anymore. For some reason i found that the JS part isn't necessary anymore and the dialog titles are displayed properly without it as well, thus i removed it.

Didn't add tests for this yet as i wasn't really sure what needs to be tested now with the new patch. Could you give me a hint Wim?

sasanikolic’s picture

Added the test for views. Still need to extend the \Drupal\system\Tests\Ajax\DialogTest.

sasanikolic’s picture

Removed the html tags from the title in the routing file, since it doesn't matter. Instead, added the tags in the AjaxTestController, so we actually test them in the DialogTest.

sasanikolic’s picture

Status: Needs review » Needs work

The last submitted patch, 29: dialog_titles_double-2207247-29-tests-only.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review

Test only patch was expected to fail.

joelpittet’s picture

Attaching to a meta.

joelpittet’s picture

The meta has little to do with this @Berdir pointed out to me and I kinda knew when I added. Just want to give this some meta visibility, if there is another escaping meta floating around please swap it out.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -28,7 +28,7 @@ class AjaxTestController {
    -      '#title' => 'AJAX Dialog contents',
    +      '#title' => '<em>AJAX Dialog contents</em>',
    

    May want to put in a & in this to show that we aren't getting &amp; as well. And add a couple tests to assert that there is no '&amp'?

  2. +++ b/core/modules/views_ui/src/Tests/FieldUITest.php
    @@ -57,6 +58,24 @@ public function testFieldUI() {
    +    // Ensure that dialog titles are not double escaped.
    

    We are asserting there is no-escaping, not double escaping because we are stripping out the tags and decoding all the entities that may have been escaped.

sasanikolic’s picture

Added the test for & and fixed the comment.

maijs’s picture

maijs’s picture

dawehner’s picture

Status: Needs review » Closed (duplicate)

Yeah let's certainly mark this issue as duplicate. Having more issues doesn't really help :)

Berdir’s picture

Status: Closed (duplicate) » Needs review

We might need some more discussion on which issue is a duplicate of what?

The other issues are not addressing the tags problem for example, they only do decodeEntities().

Should we maybe treat them as plain text and use PlainTextOutput?

dawehner’s picture

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -247,7 +247,7 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
 
-      $response->addCommand(new OpenModalDialogCommand($title, $display, $options));
+      $response->addCommand(new OpenModalDialogCommand(Html::decodeEntities(strip_tags($title)), $display, $options));
 

See https://www.drupal.org/node/2207247#comment-8533791

dawehner’s picture

Should we maybe treat them as plain text and use PlainTextOutput?

I guess we have to? Too bad that the nice functionality of using a limited list of HTML tags got lost.

Truptti’s picture

FileSize
183.62 KB

The patch "dialog_titles_double-2207247-35.patch" in #35 has errors.Attaching snapshot for reference.
Updating the status to Needs Work.

Truptti’s picture

Status: Needs review » Needs work
sasanikolic’s picture

Rerolled the patch and changed decodeEntities() to renderFromHtml().

sasanikolic’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
62.13 KB

Providing a screenshot to prove that the title is now displayed without any HTML markups and updating the issue summary.

Berdir’s picture

Issue summary: View changes

Updating the issue summary a bit more.

We have tests and we have been using the patches from this issue in our news portal distribution where we are displaying nodes in a dialog for more than a year now. Never had any problems with it.

This is tagged Javascript, but we're not actually touching any JS anymore, so I think I can RTBC this.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So it is jQuery that is causing the double escaping? I wonder why we're not putting this behaviour in the constructor of OpenDialogCommand? Also I think we should be detecting if $title is an object that implements MarkupInterface and only doing the PlainTextOutput::renderFromHtml() then since that is the only time when we know we are doing what we are supposed to do.

If the above approach is wrong (and even it it is not) we need to document the expectation on both OpenDialogCommand and OpenModalDialogCommand.

Berdir’s picture

Yes, jQuery does the escaping AFAIK. So we need to send the unescaped HTML/plain text.

I also don't think that we should limit it to MarkupInterface. Any kind of input should be stripped of HTML tags and html encoded entities should be decoded, it's not relevant if it's safe or not.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.61 KB
6.68 KB

Reroll and fixes as per #48

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, 52: 2207247-modal-dialog-dbl.52.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
618 bytes
7.28 KB

meh

nod_’s picture

Issue tags: -JavaScript
dawehner’s picture

Status: Needs review » Needs work

We lack some documentation on those ajax commands what we are actually doing with the passed in title, as per #48

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
9.62 KB

Yep

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for adding more details to the docs, sending back to RTBC.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Ajax/OpenDialogCommand.php
@@ -57,8 +58,9 @@ class OpenDialogCommand implements CommandInterface, CommandWithAttachedAssetsIn
+   *   The title of the dialog. If the title is an instance of MarkupInterface,
+   *   it will be converted to plain text before being output.

+++ b/core/lib/Drupal/Core/Ajax/OpenModalDialogCommand.php
@@ -16,8 +16,9 @@ class OpenModalDialogCommand extends OpenDialogCommand {
+   *   The title of the dialog. If the title is an instance of MarkupInterface,
+   *   it will be converted to plain text before being output.

+++ b/core/modules/outside_in/src/Ajax/OpenOffCanvasDialogCommand.php
@@ -17,8 +17,9 @@ class OpenOffCanvasDialogCommand extends OpenDialogCommand {
+   *   The title of the dialog. If the title is an instance of MarkupInterface,
+   *   it will be converted to plain text before being output.

I don't want to be anal about it, but this describes what its doing vs. why. We should describe that the JS does its own escaping, so we prevent double escaping by doing that.

larowlan’s picture

FileSize
2.63 KB
9.94 KB

For you, sure

dawehner’s picture

Thank you @larowlan!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I also don't think that we should limit it to MarkupInterface. Any kind of input should be stripped of HTML tags and html encoded entities should be decoded, it's not relevant if it's safe or not.

From @Berdir...

+++ b/core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php
@@ -54,7 +53,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    $response->addCommand(new OpenDialogCommand($target, PlainTextOutput::renderFromHtml($title), $content, $options));
+    $response->addCommand(new OpenDialogCommand($target, $title, $content, $options));

+++ b/core/lib/Drupal/Core/Render/MainContent/ModalRenderer.php
@@ -34,7 +33,7 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    $response->addCommand(new OpenModalDialogCommand(PlainTextOutput::renderFromHtml($title), $content, $options));
+    $response->addCommand(new OpenModalDialogCommand($title, $content, $options));

+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -245,7 +245,7 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
-      $response->addCommand(new OpenModalDialogCommand(Html::decodeEntities(strip_tags($title)), $display, $options));
+      $response->addCommand(new OpenModalDialogCommand($title, $display, $options));

I think I was wrong in #48 and @berdir was right... sorry.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -frontend +Autoescape
FileSize
6.94 KB

@alexpott was this what you had in mind, I agree just strip tags on all things going into the modals and other js titles.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

lets do it

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 8f3e25c to 8.3.x and 3818756 to 8.2.x. Thanks!

+++ b/core/modules/views_ui/src/Tests/FieldUITest.php
@@ -55,6 +56,24 @@ public function testFieldUI() {
+    $this->assertNotEqual($data[3]['dialogOptions']['title'], 'Configure aggregation settings for field <em class="placeholder">Views test: Name</em>');
+    $this->assertEqual($data[3]['dialogOptions']['title'], 'Configure aggregation settings for field Views test: Name');

Asserting not equal and equal on the same thing is a bit weird... I'm removing the assertNotEqual.

  • alexpott committed 8f3e25c on 8.3.x
    Issue #2207247 by larowlan, sasanikolic, olli, s_leu, Berdir, joelpittet...

  • alexpott committed 3818756 on 8.2.x
    Issue #2207247 by larowlan, sasanikolic, olli, s_leu, Berdir, joelpittet...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.