Problem/Motivation

Untranslatable fields are extended with a "all translations" hint to make sure the editor understands the affect of changing it.

Since the recent change in SafeMarkup, the UI with "all languages" output is broken.
#2506195: Remove SafeMarkup::set() from Xss::filter()

Proposed resolution

Follow the change, improve test coverage.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#25 html_escaped_fix_revision.patch409 bytessamimalik
#23 html_is_escaped_in_all-2532284-23.patch931 bytesPeacog
#22 all_languages_markup_in_detail_title.patch1007 bytesayalon
#21 Screen Shot 2016-02-29 at 09.55.49.png10.88 KBreekris
#16 html_is_escaped_in_all-2532284-16-interdiff.txt4.47 KBsasanikolic
#16 html_is_escaped_in_all-2532284-16.patch3.44 KBsasanikolic
#14 html_is_escaped_in_all-2532284-14-interdiff.txt4.79 KBsasanikolic
#14 html_is_escaped_in_all-2532284-14.patch6.17 KBsasanikolic
#11 html_is_escaped_in_all-2532284-11-interdiff.txt1.07 KBsasanikolic
#11 html_is_escaped_in_all-2532284-11.patch2.4 KBsasanikolic
#9 Screen Shot 2015-07-30 at 16.36.04.png16.17 KBsasanikolic
#9 html_is_escaped_in_all-2532284-9-interdiff.txt1.05 KBsasanikolic
#9 html_is_escaped_in_all-2532284-9.patch1.33 KBsasanikolic
#7 Edit_Article_Test__French_translation____drupal_8_0_x.png45.31 KBjoelpittet
#7 Create_French_translation_of_Test___drupal_8_0_x.png17.38 KBjoelpittet
#6 html_is_escaped_in_all-2532284-6.patch1.39 KBjoelpittet
#5 html_is_escaped_in_all-2532284-5.patch1.49 KBjoelpittet
#2 html_is_escaped_in_all-2532284-2.patch1.24 KBcilefen
Screen Shot 2015-07-13 at 17.28.39 .png15.74 KBmiro_dietiker
Screen Shot 2015-07-13 at 17.28.25 .png21.28 KBmiro_dietiker
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen’s picture

Title: Broken "all languages" in UI after SafeMarkup change » HTML is escaped in "all languages" in UI after SafeMarkup change
Parent issue: » #2297711: Fix HTML escaping due to Twig autoescape
cilefen’s picture

Status: Active » Needs review
FileSize
1.24 KB
miro_dietiker’s picture

Status: Needs review » Needs work

This does not resolve neither of both cases in screenshot above for me.

cilefen’s picture

Ha - I didn't actually test it. Could you provide steps to reproduce. I know little about content translation. How do I get that HTML to appear?

joelpittet’s picture

Component: render system » forms system
Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs manual testing
FileSize
1.49 KB

@cilefen I think your solution there is fine, but the real problem is further down the stack.

Xss::filterAdmin in template_preprocess_form_element_label().

Should be using SafeMarkup as they are now separated nicely for their appropriate domains:)

joelpittet’s picture

I didn't need the extra parens in there and twig with strict off doesn't need to initialize the variables, so this is a bit cleaner.

Since @alexpott worked hard on the other one, it would be good to get his opinion here. Also SafeMarkup::checkAdminXss() looks like a good use case here but it's been deprecated also.

joelpittet’s picture

Here's some manual testing on simplytest.me:

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.inc
@@ -191,7 +191,10 @@ function template_preprocess_fieldset(&$variables) {
+    $variables['legend']['title'] = SafeMarkup::xssFilter($element['#title'], Xss::getAdminTagList());

@@ -489,7 +492,10 @@ function template_preprocess_form_element(&$variables) {
+    $variables['title'] = SafeMarkup::xssFilter($element['#title'], Xss::getAdminTagList());

I don't think we need to be doing the escaping here ['#markup' => $element['#title']] should make the render system do this later.

sasanikolic’s picture

Here's the updated patch and a screenshot.

Berdir’s picture

Status: Needs review » Needs work

I think this addressed the feedback from @alexpott, but we still need review.

We can probably just add some assertRaw statements to the existing content translation UI tests?

sasanikolic’s picture

Added the test (as suggested in #10).

Status: Needs review » Needs work

The last submitted patch, 11: html_is_escaped_in_all-2532284-11.patch, failed testing.

olli’s picture

How is this different from #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()? It looks like the source of this is the concatenation in ContentTranslationHandler::addTranslatabilityClue().

sasanikolic’s picture

sasanikolic’s picture

Fixed the failing tests.

sasanikolic’s picture

mErilainen’s picture

Status: Needs review » Reviewed & tested by the community

Fixes the rendered html and tests pass now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bfb0580 and pushed to 8.0.x. Thanks!

  • alexpott committed bfb0580 on 8.0.x
    Issue #2532284 by sasanikolic, joelpittet, cilefen, miro_dietiker: HTML...

Status: Fixed » Closed (fixed)

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

reekris’s picture

I'm still having this issue in the details field using Drupal 8.0.4. HTML is escaped in the details title. See attached screenshot.

ayalon’s picture

Status: Closed (fixed) » Needs review
FileSize
1007 bytes

The bug is not fully resolved as you forgot to also fix the template_preprocess_details(9 function. I made a patch. This should be fixed aswell

Peacog’s picture

Version: 8.0.x-dev » 8.1.x-dev
FileSize
931 bytes

Thanks for the new patch @ayalon. I've re-rolled it and set version to 8.1.x. Let's see if it passes tests.

joelpittet’s picture

Status: Needs review » Closed (fixed)

Try to avoid re-opening issues, it will get confusing to what the issue was trying to solve. Instead create a follow-up. But first try to see if an existing issue exists.

This exact patch is here #2652850: Title for details form elements is not set as '#markup' and it will be escaped, but all other form elements use '#markup' and are not escaped. It's good to post a comment to cross-reference if they are related.

samimalik’s picture

The last patch did not completely fix the issue, if you tried to go and modify content (a page with a form field) or edit any form an error occurs that causes the site to break. Fixed.

handkerchief’s picture

on date fields the label problem still exists. For more details follow the issue linked in #24