Convert Views t() usage where it is used as an attribute value

@catch
Can you specific which one you talk about?

I just see examples of @url and !url pretty much. Used "=\"@\"" and and =\"!\"

IS can be worked on before #2509218: Ensure that SafeString objects can be used in non-HTML contexts is in

This issue will need to add and test an HTML to attribute output formatter and test it.

Will be working on this

@laurii: @alexpott:

Actually @catch suggested in #2509218-142: Ensure that SafeString objects can be used in non-HTML contexts that we don't need and output strategy for HTML attributes, see also the recommended way in the related CR.

So $display_title is

  public function getDisplayLabel(ViewUI $view, $display_id, $check_changed = TRUE) {
    $display = $view->get('display');
    $title = $display_id == 'default' ? $this->t('Master') : $display[$display_id]['display_title'];
    $title = views_ui_truncate($title, 25);

    if ($check_changed && !empty($view->changed_display[$display_id])) {
      $changed = '*';
      $title = $title . $changed;
    }

    return $title;

So the problem is that t('Master') might have bad strings, which then ends up in the attribute. In the meantime I'll write on a test, just to be sure about it.

This creates double autoescape for the Enable view mode button. Reproduce by adding a view mode with html inside display name, then disable view mode.

I think it was View display not view mode

Here is a test coverage. It just proves that there is no problem yet.

Having a look to this one

Discussed with @alexpott and @dawehner a slightly different way: since attribute values will always be autoescaped by Twig, we just need to convert safe markup to string and we are done.

Moar test coverage

I updated the CR at https://www.drupal.org/node/2574697 to reflect the new approach taken here.

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -122,6 +124,11 @@ protected function createAttributeValue($name, $value) {
+      if (SafeMarkup::isSafe($value)) {
+        $value = PlainTextOutput::renderFromHtml($value);

Just curious, why are we testing the safeness here?

Failures...

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1307,7 +1307,10 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
   
   @@ -1307,7 +1307,10 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
        // long as $form['#name'] puts the value at the top level of the tree of
        // \Drupal::request()->request data.
        $input = $form_state->getUserInput();
   -    if (isset($input[$element['#name']]) && $input[$element['#name']] == $element['#value']) {
   +    // Browsers submit the unescaped version of the input submit value, but the
   +    // element "value" attribute is supposed to hold plain text, so HTML markup
   +    // should be escaped.
   +    if (isset($input[$element['#name']]) && Html::escape($input[$element['#name']]) == $element['#value']) {
          return TRUE;
   

   We should better expand the unit tests here

2. +++ b/core/modules/views_ui/src/Tests/DisplayTest.php
   @@ -215,6 +217,35 @@ public function testViewStatus() {
   +      $escaped = substr($input, 0, 25) . '...';
   

   We could just call out to views_ui_truncate()

Addressed #22, now working on removing ! support altogether.

@lauriii:

Just curious, why are we testing the safeness here?

Because safe strings have already been escaped, instead unsafe strings need regular escaping.

Just a small clean-up, removing of ! is being addressed in #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format(). If this is green we should be done here.

Updated IS

The patch is looking great beside this additional test coverage

Is there test coverage for the change in FormBuilder?

Yes, see the FormBuilderTest hunk.

Its green and I consider it as green.

Should the Attribute class explicit document this new behaviour? It has quite some documentation already.

Added docs. I reviewed the additional test coverage @dawehner provided and this is RTBC on my end.

Perfect!

There are two still !placeholder in Views with this applied:

core/modules/views/src/Plugin/views/filter/StringFilter.php:        'short' => $this->t('!has'),
core/modules/views_ui/src/ViewEditForm.php:            '#value' => $this->t('Duplicate as !type', array('!type' => $label)),

Should those be addressed here or in a separate issue?

@xjm:

I'll remove them in #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format().

Nope, sorry, one is an attribute.

Given our discussion the second one needs some conversion, the first one is totally fine.

So we talked about the first hunk there; it's not a placeholder... there are no $args. It's just actually the string !has.

Not sure how a translator would ever translate that into their own language since it just looks like a placeholder. :P But that's not in scope here.

Second hunk though will need to be addressed.

Fixed #36.

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -456,7 +456,7 @@ public function getDisplayDetails($view, $display) {
+            '#value' => $this->t('Duplicate as @type', ['@type' => $label]),

so $label is coming from \Drupal\views\Views::fetchPluginNames, which does the following bits:

      if (empty($plugin['no_ui']) && (empty($base) || empty($plugin['base']) || array_intersect($base, $plugin['base']))) {
        $plugins[$id] = (string) $plugin['title'];
      }

which will loose safeness

Alright

Thank you!

Slightly confused why this breaks. If we don't decode entities for both variables, then we decode entities for both variables, why would the former comparison fail?

Does that mean we need to check first for safeness on the $element['#value'] before converting it and otherwise use the raw value?

Slightly confused why this breaks. If we don't decode entities for both variables, then we decode entities for both variables, why would the former comparison fail?

The comparison may fail if we provide a safe string as $element['#value'] having HTML markup inside (HTML-escaped during the comparison): the value will be sent as unescaped by the browser. For instance:

$element['#value'] = t('Tag @foo', ['@foo' => '<foo>']);
// In the page source appears as "Tag &lt;foo&gt;"
// The browser posts "Tag <foo>"

Does that mean we need to check first for safeness on the $element['#value'] before converting it and otherwise use the raw value?

I don't think so, I discussed this with @alexpott: the current approach will work in any scenario (as the tests show).

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1307,7 +1307,9 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
+    // Browsers submit the unescaped version of the input value, so we need to
+    // make sure the compared values have the same "encoding".
+    if (isset($input[$element['#name']]) && Html::decodeEntities($input[$element['#name']]) == Html::decodeEntities($element['#value'])) {

It is not just that browsers submit the unescaped version of the input value is that they basically to a decode entities on the value of a button. <input type="button" value="Cancel <br&gt;"> will make a button that looks like this...

I've tested this on multiple browsers on a Mac:
Chrome Version 45.0.2454.99 (64-bit)
Firefox 40.0.3
Safari Version 8.0.8

The relevant docs are http://www.w3.org/TR/html401/interact/forms.html#h-17.4 which says that value is CDATA which means :

• Replace character entities with characters,
• Ignore line feeds,
• Replace each carriage return or tab with a single space.

According to http://www.w3.org/TR/html401/types.html#type-cdata

Improved docs, thanks for the pointers :)

the first hunk is used in #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format() as well, maybe we just copypaste from what we did there or wait for that to go in first? It also has tests.

Better version + some changes from #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format().

Thank you @plach
This documentation helps people to understand things and well, it seems to be an outstanding bug since forever in form API. If someone cares about it,
them could create a new isssue for backporting this particular change.

(No smiley here)

See #2575789: Decode entities when determining which button was clicked for the D7 backport, it just needs tests