Unnecessary space while removing the image in popup. Please check screencast of the issue.

I think the main reason of this issue is :

space

https://www.drupal.org/files/issues/pop-image.mp4

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Manjit.Singh created an issue. See original summary.

aneek’s picture

Version: 8.1.x-dev » 8.0.x-dev
Issue tags: +markup, +CSS

Seems to be an issue to me. I'll have a go on this.

Thanks!

aneek’s picture

Issue tags: -markup +frontend
Manjit.Singh’s picture

Issue tags: +Usability
Wim Leers’s picture

Title: Unnecessary space while removing the image in popup » Unnecessary space while removing the image in EditorImageDialog — potentially a generic AJAX file upload problem?
Issue tags: +Novice, +css-novice

Ohhh! Great find :) Thank you!

This seems like it'd be an issue also on the full node form, no?

aneek’s picture

FileSize
132.08 KB

@WimLeers, the HTML elements (<span class="ajax-new-content" style="display: inline-block;"></span>)do appear in the full node create form. Though I am not able to find the extra space. Screenshot attached.

Wim Leers’s picture

It must originate from either \Drupal\file\Element\ManagedFile::uploadAjaxCallback() or \Drupal\Core\Field\WidgetBase::addMoreAjax(). In this case, it's clearly the first.

The fact that you see those same elements appear on the node form without the whitespace problem means that the whitespace problem is caused by CSS. So we'll need to analyze the CSS to find the problem.

A quick test reveals that changing that display: inline-block; to display: none; fixes the problem. But I'm sure that current inline style is there for a good reason. So we must look at the CSS of the surrounding elements. I don't see it yet…

Wim Leers’s picture

Title: Unnecessary space while removing the image in EditorImageDialog — potentially a generic AJAX file upload problem? » Unnecessary space while removing the image in EditorImageDialog

Updating the title since #6 answered #5, so we can make the title more specific again.

serg2’s picture

Are you sure it is a CSS issue rather than JS?

The white space is added on both the modal and the node page.
On the node page it is directly below the the "Allowed types: png gif jpg jpeg."

Clicking "Remove" images seems to adds both:
<span class="ajax-new-content" style="display: inline;"></span>

<span class="ajax-new-content" style="display: inline-block;"></span>
in sequence. This occurs every time Remove is clicked, whether on the modal or node.

Manjit.Singh’s picture

FileSize
99.52 KB

yeah, right @Lostandfound !! The same is on node page.

ajax

aneek’s picture

FileSize
47.83 KB

@WimLeers, I missed the node creation page, this is also present in the node creation page. Please have a look at the attached image. I had a curious look at it and found that the new_content object coming in core/misc/ajax.js line # 912 has the "innerHTML" where <span class="ajax-new-content" style="display: inline-block;"></span> is added.

So it seems to me that this might be an JS issue but I am not able to find from where the element attribute style="display: inline-block;" is being added. Changing the title to a generic one that should cover Node Add page and the image editor dialog window.

Thanks.

aneek’s picture

Title: Unnecessary space while removing the image in EditorImageDialog » Span Class "ajax-new-content" adds extra space with inline-block display
Manjit.Singh’s picture

Is it possible to changes <span class="ajax-new-content" style="display: inline-block;"></span> into <span class="ajax-new-content" style="display: inline;"></span>.

Because style="display: inline-block;" added a space. If we can change it as i mentioned earlier then our problem will get resolved.

Wim Leers’s picture

This is starting to smell like a jQuery.hide()-induced problem.

aneek’s picture

A quick patch. @WinLeers I think not only .hide() but [effect.showEffect](effect.showSpeed) has effects on this too. Since the effects can be dynamic and can have different functions like .hide(), .show()etc. So I've added extra CSS attribute forcefully for inline display.

I'd love to hear some reviews from UI guys :)

FYI: Changing the components and other since this is associated with ajax.js. Also this needs some manual testing.

Thanks!

aneek’s picture

Component: CSS » ajax system
Status: Active » Needs review
Issue tags: -CSS, -css-novice +Needs manual testing
Sagar Ramgade’s picture

The approach seems to working, me too tried the same :-)

snehi’s picture

Status: Needs review » Reviewed & tested by the community

+1 for RTBC Working for me too :)
This should be gone to RTBC.

Manjit.Singh’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
147.99 KB

Thanks Aneek !! Looks good now.

