Opened a tracker item http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32924. Should the caption function in the JHtmlBehavior class not marked as deprecated?

Thanks @laoneo :)

Probably yes, commit 801df12 restores removed call and comments it out instead.

Test works so far for the intro and full article image. It even fixes the alignment which didn't work with the caption.js.

However it doesn't work for pictures added to the article itself using the "image" button in the editor. I think this shouldn't be too hard to change so it produces correct markup here as well.

I didn't mean to uncomment the existing code as it is useless anyway. What I though it would make sense to deprecate the whole JCaption code in the JHtmlBehavior class itself. What do you think?

@Bakual No, of course it has no effect on images added via the editor and TBH I'm not totally convinced it should.

An image inserted in that way may not be a figure at all, it could simply be a decorative image so always wrapping it in figure tags would not be correct. I guess we could check to see if the caption option is set and then wrap the image in figure tags and the caption in figcaption, but if not do nothing…

We have two mechanisms to insert article images in articles, and I'm only attempting to resolve the primary one in this PR, though the editor should of course be looked at also. It's my opinion that the behaviour of caption.js is totally incorrect anyway, so disabling it is already part of the fix!

@laoneo Sorry, I misunderstood.

AWESOME! This would improve page speed sooooo much!

@nternetinspired The image button below the editor (not the one in the taskbar) has an option to set a caption. I think that one should be changed to use the figure tags if a caption is set. If no caption is set, of course only the image tag should be created. I can have a look at it probably tomorrow.

@Bakual Agreed. I'm not even sure where to start looking to make that change though.

It seems strange that the current fields on that form are 'Image description' (What the heck does that mean?), 'Image Title' and 'Caption - Yes/No'.

I think it would make more sense to only refer to the standard image tags; alt="" and title="", and have another text input for caption, to feed into . An author could, quite properly, want to set a different title="" to , e.g.:

<figure>
<img alt="a photo of Seth jet skiing" title="Me, jet skiing in the South of France!" src="">
<figcaption>If you ever get a chance to do something like this, you really should take it.</figcaption>
</figure>

Currently, it isn't possible to do that with the insert image button. I think it should be.

Aye, you are right. I think that's because of the way caption.js worked. I understand it that it took the title and put it as a caption. But I may be wrong.
I think the JavaScript code for that is somewhere in the mediamanager view. But I'll have to look myself first :)

@nternetinspired I found the place where the JavaScript does the job: https://github.com/joomla/joomla-cms/blob/master/media/media/js/popup-imagemanager.js#L121 (loaded in the
Unfortunately I suck with JavaScript and will likely need a bit more time to change that (and need to go to sleep now). So if you are as good with JS as you are with HTML/CSS, feel free to have a look
For now I would just adjust the markup based on the caption Yes/No toggle and change the whole thing in a later PR to allow for more flexibility.

‘So if you are as good with JS as you are with HTML/CSS…‘

No, unfortunately, I'm not. I also suck with javascript! :D

For those interested: #2664 changes the editor button to use semantic captions and also allows to set the caption separate from the title.

Great stuff. Thanks @Bakual this is a very welcome change.

Tested, performs as expected - all checks passed.

• Insert article images (intro and full-text) using the 'images and links' fields.
• Insert image captions using the 'images and links' Caption field, one for intro image, one for full-text.
• Set the article to 'featured' status.
• Check that the image caption appears in article, category blog and featured blog view.
  PASS
• Check that the 'Image float' option is honored in these views.
  PASS
• Check that caption.js is no longer loaded in these views.
  PASS

Chris.

PS. also posted results to the tracker.

Excellent! Thanks for testing @KISS-Web-Design

Cheers.

This is not mergeable, therefore I could not test. Is this B/C with existing articles containing images? Should not Beez be also patched once all here is solved?
Also please look at some issues we have here concerning alignment

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32935&start=0
http://joomlacode.org/gf/download/trackeritem/32935/134118/oldcaption.png/open
http://joomlacode.org/gf/download/trackeritem/32935/134119/newcaption.png/open

@infograf768 Why is it not mergeable? I just tested here and can merge with master.

