Usage of field_prefix and container-inline creates invalid markup.

Hi,

I can confirm that the usage of the #field_prefix and #field_suffix in the max_resolution or the min_resolution item type key causes an invalid markup but with a little different output. Here is the structure I encountered:

<div id="edit-settings-max-resolution">
  <span class="field-prefix">
    <div class="container-inline">
      <div class="form-item-x">...</div>
      <div class="form-item-y">...</div>
      <span class="field-suffix"></div>
    </div>
  </span>
</div>

To view/reproduce this issue:

1. Go to the Article content type's Image Settings Edit tab(/admin/structure/types/manage/article/fields/node.article.field_image)
2. Inspect the elements of the Maximum image resolution and/or Minumum image resolution

Here is the screenshot before the patch was applied:

Here is the screenshot after the patch was applied:

Hope everything is in order.

Thanks!

Here I have attached the patch and screenshot containing some new changes in the markup based from the previous one in 8.0.x. I also followed the Re-rolling patches guide.

This is the screenshot of the output after applying the re-rolled patch:

The commit that worked was from January 5, 2016 using `git log -1 --before="January 5, 2016"` and the commit hash is 425d682aa6a6b97e42f15813885ad0603b018a37. I have also tried to apply it in 8.1.x and 8.2.x branches and all worked.

Hope everything are in order.

Thanks!

Two small remarks about your patch:

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -208,7 +208,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
       '#field_prefix' => '<div class="container-inline">',
       '#field_suffix' => '</div>',

I think that you forgot to remove these lines.

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -208,7 +208,7 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
-      '#description' => t('The maximum allowed image size expressed as WIDTH×HEIGHT (e.g. 640×480). Leave blank for no restriction. If a larger image is uploaded, it will be resized to reflect the given width and height. Resizing images on upload will cause the loss of <a href="http://wikipedia.org/wiki/Exchangeable_image_file_format">EXIF data</a> in the image.'),
+      '#description' => t('The maximum allowed image size expressed as WIDTH×HEIGHT (e.g. 640×480). Leave blank for no restriction. If a larger image is uploaded, it will be resized to reflect the given width and height. Resizing images on upload will cause the loss of <a href=":url">EXIF data</a> in the image.', array(':url' => 'http://en.wikipedia.org/wiki/Exchangeable_image_file_format')),

And why did you change the description line?

@leolando.lan: Can you create an interdiff.txt file for me. Makes reviewing your patch easier.

@daffie thanks for the initial review. I think I missed this and will definitely look into this and get back to you asap.

Here I have created a new patch and looked into the details of applying the patch more carefully.

Conflict

First, rewinding head to replay your work on top of it...
Applying: 2667340-4-8.2.x-roll-back-new
Using index info to reconstruct a base tree...
M       core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
CONFLICT (content): Merge conflict in core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
Failed to merge in the changes.
Patch failed at 0001 2667340-4-8.2.x-roll-back-new

Patch and Interdiff creation

1. `git pull origin 8.2.x` to get new upstream changes
2. Created a branch from Jan. 5 where the patch cleanly applies
3. Committed the applied patch update
4. Branched off and did `git rebase 8.2.x`
5. Fixed the conflict on core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
6. Fixed the conflicts in the description section of the said file
7. Compared the two files updated by the patch and did `git rebase --continue`
8. Did, the normal command `git diff 2667340-4-8.2.x-roll-back-new >interdiff.txt`

I'm not sure on this part because there are other files changes from Jan 5 and the present rebased branch which I don't know how to remove. I just didn't include it. I would need guidance on this part its really required.

Hope everything is in order.

Thanks!

It all looks good to me.

The patch fixes the invalid markup.

Thanks everyone for the work on this so far. I don't know the correct fix but what is being proposed here seems problematic to me. The patch is moving the prefixing and suffixing to the child elements that are first and last which seems rather fragile.

How about instead of creating manual HTML wrappers, we:

1. Add another form api container that wraps the width and height fields
2. Add the "Maximum/Minimum image resolution" labels and its description to the top wrapper.

So the second form api wrapper would replace the container-inline div and we womt be breaking up the opening and closing divs to the prefix and suffix of the form items.

I'll try to do this.

Hi @Cottser or anyone,

Proposed solution

Use a container form element to wrap the x and y fields.

Editor before and after screenshots

Before

After

Image field settings before and after screenshots

Before

After

Things done

• Used a container form element to wrap the x and y fields.
• Made sure the entered values get saved and retrieved.
• Looked for some other instances of the fields like in validation, submission forms and tests to update the array structure too if needed.
• Ran the Editor module's tests because it was the one affected with the change. Command used: `php core/scripts/run-tests.sh --module editor`

SimpleTest Result for the Editor module

$ php core/scripts/run-tests.sh --module editor

Drupal test run
---------------

Tests to be run:
  - Drupal\editor\Tests\EditorAdminTest
  - Drupal\editor\Tests\EditorLoadingTest
  - Drupal\editor\Tests\EditorSecurityTest
  - Drupal\editor\Tests\EditorUploadImageScaleTest
  - Drupal\editor\Tests\QuickEditIntegrationLoadingTest
  - Drupal\editor\Tests\Update\EditorUpdateTest
  - Drupal\Tests\editor\Kernel\EditorFileReferenceFilterTest
  - Drupal\Tests\editor\Kernel\EditorFileUsageTest
  - Drupal\Tests\editor\Kernel\EditorFilterIntegrationTest
  - Drupal\Tests\editor\Kernel\EditorImageDialogTest
  - Drupal\Tests\editor\Kernel\EditorManagerTest
  - Drupal\Tests\editor\Kernel\QuickEditIntegrationTest
  - Drupal\Tests\editor\Unit\EditorConfigEntityUnitTest
  - Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest

Test run started:
  Friday, May 20, 2016 - 09:46

Test summary
------------

Drupal\editor\Tests\EditorAdminTest                          153 passes
Drupal\editor\Tests\EditorLoadingTest                        101 passes
Drupal\editor\Tests\EditorSecurityTest                       265 passes
Drupal\editor\Tests\EditorUploadImageScaleTest                 9 passes             1 exceptions
- Found database prefix 'simpletest771081' for test ID 70.
Drupal\editor\Tests\QuickEditIntegrationLoadingTest           72 passes
Drupal\editor\Tests\Update\EditorUpdateTest                  223 passes
Drupal\Tests\editor\Kernel\EditorFileReferenceFilterTest       1 passes
Drupal\Tests\editor\Kernel\EditorFileUsageTest                 1 passes
Drupal\Tests\editor\Kernel\EditorFilterIntegrationTest         1 passes
Drupal\Tests\editor\Kernel\EditorImageDialogTest               1 passes
Drupal\Tests\editor\Kernel\EditorManagerTest                   1 passes
Drupal\Tests\editor\Kernel\QuickEditIntegrationTest            4 passes
Drupal\Tests\editor\Unit\EditorConfigEntityUnitTest            1 passes
Drupal\Tests\editor\Unit\EditorXssFilter\StandardTest        194 passes

Test run duration: 8 min 43 sec

Hope everything is in order.

Thanks!

Ah! Thanks @twistor! I'll try to apply your recommendation. I've been trying to work on this but I'm quite noob still on configurations and testings.

Here are the initial changes that @twistor has recommended.

Current issue

As I was testing the fields, I noticed that on the Text Format's Maximum Dimensions width and height field values doesn't get saved. I'm not sure on what's wrong but it might be the #parents attribute isn't being "respected" when saving the values for the Editor module?

Any thoughts or help would be very appreciated.

Thanks!

This issue blocks #2441373: Upgrade tests to HTML5 which in turn blocks #2441811: Upgrade filter system to HTML5 and #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5

This is not just about cases when there is a block-level tag in #field_prefix or #field_suffix.

Every case affected when #field_prefix contains any kind of

• a start tag without end tag
• an end tag without start tag

(...and this is true for #field_suffix too.)

Check this example in Views module's PathPluginBase.php:

$form['path'] = [
  '#type' => 'textfield',
  '#title' => $this->t('Path'),
  '#description' => $this->t('This view will be displayed by visiting this path on your site. You may use "%" in your URL to represent values that will be used for contextual filters: For example, "node/%/feed". If needed you can even specify named route parameters like taxonomy/term/%taxonomy_term'),
  '#default_value' => $this->getOption('path'),
  '#field_prefix' => '<span dir="ltr">' . $this->url('<none>', [], ['absolute' => TRUE]),
  '#field_suffix' => '</span>&lrm;',
  '#attributes' => ['dir' => LanguageInterface::DIRECTION_LTR],
  // Account for the leading backslash.
  '#maxlength' => 254,
];

I'd bet that here the developer assumed that the textfield <input /> will be wrapped by the <span dir="ltr"> tag.
I'm really just guessing that the expected output is something like this:

<div class="js-form-item form-item ...">
  <label>Path</label>
  <span dir="ltr">
    <input />
  </span>&lrm;
  <div class="description">
    This view will be displayed by visiting...
  </div>
</div>

What actually happens there:

• the <span dir="ltr"> get's closed right after the url, so
• the <input /> will be the child of the prefix wrapper span;
• the end tag in the field suffix will be wrapped in the suffix wrapper span, so it will actually close that tag immediately
• and finally, the end tag of the suffix wrapper span will close the prefix wrapper span

So the output will be:

<div class="js-form-item form-item ...">
  <label>Path</label>
  <span class="field-prefix">
    <span dir="ltr"></span>
    <input />
    <span class="field-suffix"></span>&lrm;
  </span>
  <div class="description">
    This view will be displayed by visiting...
  </div>
</div>

And this happens even if System module's, Stable or Classy's form-element.html.twig template is used.

I'm sure that we have to make a decision. These are out options IMHO:

1. Remove every non-closed/non-opened html tags from every #field_prefix and #field_suffix and document that they cannot contain them (possibly a BC break?)
2. Do not use <span class="field-prefix"/> and <span class="field-suffix"/> anymore (template change; definitely BC break, lower markup semantics)
3. Check somehow that #field_prefix and #field_suffix contains widow or orphan tags and conditionally output wrapper spans (at least template change; definitely BC break)

This is blocking #2441373: Upgrade tests to HTML5 which in turn blocks another major bug (and even another bug behind that one), so bumping to major.

Remove every non-closed/non-opened html tags from every #field_prefix and #field_suffix and document that they cannot contain them (possibly a BC break?)

If this is viable it seems like a good option, and it's not a bc break just a documentation of existing behaviour.

#2667340-19: Usage of field_prefix and container-inline creates invalid markup. #2 might be acceptable despite the markup change, if it's the only way to fix the bug.

I was about to reroll the patch to get started on the issue but this bug has been fixed in #3072752: Invalid markup in ImageItem (causes test failures with symfony/dom-crawler:4.3+). We can close this as a duplicate now.