Can we remove the unnecessary span's. Seems like HTML markup is getting to complex. and we don't need that in D8.

issue

Wim Leers’s picture

#19: that's a separate issue. It's independent from the problem reported here. Can you open a separate issue for that? :) Thanks!

I think this should be reviewed by @nod_ or @droplet, and preferably also by a strong CSS person.

Manjit.Singh’s picture

Wim Leers’s picture

Title: Span Class "ajax-new-content" adds extra space with inline-block display » <span class="ajax-new-content" style="display:inline-block;"> causes unwanted whitespace

Clarifying title.

droplet’s picture

why do we adding an empty placeholder ?

Wim Leers’s picture

HAH!

Good question.

We would need an AJAX system expert to know the answer to that. Surely this was there for a reason? Doesn't this break something?

serg2’s picture

These spans are created in Drupal 7 nodes too, both types:
<span class="ajax-new-content" style="display: inline;"></span>
and

It was not an issue before because the following css was applied to 'spans':

margin: 0;
    padding: 0;
    border: 0;
    vertical-align: baseline;

This CSS was provided by /themes/seven/reset.css . Reset was removed in #723392: Tame seven's reset.css and regressions fixed in #1837514: Fix visual regressions from taming Seven’s reset.

Presumably the same upload/remove functionality was copied from the node to the modal so the issue appears there too.
The AJAX doesn't appear to have changed since 7 but the CSS has, hence the white-space appearing now.

The display regression could be fixed in CSS but probably best to fix/understand what the AJAX is doing.

serg2’s picture

Manjit.Singh’s picture

Issue summary: View changes
anil280988’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
42.67 KB
50.8 KB
30.76 KB
28.26 KB

Patch worked fine for me.
Both the spacing issue from editor as well as Image Fields.

Editor Before Patch

ajax-new-content_0.png

Editor After Patch

ajax-new-content_0.png

ImageField Before Patch

ajax-new-content_0.png

ImageField After Patch

ajax-new-content_0.png

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

We would need an AJAX system expert to know the answer to that. Surely this was there for a reason? Doesn't this break something?

This has not yet had sufficient reviews.

droplet’s picture

Yes, needs a AJAX expert to figure out if there's anything do not documented and widely used in popular modules. Reading from code side, it can be removed safely.

Wim Leers’s picture

Assigned: Unassigned » effulgentsia
Issue tags: +Needs subsystem maintainer review

From MAINTAINERS.txt:

Ajax system
- Alex Bronstein 'effulgentsia' https://www.drupal.org/u/effulgentsia
- Earl Miles 'merlinofchaos' https://www.drupal.org/u/merlinofchaos

Therefore assigning to @effulgentsia.

Wim Leers’s picture

Issue tags: -Novice
anil280988’s picture

As per the code, this function is called only when a file is successfully uploaded.

if (isset($form['#file_upload_delta']) && $current_file_count < $form['#file_upload_delta']) {
      $form[$current_file_count]['#attributes']['class'][] = 'ajax-new-content';
}

Basically, it checks if the file is uploaded and if the current File count is more then the initial count that means file has been uploaded, and it adds 'ajax-new-content' to the attribute.

When the file is removed, the $current_file_count goes decreases so the condition goes to false case.

else {
      $form['#suffix'] .= '<span class="ajax-new-content"></span>';
}

Which add the as suffix causing the empty space.

I have checked this for upload in both file field and Editor also. And its not impacting anything else.

nod_’s picture

tic2000’s picture

FileSize
27.02 KB

I investigated a little bit this issue and I found multiple problems with image upload and since it uses mainly the file upload at it's base I assume that's the case for file uploads too.

First, $form['#file_upload_delta'] and $current_file_count exist only when we deal with a multiple field widget. Which means in the case of a single image upload we always add the span. The questions are if is that required in case of a single image widget and why that can't be added as a class to the element?

A second issue I have is that in the case of single file widget it wraps all in a div with id "ajax-wrapper", but in the case of a multi field it uses "edit-field-image-ajax-wrapper" (the field has an machine name of "field_image"), both of which are not used. What happens is that those are replaced but the command passes NULL.

A third issue is that the IDs in the second issue are replaced with ajax "unique" IDs, which in the case of multiple file widgets gives us this:
Duplicate IDs

I will not talk here about the problem with nested wrapping cause that's because of #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs.