Not sure what you mean by BC with articles containing images. It has no effect on articles containing images, but as previously explained it fixes the incorrect behaviour in the handling of image captions when using the images and links fields. This change means that when a user enters something in the caption field, they get a caption, previously they got a paragraph.

The screenshots posted also seem correct, as the previously broken behaviour is now fixed. Image alignment is clearly working in the shots posted. Can you explain why you think alignment is not working?

Beez is overriding com_content views, so I'm not sure what you mean by patched. Can you explain please?

The image alignment question is the same as that in PR #2664 I think. To avoid unnecessary repetition let's discuss image alignment in PR #2664 and the remaining questions, exclusive to this PR, here. Thanks.

Read above or below
"We can’t automatically merge this pull request."

Tested also with git apply:

macbookpro-2:trunkgit mac$ git apply /Users/mac/Downloads/patches16/2657.diff
error: patch failed: components/com_content/views/article/tmpl/default.php:18
error: components/com_content/views/article/tmpl/default.php: patch does not apply

The screenshots are not correct, compared to what we had before (No align meant centered). But not all is wrong as we have to also act now on the items themselves after using the image button ( see my last comment and code source on http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32935&start=0 to get the caption to be centered under the image).

"Read above or below" I don't have commit rights @infograf768, so I don't get such messages.

As I say, I can merge this branch with master without issue so I can't understand why you'd have issues. If you look at the the 'files changes tab' you'll see that the changes to this file are incredibly basic, especially round line 18.

All that is required to emulate the centred captions seen previously is a minor change in Protostar's CSS, as detailed in my comments on PR #2664 though I'd argue this would be incorrect; captions should not be centred.

@nternetinspired I think your master is outdated. After the line JHtml::_('behavior.caption'); in the article layout there should be this line which is missing in your PR:

$useDefList = ($params->get('show_modify_date') || $params->get('show_publish_date') || $params->get('show_create_date')
|| $params->get('show_hits') || $params->get('show_category') || $params->get('show_parent_category') || $params->get('show_author'));

That's the conflict.
You should try to merge (or better rebase) the upstream master into your PR branch and resolve the conflict.

"captions should not be centred."

+1

In other words, captions should styled by the template.

Best,

Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain

Sent from mobile. Please pardon any typos or brevity.
On Dec 13, 2013 3:58 PM, "Thomas Hunziker" notifications@github.com wrote:

@nternetinspired https://github.com/nternetinspired I think your master
is outdated. After the line JHtml::_('behavior.caption'); in the article
layout there should be this line which is missing in your PR:

$useDefList = ($params->get('show_modify_date') || $params->get('show_publish_date') || $params->get('show_create_date')
|| $params->get('show_hits') || $params->get('show_category') || $params->get('show_parent_category') || $params->get('show_author'));

That's the conflict.
You should try to merge (or better rebase) the upstream master into your
PR branch and resolve the conflict.

—
Reply to this email directly or view it on GitHub#2657 (comment)
.

Great spot, thank you so much Thomas for pointing me in the right direction!

I'll update my PR asap.

Cheers,

Seth Warburton
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Friday, 13 December 2013 at 20:58, Thomas Hunziker wrote:

@nternetinspired (https://github.com/nternetinspired) I think your master is outdated. After the line JHtml::('behavior.caption'); in the article layout there should be this line which is missing in your PR:
$useDefList = ($params->get('showmodify_date') || $params->get('show_publish_date') || $params->get('show_create_date') || $params->get('show_hits') || $params->get('show_category') || $params->get('show_parent_category') || $params->get('show_author'));

That's the conflict.
You should try to merge (or better rebase) the upstream master into your PR branch and resolve the conflict.

—
Reply to this email directly or view it on GitHub (#2657 (comment)).

@nternetinspired
Also, "Beez is overriding com_content views, so I'm not sure what you mean by patched. Can you explain please?"
When we change code in core (which can be tested in Protostar), we should not forget to also change in Beez overrides is what I meant... Same when we change something in Isis, we should try to change in Hathor, although I know some people do not care.

Tue, it needs to be changed in /templates/beez3/html/com_content/.. as well.

Also, this is not B/C (I manually patched the file concerned)

(The mail cloaking error in the capture is NOT related, was due to unrelated former Tiny tests)

Before patch capture:

After patch

@infograf768 The right floating of the image after the patch is actually correct. The problem here is a bug in our current implementation with caption.js. As soon as you set a caption, the image will always be centered and ignores the "image float" setting. You can test this if you remove the caption, then the image will be floated to the rigth as well.

The above captures only concerned images added via images intros or full (for which former implementation was buggy indeed).

Here are the results obtained on a LEGACY article containing images with caption in their body if caption.js is not loaded anymore. We definitely are not B/C...

Before taking off JHtml::_('behavior.caption');

After taking off JHtml::_('behavior.caption');

This breakage is acceptable for 4.0, not in the 3.x series imho

Ah, my master was a little out of date! Updated now and removed commented code sections.

I've elaborated on my reasoning on the formatting of captions and images, for core outputs, in PR #2664.

I didn't modify template code as I believe it is the prerogative of the template to style such elements, or not. I do care, I just don't think it is the correct course of action.

If the Beez or Hathor authors have decided to override core outputs then so be it, I don't see a good reason to change template overrides; that would defeat their purpose. If templates were all kept in sync with core output, they would all be the same; again defeating the purpose of the template.

Cheers,

Seth

@nternetinspired PR is mergable again, thanks.

JM makes a valid point about BC for existing embedded images. Thus we may have to keep caption.js in for now to not break anything. Would that be ok with you?
The unneeded loading of Mootools can be solved with #2658, which leaves us with the small caption.js script. I would love to get rid of that as well but looks like it's something for the next minor or major release to remove that.
If we can remove the need for it now, that would still be helpful for when we are going to finally remove it.

Means this PR will come in 4.0 and not earlier?

If we don't remove caption.js, it should be no problem to merge it for 3.2.2 (I think).
If we remove caption.js, it may be that it has to be delayed due to the (small) BC break at least for the next minor release. I would have to ask PLT, especially @mbabker about this

I can see your point, but it is is where we differ on opinion.

I see the function of caption.js to be fundamentally incorrect so I view its removal as a fix in itself, even without the addition of semantic markup. The removal of a flaw should not be seen as a break in backward compatibility.

@nternetinspired I actually agree with you, but not all do. And maybe we have to make a compromise here

What could be done is to add a parameter in Configuration. By default it would keep caption.js. I personally have sites with dozains of images centered with centered captions... And I guess I am not alone.

As we have seen before adding options to the configuration is not a valid
solution for people updating their sites as we currently don't have a
method to add the value during the update
On 17 Dec 2013 07:33, "infograf768" notifications@github.com wrote:

What could be done is to add a parameter in Configuration. By default it
would keep caption.js. I personally have sites with dozains of images
centered with centered captions... And I guess I ma not alone.

—
Reply to this email directly or view it on GitHub#2657 (comment)
.

I would also not overload the configuration with options which are hard to understand. There are many enough at the moment.

+1 for not creating additional configuration options.

Existing article images don't have captions they have images with title tags and after these changes images will still have title tags (if they have been set).

These changes ensure that when users choose captions, they get captions; an improvement. Removing caption.js ensures previous users who chose captions don't get something that isn't a caption; a fix.

Existing article images don't have captions they have images with title tags and after these changes images will still have title tags (if they have been set).

While technically true, together with caption.js and the 'caption' class they got a caption instead of the title. It was the intended behavior and removing this is indeed a BC break.

As we have seen before adding options to the configuration is not a valid solution for people updating their sites as we currently don't have a method to add the value during the update

If done right, that's actually easy. The problem is only that not all people understand how to do it properly

I think adding an option to com_content is the only way to get this done soon.

@nternetinspired I can do a PR to add this option to com_content, making caption.js optional. I would do it so that new installations have it disabled by default and updating users have it enabled. But you need to update your PR so captions.js stays in.

@Bakual I would not add options for something little like this. Nobody will understand it. And this is not only com_content related. Every component which uses the editor field is affected.

While technically true, together with caption.js and the 'caption' class they got a caption instead of the title. It was the intended behavior and removing this is indeed a BC break.

No, because they never got a caption they got a new paragraph. That wasn't what was offered, what was correct or what was sensible; it was a flaw. It doesn't make any sense to me to not fix flaws.

No, because they never got a caption they got a new paragraph. That wasn't what was offered, what was correct or what was sensible; it was a flaw. It doesn't make any sense to me to not fix flaws.

For the user, it doesn't matter. He got a text below his image which looks like a caption. And if we remove caption.js, this is no longer. Thus we broke it. Even if we fixed it really.

@laoneo We are only talking about removing the caption.js call in com_content. So only com_content is affected. Other extensions which used the editor either do load caption.js themself, or the caption stuff just never worked for them.

Updated to add back the call to caption.js, in order to continue providing faux-captions for images in existing articles.

Added a comment that these should be removed in j4, when it should be OK to break BC on legacy content.

After patch, we still have a B/C issue for people who used protostar or no overrides, specially when Float None was chosen.
Before patch:

After patch

I solved the issue here by adding this css

.pull-none {
margin-left: auto;
margin-right: auto;
text-align: center;
}

In a LTR document all content should default to text-align: left; the universal browser default.

a .pull-none class is nonsensical. It should be noted also that Bootstrap doesn't contain a .pull-none class.

Based on the BC issues referenced here and on the tracker I am closing this. Thanks for working on this so far.

The only valid BC issues referenced in this thread were catered for.

Centering text, for no good reason, within a LTR document is not only a very poor typographical treatment, it is against the browser default rendering and against all common sense. Not centering that text by default is the right thing to do and causes no BC issues.

It appears the BC issues were resolved. Why close this Brian?

Section 6.1.7. of the Joomla Development Strategy clearly states that rendered markup is not subject to the backwards compatibility promise.

http://developer.joomla.org/news/586-joomla-development-strategy.html

@brianteeman Can you please elaborate on your reasons for closing this PR?

While I did argue in favor of including rendered markup in our backwards compatibility promise, @nternetinspired is correct that it is not included. It's also worth adding that we also state:

We will try not to change markup in such a way that a site might render differently, but we can't promise not to break anything at the present time.

From my review of this issue and testing, the only potential issues are that captions are no longer centered and that when Image Float is set to None, images are no longer centered.

In my opinion, not centering captions is not a deal breaker, and the correct way to handle it.

I also feel that the previous centering of images, when Image Float was set to None, was not the best way to implement that feature. But, that issue is limited to only those using Protostar as their default site template. The likelihood of Protostar being used for a production site is minimal and if it continues to be a pressing issue to get this PR merged, I'd propose a Centered option be added so that any affected sites can choose that option.

I am reopening this PR as I do feel that it is still valid.

Hi @nternetinspired @betweenbrain

is this issue still valid? As the last comment here was on 1 Aug 2014? Also pull requests to the master branch of this repo are not longer accepted. Can you close this pull request and submit a new one against the staging branch, if this issue still exists?
If there is no Response in a month we will close the PR here.

Thanks for understanding!

Yes the issues still exist;

1. We call things that aren't captions “captions”,
2. We center them when alignment>none is chosen,
3. We use javascript to do something that can be done with a single html tag.

I call bull on all three.

Tested on introimage and Full Article Image in Images and Links Tab:
Failed:

• The output of the caption is not in "figcaption" tags but in paragraphs.
• caption.js is not removed by the patch.

Works: When i insert an image with captions in the editor it works.

@designbengel Did you apply the patch? If you did, and you insert an image with a caption it will be inserted with <figcaption>. The caption inside <p> tags is the existing behaviour which this PR attempts to correct. This patch does not remove caption.js

@nternetinspired Yes i applied the patch. It works for images which are inserted with editor in the content. But when i insert Intro or Fulltext image in the Images and Links tab of an article it does not put the caption into but in

tags.

It works for images which are inserted with editor in the content.

That has nothing to do with this PR, that is native editor functionality and not changed by this PR.

But when i insert Intro or Fulltext image in the Images and Links tab of an article it does not put the caption

Are you checking with Protostar? What you describe sounds like this PR is not applied at all.

@nternetinspired Could you please make this PR against staging? I also think that @designbengel may not have applied the PR because it is against master.

Thanks Seth.

New PR against staging: #7450 Closing.

@nternetinspired can i get you to review the new PR? And others on test?