Open ghost opened 6 years ago
olafgrabienski
commented
6 years ago That's an interesting issue! I can confirm the behaviour: When you change Align to Center, you get an empty paragraph followed by a wrapper div around the image.
Two observations:
div isn't there. It appears only when I view the node.div is even rendered if I don't allow this HTML tag in the text format configuration.
ghost
commented
6 years ago Also, more weirdness if you try to add a link to the centered image (link gets added to empty paragraph AND div).
ghost
commented
6 years ago The problem appears to lie with the _filter_image_align() function in filter.module.
Putting a dpm($text); at the beginning of that function shows the incoming HTML:
<p>Some text and paragraphs here...</p>
<p><a href="/files/images/crocosmia.jpg"><img alt="" data-align="center" data-file-id="42" src="/files/images/crocosmia.jpg" width="500" /></a></p>
<p>And an image above.</p>And putting a dpm(filter_dom_serialize($dom)); at the end of that function shows the output HTML:
<p>Some text and paragraphs here...</p>
<p><a href="/files/images/crocosmia.jpg">
<div class="centered-wrapper">
<img alt="" data-file-id="42" src="/files/images/crocosmia.jpg" width="500" class="align-center" />
</div>
</a></p>
<p>And an image above.</p>The <div> inside the <p> is not valid HTML and is therefore causing the issue...
ghost
commented
6 years ago I've submitted a PR that fixes this for me, however it's likely not the best solution and I had trouble with figures (I don't normally use them, and in my testing they seemed to be applied after the centering (so not sure why they're in this bit of code)).
As a comparison, here's the output HTML from my PR:
<p>Some text and paragraphs here...</p>
<p class="centered-wrapper"><a href="/files/images/crocosmia.jpg"><img alt="" data-file-id="42" src="/files/images/crocosmia.jpg" width="500" /></a></p>
<p>And an image above.</p>It adds the 'centered-wrapper' class to the nearest <p> tag instead of adding a new <div> and it removes the 'align-center' class from the <img> itself (since it's not needed and just causes the image to become a block-level element, which affects the anchor around it).
Modifications and suggestions welcome!
olafgrabienski
commented
6 years ago Changed the tags (added "needs review") to get suggestions.
quicksketch
commented
6 years ago Thanks @BWPanda for the PR! Your suggestions to the markup sound good.
We encountered the "empty paragraph tag" bug in CKEditor when we added linking to images I believe. It's been fixed in Backdrop in the original commit (https://github.com/quicksketch/backdrop/commit/bf040205d4c009942d5e04044ae0eb9985cc377e) but I made a separate issue in the Drupal queue at https://www.drupal.org/node/2876094.
We probably need a similar fix here for centering images. Apparently if the DOM is modified where the caret is, CKEditor reacts by creating an empty paragraph tag to put the caret in. Refocusing the image after centering may fix the problem.
// (Re-)focus the widget.
widget.focus();We'll also want to make the markup in CKEditor match as closely as possible. It's not a perfect match because CKEditor adds its own wrappers for resizing images, but if we're changing the front-end markup, we'll probably have to get into the CKEditor plugin too.
olafgrabienski
commented
5 years ago I've encountered a related issue which applies with and without the PR by @BWPanda: Images with a caption produce an empty paragraph above and another one below the image:
<h3>Image with caption</h3>
<p></p>
<figure class="caption caption-img">
<img alt="Dummy image" data-file-id="2" height="450" src="/files/inline-images/dummy-04.png" width="600" />
<figcaption>This is an image caption.</figcaption>
</figure>
<p></p>
<p>Lorem ipsum dolor sit amet...</p>Adding the "help wanted" tag. (I'm very interested even in a workaround because I've a site running with a paragraph counting script which doesn't produce correct results as is counts the empty paragraphs.)
klonos
commented
5 years ago Not sure if this was the case before, but in the recent 1.12 release images cannot be aligned in the center. Their captions are centered though.
Should we be supporting different aligning for images and their captions? For example a left-aligned image with a center-aligned caption, or a centre-aligned image with a right-aligned caption.
Graham-72
commented
5 years ago @klonos I tried to replicate this. Although the image was not centered in the preview, it was centered after saving.
klonos
commented
5 years ago @Graham-72 yep, I was only checking the preview; never actually attempted to save the node.
jenlampton
commented
5 years ago Should we be supporting different aligning for images and their captions?
@klonos No. let's leave that to contrib :)
Although the image was not centered in the preview
I'm pretty sure it used to be aligned in the preview, too. Either way, we should fix that too.
I'm also noticing that the PR fixes the center-alignment of images without captions, but when there are captions (and thus, <figure> tags) the alignment is still left when I request center.
We'll also want to make the markup in CKEditor match as closely as possible. ... but if we're changing the front-end markup, we'll probably have to get into the CKEditor plugin too.
In the backdropimagecaption plugin it looks like a paragraph with an image within is already returned for a centered image without a caption, so after this PR the front-end now matches the ckeditor back-end.
jenlampton
commented
5 years ago In the backdropimagecaption plugin there is a line that adds classes to the <figure> tag from editor.config.image2_captionedClass. This includes the '.caption' class which is intended for use on the <caption> tag only. When this class is added to the <figure> tag, the alignment breaks due to CSS for the <caption> tag being added to the <figure> tag.
I've removed the line that added the problematic class, and adjusted the markup changes in the previous PR to add the center-aligned class to a parent <figure> tag as well the parent <p> tag.
New PR up for review: https://github.com/backdrop/backdrop/pull/2759
quicksketch
commented
4 years ago In the backdropimagecaption plugin there is a line that adds classes to the
<figure>tag from editor.config.image2_captionedClass. This includes the '.caption' class which is intended for use on the<caption>tag only.
I don't think this is correct. That option is intended to be added to the figure tag. From the docs at https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-image2_captionedClass
A CSS class applied to the
<figure>element of a captioned image.
In our case, we use it to distinguish between a <figure> element that contains a caption and one that does not contain a caption. So I believe that this functionality should be retained.
quicksketch
commented
4 years ago I pushed up a new PR at https://github.com/backdrop/backdrop/pull/2982 that iterates on @BWPanda's PR. There were a few issues that I fixed in testing:
<p> tag would be found, causing an infinite while loop.<p> tag is generated on the fly.<p> tag is found, any classes on that <p> tag should be maintained, only with centered-wrapper added.I think this PR addresses all these issues.
olafgrabienski
commented
4 years ago I pushed up a new PR at backdrop/backdrop#2982 ...
Hm, the PR has been closed, accidentally or on purpose?
jenlampton
commented
4 years ago @olafgrabienski re-opened the PR and the markup in the sandbox looks great:
jenlampton
commented
4 years ago The code also looks good, but we may have some failing tests.
herbdool
commented
4 years ago I restarted the sandbox and all tests pass. I haven't checked the code. And looks like it needs to resolve conflicts before merging.
herbdool
commented
4 years ago Code looks good to me. Resolve conflicts with 1.x and it's ready in my opinion.
olafgrabienski
commented
4 years ago HTML code looks good in the sandbox for an centered image without a caption:
<p>Some text, and then a centered image:</p>
<p class="centered-wrapper">
<img alt="My image" data-file-id="1" height="480" src="/files/inline-images/my-image.jpg" width="640" class="align-center">
</p>
<p>More text.</p>Interestingly, around a centered image with a caption, I see two empty paragraphs in the HTML:
<p>Some text, and then a centered image:</p>
<p></p>
<figure class="caption caption-img align-center">
<img alt="My image" data-file-id="1" height="480" src="/files/inline-images/my-image.jpg" width="640">
<figcaption>This is an image caption</figcaption>
</figure>
<p></p>
<p>More text.</p>Can someone confirm the latter? Here's a test page: http://2982.backdrop.backdrop.qa.backdropcms.org/test-page
herbdool
commented
4 years ago Based on @olafgrabienski test needs more work.
jenlampton
commented
3 years ago I'm unable to reproduce the issue @olafgrabienski reported (but I do see the extra tag on the previous test page!), Here's my recent test page: http://2982.backdrop.backdrop.qa.backdropcms.org/jens-test-page
Markup looks like this:
<p>first paragraph</p>
<p class="centered-wrapper"><img alt="" data-file-id="3" src="/files/inline-images/jump.png" width="300" class="align-center" /></p>
<p>third paragraph</p>@jenlampton Thanks for having a look at the issue. The empty paragraphs only appear when you add a caption to the image. I've verified this again on a new test page. Result:
<p>First paragraph</p>
<p></p>
<figure class="caption caption-img align-center"><img alt="Dummy image" data-file-id="4" height="200" src="/files/inline-images/dummy_300.png" width="300">
<figcaption>Figure: Dummy image</figcaption>
</figure>
<p></p>
<p>Third Paragraph</p>Btw, for an image with a caption it doesn't matters if the image is centered or not.
Back to "needs work", I guess.
When I upload an image in the editor and leave 'Align' as 'None', this is the HTML that is correctly output:
However, if I change 'Align' to 'Center', this is what is (I believe, incorrectly) output:
There's an empty paragraph above the image and, since the image is now outside of the paragraph, it butts up against the following paragraph with no margin between them.
I think adding the "centered-wrapper" class to the paragraph itself should suffice. For example:
Or if not, then at least remove the empty preceding paragraph and I can style the div with margins accordingly.
PR by @BWPanda : https://github.com/backdrop/backdrop/pull/2009PR by @jenlampton: https://github.com/backdrop/backdrop/pull/2759PR by @quicksketch: https://github.com/backdrop/backdrop/pull/2982