With the exception of the 3rd issue, all of the other would be solved with smarter AJAX Commands.
IE: No matter what, the whole widget is always fully reloaded. So instead of using a ReplaceCommand use a HtmlCommand on data-drupal-selector="edit-[field-name]-wrapper". This would solve even the nesting, by always being nested in only one div, and not what it does now.

Another solution would be to run one invoke command before the replace which removes data-drupal-selector="edit-[field-name]-wrapper" span.ajax-new-content. This would not solve the endless nesting though.

tic2000’s picture

Status: Needs review » Needs work

This needs work because the patch always removes the span. That is added when a file is removed or every time an action is performed on a single file widget. I think there is a reason for it's existence and should not be removed. Even if seven right now doesn't provide a style for it, other themes might use it.

nod_’s picture

If it was wrapping the HTML of the managed field it would make sense. As it is now it's totally useless.

tic2000’s picture

Status: Needs work » Needs review
FileSize
860 bytes

Or we could just add the class to the container.

While working with this I noticed that the messages are added twice, one before the container and once before the upload element. There is an issue opened for that?

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.

sumthief’s picture

Hi. Have the same problem in Drupal 7.
Looks like patch from #38 works good and solves the problem.
Here is patch for Drupal 7.43 version. Maybe it will be helpfull for somebody.

Status: Needs review » Needs work

The last submitted patch, 40: 2588013-40-D7Backport.patch, failed testing.

Manjit.Singh’s picture

@Shlyapkin It seems like you have created a patch file wrongly.
/docroot/modules/file/file.module this path is not exist in Drupal 7. Can you check that please.

Manjit.Singh’s picture

Version: 8.1.x-dev » 8.2.x-dev
Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
227.93 KB

Looking into #38 and <span class="ajax-new-content" style="display: inline;"></span> is not going to create anymore. Looks good now. Thanks @tic2000 :)

But i was thinking that Is there any need to add ajax-new-content class ? Because there is no style associate with it. Please check the screenshot.

upload

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -160,9 +160,9 @@ public static function uploadAjaxCallback(&$form, FormStateInterface &$form_stat
     if (isset($form['#file_upload_delta']) && $current_file_count < $form['#file_upload_delta']) {
       $form[$current_file_count]['#attributes']['class'][] = 'ajax-new-content';
     }
...
+    // Otherwise just add the class to the container.
     else {
-      $form['#suffix'] .= '<span class="ajax-new-content"></span>';
+      $form['#attributes']['class'][] = 'ajax-new-content';
     }

See if we can remove the above code.
suggestions please.

sumthief’s picture

@Manjit.Singh, thank you for the catch.
I've recreated patch for 7.x branch.
Also about your suggestion about class 'ajax-new-content' unnecessary:
I don't sure if D8 have the same logic, but in D7 we have the code snippet in misc/ajax.js:

// Determine which effect to use and what content will receive the
    // effect, then show the new content.
    if ($('.ajax-new-content', new_content).length > 0) {
      $('.ajax-new-content', new_content).hide();
      new_content.show();
      $('.ajax-new-content', new_content)[effect.showEffect](effect.showSpeed);
    }
    else if (effect.showEffect != 'show') {
      new_content[effect.showEffect](effect.showSpeed);
    }

Looks like this class is the basis of jQuery selector for manipulations with new content.

Status: Needs review » Needs work

The last submitted patch, 44: 7.x-ajax-file-extra-wrapper-2588013-44.patch, failed testing.

droplet’s picture

RE: @tic200 #35 ~ #38,

I still can't see any reason to leave it. Please remind me again if I missed any points.

Let me guess why it's there.

This file introduced from
https://www.drupal.org/node/544418#comment-1932744
(no if-else condition at that time)

Then, it refactored to file field API and introduced if-else
https://www.drupal.org/node/391330#comment-1974982

There is 3 reason I think:

1. PHP Developers assumed if there's any content, we join the string. Otherwise, left as it with if-else condition.
2. Addition to Point 1, they don't know what that is also. Or say they don't mind what that is. I always see these happened in big refactoring commits really.
3. PHP Developers made an assumption that may use or cause script syntax error in the following code:
$('.ajax-new-content', new_content).length

msankhala’s picture

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nod_’s picture

Status: Needs work » Needs review
Issue tags: -Needs subsystem maintainer review
FileSize
817 bytes

I agree with @droplet, the span should be removed. Adding the class to the whole form is a little dangerous and i don't see the use for it.

Let's see if anything breaks.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.