#18311 closed task (blessed) (fixed)
Support HTML in image captions.
Reported by: | prettyboymp | Owned by: | sushkov |
---|---|---|---|
Milestone: | 3.4 | Priority: | highest omg bbq |
Severity: | blocker | Version: | |
Component: | Editor | Keywords: | has-patch |
Focuses: | Cc: |
Description (last modified by )
The ability to add any HTML formatting to text in captions is very limited.
- HTML markup can be added from the HTML editor as long as you don't switch to the visual editor and the markup does not contain any quotes.
- Currently the visual editor removes any HTML tags from captions.
To do:
- Handle HTML in the caption text escaping/encoding it to pass through the shortcode matching regex.
- Provide some way of adding HTML in the captions in the gallery -> image properties. (This may be limited to links only as they seem most needed).
- Improve editing of caption text in the visual editor after inserting an image with caption or add third "popup button" to images that would add/remove captions directly without needing to open the uploader/gallery popup.
Attachments (14)
Change History (105)
#3
@
13 years ago
- Component changed from Media to TinyMCE
This is related to how the wpeditimage plugin for TinyMCE currently works. It was originally written to support images, not other "stuff" in the media library or a generic for other things. I use it as a container for the flowplayer shorttag.
The first half of the problem is to stop the plugin from stripping the caption tag unnecessarily when switching from HTML to Visual mode.
"my-patch.diff" fixes that part of the issue.
#4
@
13 years ago
per http://konstruktors.com/blog/wordpress/3203-how-to-automatically-add-image-credit-or-source-url-to-photo-captions-in-wordpress/ the source URL might be a good alternative, though it borders on 'too many options'
#6
@
13 years ago
I don't think the source_url method is the way to go. Definitely adds an unnecessarily confusing element to the UI and the whole system. I also don't think that just adding attribution in brackets like that is nearly universal enough.
What we need is to stop the caption system from stripping out light HTML from the caption field. There's no semantic justification for not having formatting and in fact there are MANY scenarios where having it is important, not the least of which is the attribution scenario which is vital for legal reasons.
WP IDEA with 55 supporters: http://wordpress.org/extend/ideas/topic/fix-the-caption-system-to-allow-links-inside-captions
WP Tavern Post about it: http://www.wptavern.com/how-to-insert-links-inside-of-image-captions
#7
@
13 years ago
Someone just needs to write the patch.
It's not a lack of support that is holding us back, it's the lack of code to implement this.
#8
@
13 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#9
@
13 years ago
- Cc jer@… added
The first step of the problem is in image_add_caption()
, which is attached to the image_send_to_editor
filter and handles actually inserting the caption shortcode into post content. Notably, the image_add_caption()
function is completely undocumented ( {@internal Missing Short Description}}) so it's hard to know what was intended by it's behavior, though clearly the idea was to simply remove anything that might be offensive inside a shortcode attribute without consideration for formatting that might be important to preserve.
Interestingly the upload manager popup correctly saves whatever HTML you insert in the caption without stripping it or showing an error. The problem is that when the shortcode is inserted into the post, image_add_caption()
runs the following string replacement, which completely neutralises any HTML in the caption by replacing it with HTML entities:
$caption = str_replace( array( '>', '<', '"', "'" ), array( '>', '<', '"', ''' ), $caption );
This is despite the fact that re-opening the image in the gallery shows you that the HTML is still saved correctly and unmolested in the caption field, which is very confusing. At a minimum the media uploader should not save any HTML in the caption field that will not ultimately function in the caption shortcode itself.
If the str_replace is removed then inserted HTML continues to not function because when the actual $caption content is output, addslashes() is run on it first( addslashes($caption)
) which creates useless broken HTML.
The issue is further complicated by the fact that the caption text is stored in the caption=""
attribute of the shortcode, so any double-quotes within the caption text truly don't make any sense and absolutely SHOULD be either escaped/slashed or replaced with single-quotes for storage inside the shortcode attribute.
THE CAPTION SHORTCODE IS FUNDAMENTALLY FLAWED, NUTS
Looking at it closely it seems like the architecture of the caption shortcode is fundamentally flawed because it uses an 'attribute' of the shortcode (caption="") to store the caption text, and attributes are rightly blocked from having the kind of formatting a useful caption needs. Since the shortcode has only one piece of content that can go between the two tags (i.e. between [caption] and caption) that content should have been the caption text itself (which could then take advantage of being regular post content), rather than the image html tag that is currently there (which could have been broken down into esc_attr()-safe attributes stored inside the initial [caption] shortcode and parsed into an img tag during shortcode execution).
Unfortunately re-architecting the caption shortcode now is probably off the table, and I doubt we can get traction for fully replacing it with a new more powerful shortcode in core. That is the problem we face though, the current architecture is just wrong for captions that are any more than another kind of alt label.
WE NEED SOME WAY TO ESCAPE HTML IN THE CAPTION TEXT ON SAVE, THEN UNESCAPE ON DISPLAY
So the trick we need to pull off is getting the system to somehow allow for at least a limited set of HTML (personally I'd settle for just links) inside the 'caption' attribute of the [caption] shortcode, then making the shortcode function img_caption_shortcode()
display it correctly. If that worked, then it should be possible to get the visual editor to stop messing it up when you switch back and forth.
To me it seems like the best approach within the current shortcode would be to carefully account for particular HTML tags inside the 'caption' attribute by escaping them in a particular way while inserting them into the post, then when executing the shortcode carefully accounting for the escaped HTML and reverting it into an active state.
USING ADDSLASHES()/STRIPSLASHES() TO ESCAPE CAPTION TEXT: FAIL
The simplest version of that idea is to just remove the anti-html str_replace (quoted above) from image_add_caption()
and instead depend on addslashes() to keep the content safe, then, in {{img_caption_shortcode()}} using stripslashes() to get the content back to it's original state.
Unfortunately we can't just stripslashes() in the current state of the codebase because a slash-escaped link tag inside the caption attribute causes the whole shortcode's $attr array to lose it's lunch and become broken, here's and example where the caption was "Testing a link" and the link was an a tag leading to test.com which used double-quotes around the URL:
[id] => attachment_74823 [align] => alignnone [width] => 375 [0] => caption="Testing [1] => a [2] => href="http://test.com">link"
That's the $attr array recieved by the shortcode function img_caption_shortcode(), so before we even have a chance to stripslashes($caption) the shortcode fails completely because the $caption attribute is mandatory for the shortcode and not present in $atts.
The culprit here is shortcode_parse_atts()
which uses some intense regex voodoo I can't even begin to understand but is clearly not ready for a world where slash-escaped double-quotes are part of a shortcode attribute.
However, in this case switching to single-quotes actually fixes the problem, and results in the link showing correctly on the image. Maybe a final solution will involve enforcing single-quotes -only within caption text HTML, though that seems awkward, since it will be hard to distinguish between a double-quote used as part of a link and one used as part of regular text, and we don't want to replace text-quotes with a single-quote as that would be confusing for authors.
USING URLENCODE()/URLDECODE() TO ESCAPE CAPTION TEXT: UGLY
An alternate idea would be to use another escaping system entirely, to completely neutralize the caption text before sending it to the editor, then re-activate it when parsing the shortcode on display. I tried replacing addslashes/stripshashes with urlencode/urldecode and the results were very promising. In image_add_caption()
I use urlencode($caption) before sending it to the editor, which sends things like this to the post:
caption="TEsting+a+%3Ca+href%3D%22http%3A%2F%2Ftest.com%22%3Elink%3C%2Fa%3E+with+also+some+%3Cstrong%3Ebold%3C%2Fstrong%3E+text.+"
Then in img_caption_shortcode()
I run $caption = urldecode($caption), which reverts it back to it's original state and shows perfectly fine on the post preview.
This even avoids having the visual editor break the content when switching back and forth, but unfortunately it also looks AWFUL in the visual editor, because the urlencode() version of the caption is shown, which would probably confuse even users savvy enough to be adding HTML links in the caption field in the first place. This same awfulness applies to trying to read or edit the caption inside the HTML editor, which is infinitely more difficult then when the caption text is shown as plain text. Yet another place where the ugly urlencoded version shows is in the visual editor when you edit image settings.
WHAT OTHER FORMAT MIGHT WORK?
It seems like a big problem here is that any attempt to escape the HTML will need us to account for every single place where it is shown to users and unescape it for display in that space. Otherwise the escaping pattern would have to be simple/obvious enough that looking at the escaped version still made it clear what the HTML was for.
Maybe the solution is some kind of non-HTML code similar to shortcodes that could be used inside the caption attribute to identify links etc. This is not ideal as it creates yet another layer of syntax to learn and I would avoid that if at all possible, but it may be the only way to get around the problematic architecture that the caption shortcode currently suffers from. Maybe something that emulates mediawiki markup like this could work?
[http://test.com/ test]
OOF! THERE IS A REASON THIS HAS GONE SO LONG WITHOUT A FIX
Okay enough from me for today. Just wanted to dig into the code and do some research about the problem. I think that this probably needs more than just a patch, it needs thinking and decisions because so far there's still no solution that seems appropriate despite the obviousness of the need.
#10
@
13 years ago
- Cc kpayne@… added
- Keywords has-patch added
Perhaps something like 18311.2.patch would work?
This allows simple HTML (currently strong
, em
, and a
) in the [caption]
shortcode. It has to be turned on via the render_html=true
attribute. It works with no modification to any other code, contained completely within img_caption_shortcode
. It doesn't matter what encoded entities TinyMCE throws at it because it will decode it, keep good html, then re-encode it to sanitize bad html.
#12
follow-up:
↓ 13
@
13 years ago
We need a more general approach both in the visual editor and in parsing the shortcode. Also this could involve an UI change (How to add a link in the caption in the image properties popup?) or explicitly moving editing of captions to the visual editor after the image has been inserted.
#13
in reply to:
↑ 12
;
follow-up:
↓ 15
@
13 years ago
- Cc xoodrew@… added
Replying to azaozz:
We need a more general approach both in the visual editor and in parsing the shortcode. Also this could involve an UI change (How to add a link in the caption in the image properties popup?) or explicitly moving editing of captions to the visual editor after the image has been inserted.
Would moving the editing of captions to the visual editor affect the pulling of EXIF data (such as a caption) from those images?
#14
follow-up:
↓ 16
@
13 years ago
- Cc sushkov added
@jeremyclarke has a point when says that image caption content should go between [caption] shortcode tags.
A fix could be achieved with minimum backcompat impact by separating the [caption] shortcode from img
tag. To explain, we could move the [caption] before (makes more sense to me, or after) the img
tag, so when the parser meets it, the context is not lost.
On the javascript side, that would mean little changes too.
Example:
[caption stuff=""]My awesome caption[/caption]<img src="" alt=""/>
The good part here, is that basically we do no make changes in the shortcode api (no overhead like mustaches {{}}
) we just change the location of it inside the post_content
.
#15
in reply to:
↑ 13
@
13 years ago
Replying to DrewAPicture:
Would moving the editing of captions to the visual editor affect the pulling of EXIF data (such as a caption) from those images?
No, we "read" the EXIF data in PHP after the image is uploaded and store it as meta. This is another enhancement (that has a ticket I think): using all relevant EXIF from the images and rotating them when needed after uploading and before making smaller sizes.
#16
in reply to:
↑ 14
@
13 years ago
- Component changed from TinyMCE to General
- Description modified (diff)
- Keywords has-patch removed
- Milestone changed from Future Release to 3.4
- Owner set to sushkov
- Status changed from new to assigned
- Summary changed from Caption shortcode doesn't support markup in the caption. to Support HTML in image captions.
- Type changed from enhancement to task (blessed)
Replying to sushkov:
@jeremyclarke has a point when says that image caption content should go between [caption] shortcode tags.
Don't think changing the structure of the image caption shortcode is possible. This would break all plugins and themes that use it directly (we explicitly allow that in img_caption_shortcode()
with the 'img_caption_shortcode'
filter).
I rather see that as escaping all quotes or encoding the html string in caption="...", whichever works better. If we need to encode it, we can decode before running the 'img_caption_shortcode'
filter to keep it back-compat.
#17
@
13 years ago
- Keywords ux-feedback added
How would moving the ability to edit captions/attachment meta into the Visual Editor affect UX when working outside the post/page editor in Media > Library/Add New? My main concern is that the Visual Editor is not the only place where images (and their metadata) are used.
#18
follow-up:
↓ 19
@
13 years ago
I generally think of captions to be post/display-specific, while the library is just a file archive.
#19
in reply to:
↑ 18
;
follow-up:
↓ 20
@
13 years ago
Replying to jane:
I generally think of captions to be post/display-specific, while the library is just a file archive.
Right, but you should still be able to edit the metadata (including the caption) even if it's just an archive.
#20
in reply to:
↑ 19
;
follow-ups:
↓ 21
↓ 27
↓ 32
@
13 years ago
Replying to DrewAPicture:
Right, but you should still be able to edit the metadata (including the caption) even if it's just an archive.
Maybe maybe not. If the metadata is use-specific, not file-specific, that would not be true. Linking to the use-specific editor makes sense, but say you have a logo or a headshot of someone that you caption differently every week. The file archive should have the single headshot, not hundreds of captions you have to wade through to find a file.
#21
in reply to:
↑ 20
@
13 years ago
Replying to jane:
Replying to DrewAPicture:
Right, but you should still be able to edit the metadata (including the caption) even if it's just an archive.
Maybe maybe not.
That's kind of my point. I think it would behoove us to take into account a wider use-case for media, especially with a lot of the newer extensibility being tied to WordPress (App themes, CPTs, etc.). WordPress isn't just about blogging anymore, which means it's also not just about creating content in the editor. It seems counter intuitive to remove an option from where it always lives (media library) and move it to a place where it sometimes lives (visual editor).
Now, I understand the reasoning for moving it into the editor (shortcode parsing and all of that), but I don't think this is the right course to take.
#22
@
13 years ago
Related plugin? RPS Attach Caption Expand
#23
follow-up:
↓ 25
@
13 years ago
I'm playing around with some escaping options, and the most elegant (chances are that it is not the most supported) solution is to use JSON.stringify()
and JSON.parse()
.
JSON
is a native JSON encoding/decoding browser feature with IE6> cross-browser support:
http://en.wikipedia.org/wiki/JSON#Native_encoding_and_decoding_in_browsers
Other ideas include encodeURIComponent()
on JS side and urldecode()
. We can't use escape()
since there's no straight alternative to decode escaped strings on the PHP side (try this with languages like Tamil http://en.wikipedia.org/wiki/Tamil_language)
The biggest problem though, is that we will need to filter what tags are supported in the caption, where I would stick to basic span,a,em,strong,del,[insert markup tags here]
(could not find a reason why would someone insert a <table>
in a caption). For this we will definitely need a textarea instead of existing input type field and a UI markup editor, which is an unanswered question atm (both MCE and quicktags do not fit, unless we leave textarea use plain HTML).
Need some dev feedback.
#25
in reply to:
↑ 23
@
13 years ago
Replying to sushkov:
I'm playing around with some escaping options...
Yes, JSON seems a good encoding solution for that. Was thinking we could try escapting only, i.e. escape the single and double quotes \'
, \"
and then unescape them after the shortcode is processed.
The biggest problem though, is that we will need to filter what tags are supported in the caption, where I would stick to basic
span,a,em,strong,del,[insert markup tags here]
...
When the visual editor is used it will allow only inline tags there (or the caption format breaks). When adding tags directly, it will be the UI that determines how we handle that. For now I imagine we would only support adding links directly (outside the visual editor) and of course entering HTML by hand. In the second case we should expect the users to "know what they are doing", same as when using the HTML editor.
Need some dev feedback.
You are one of the devs for this enhancement. That includes researching, testing and deciding what would be best there (you have the power!) :)
#26
follow-up:
↓ 28
@
13 years ago
Added a diff that makes current editor allowing HTML. Though I still have to add some filtering on what HTML is allowed to be inserted.
File is not minimized!
#27
in reply to:
↑ 20
@
13 years ago
Replying to jane:
Replying to DrewAPicture:
Right, but you should still be able to edit the metadata (including the caption) even if it's just an archive.
Maybe maybe not. If the metadata is use-specific, not file-specific, that would not be true. Linking to the use-specific editor makes sense, but say you have a logo or a headshot of someone that you caption differently every week. The file archive should have the single headshot, not hundreds of captions you have to wade through to find a file.
Chiming in on this relatively old question, it's worth parsing out what each of these fields is used for in interpreting their relevance.
The caption is in fact used beyond the insertion of images into posts. At least in twenty-ten and twenty-eleven, if you use the [gallery] shortcode then on the thumbnail-list of images the text below each image is the caption as saved in the metadata. The caption is also used (albeit a bit strangely) on the single-image page when you click the thumbnail, between the image and the 'description' which is shown below.
Notably, any subsequent insertions of the image into that [gallery] post or in fact any other post (using the 'media library' tab in the uploader of other posts), will overwrite the caption text on the image, and thus change the behavior of the gallery. So if you carefully assemble a [gallery] with useful captions for each image in the context of the gallery then you need to never edit any of the captions on those images in the future or your gallery will become confusing. I actually didn't know that it would do this. I thought that the captions wouldn't be used in [gallery] but tested to make sure, glad I did!
To me the captions SHOULD be transient and insertion-specific, as Jane described. It's a logical thing to expect, especially compared to the 'description' and 'title' fields, which make much more sense as being official and linked to the image rather than the use. I'm pretty sure that the Link option actually does work like this, and isn't used anywhere other than in image insertions that happen while that link is visible in the field.
Despite what I might want though, captions are in fact a permanent part of each image attachment and thus on some level (at least for anyone who uses [gallery]) should be treated as directly linked with the image and permanent.
All that said, it doesn't really affect this ticket significantly unless we're seriously considering using the db-stored metadata to generate the [caption] shortcode at load time. As long as we stick with the current paradigm where the caption metadata parts ways with the caption text for a particular insertion at insertion time then we are fine. As an example, we should think about [gallery] and the stuff above if we are considering making the Visual Editor image settings editor capapable of editing the db-based metadata.
WHOOSH. I'm going to test the actual patch now.
#28
in reply to:
↑ 26
;
follow-up:
↓ 29
@
13 years ago
Replying to sushkov:
Added a diff that makes current editor allowing HTML. Though I still have to add some filtering on what HTML is allowed to be inserted.
File is not minimized!
Just tried this and it is a good partial solution! It does make links work if inserted with the media uploader, and gives a advanced-code-based solution for adding/editing links in captions from the visual and HTML editors.
Is there something unusual about this patch though? I tried patch -p0 FILENAME.diff
and it totally didn't work. I just went in and aplied them by hand.
Anyone trying this patch should remember to set the script debug constants so your install uses the dev JS files which are modified by the patch:
Here's a rundown of things I tried and the results:
- Insert caption w/ link into post with uploader: works naturally. Links can be added with single or double quotes. Re-opening the media library and editing the link then re-inserting also works perfectly and is simple.
- Switch back and forth between Visual and HTML: works, doesn't break the results of insert.
- Visible in TinyMCE: Yes but confusing. Shows HTML code in caption box because the < and > tags are actually <, >, so the tags look like raw HTML when displayed. (see screenshot below)
- Edit caption+link directly in TinyMCE: Works but awkward because you have to write HTML in the Visual editor. If you just type in < and > then they get saved as < which then gets the same treatment as links inserted by the media uploader.
- Edit caption+link in "edit image" popup of TinyMCE: Works partially - link can be edited but "caption" field is populated with the entity-encoded version of the caption, which normal users and even HTML-aware-but-not-expert users will find very confusing to work with. (see screenshot below)
- Editable in HTML editor: works but awkward - same problem as in Edit Image popup with entity-encoded HTML instead of ", ', < and >. Makes it very hard to edit. Obviously things like > are more reasonable in this context than TinyMCE.
- Link works when viewing post/gallery: yes, looks right and behaves as expected.
Personally I would be satisfied with the compromise that in HTML editor links-in-captions show up entity-encoded with >, &39; etc. in their raw form. People using the HTML editor who really know HTML will understand what's happening and other types of users can be taught to simply delete and re-insert captions using the media uploader if they get something wrong. It even has decent WRITE-ability given that you can copy/paste from one caption into another to create links in captions by hand, and maybe some freaks will enjoy typing out &39;'s themselves.
So the main thing this patch lacks is elegance when used inside the Visual editor. Having the links show up in the preview as raw HTML is probably not acceptable, and having them show up as HTML-encoded in the 'Edit Image' popup is definitely not acceptable.
Making caption-HTML show up as HTML in the Edit Image popup
Screenshot of the caption field from the Edit Image popup inside the Visual Editor:
This strikes me as the easier problem. We need to filter the existing caption to entity-decode it as much as possible before sending it to the popup, and upon saving the popup we need to run the same encoding procedure used when inserting an image from the uploader popup. I took at stab at doing this and ended up on editimage.dev.js
but I just can't figure out what does what.
It seems that when the Edit Image popup loads, it already has something to convert &39; to ' and " to " because those show up normally. The problem in the popup is the < and > so wherever the quote un-encoding happens hopefully it won't be too hard to account for those as well.
Sushkov or Azaozz, do you know where this happens? What do you think is the best solution?
Showing HTML in captions properly in the WYSIYWG preview
This is a lot harder, and I don't even know where to start. Here's how it looks now:
http://simianuprising.com/wp-content/uploads/2012/02/tinyMCE-link-in-caption-screenshot1.png
This is very logical behavior because the WYSIWYG is just showing us what the HTML would look like when viewed on a post. The thing is that when we actually show it on a post, the caption content will be run through the html_entity_decode()
first, making the link active instead of encoded, so ideally we should incorporate that logic into the behavior of the editor, just like the shortcode is already parsed out to have the grey box.
What we need is to get TinyMCE, in whatever place it stores it's impression of the markup, to use a html_entity_decode()
-style version of the caption text rather than the encoded version that is actually saved. Anyone have any idea how to make that happen? Is this likely to even be possible?
One move that might simplify the problem would be to completely lock editing of the caption in the WYSIWYG, and force people to use the Edit Image popup to do so. It would be annoying in some cases but it would also be clear and simple and probably not a huge pain most of the time. Really captions are very complicated and even without links, editing them inline causes weird results (what happened when I tried to delete a caption+image by starting with the caption was not at all what I expected). Again I have no idea what would be involved in taking this route, so anyone who knows please speak up.
Worst case scenario I'd accept if the answer is just "it looks wierd in the Visual editor if you have links in captions". It's still better than being unable to create links at all.
Thanks for reading, it will be great to have this issue resolved
Sushkov, please let me know if you think I got something wrong about your patch and what it does.
Others, please test it too!
#29
in reply to:
↑ 28
@
13 years ago
- Component changed from General to Editor
Replying to jeremyclarke:
Sorry for patch, my mistake. Should be fixed now.
Please make sure you are testing only after you replace the {{editor_plugin.js}} with the patched {{editor_plugin.dev.js}}.
Afaik, at this moment this issue needs dev-feedback and an UI decision.
Thanks.
#31
follow-up:
↓ 33
@
13 years ago
Been working on a patch for our 'wpeditimage' TinyMCE plugin for the last week or so. Don't think showing the raw HTML in the visual editor is acceptable (it is a visual editor). That would also prevent the user from editing or inserting any HTML there using MCE which is the main purpose if this improvement.
The problem is that whatever works in the visual editor looks pretty bad in the HTML editor and vice versa. Having a string with htmlspecialchars
as entities in the HTML is highly undesirable and a lot harder to edit in the HTML editor.
The improvement on the TinyMCE side currently pulls all image captions "constructs" out of the DOM, serializes the nodes into HTML strings, creates the caption shortcode and then adds it back after the whole content has been serialized. Working on the DOM nodes instead of post-processing the HTML code with regex ensures that all tags in the captions are captured correctly.
As far as parsing the caption shortcode in PHP is concerned it seems to work well when using single quotes in the HTML in the caption attribute.
#32
in reply to:
↑ 20
@
13 years ago
Replying to jane:
Replying to DrewAPicture:
Right, but you should still be able to edit the metadata (including the caption)...
Maybe maybe not. If the metadata is use-specific, not file-specific, that would not be true...
Agree with Jane. Perhaps some of the confusion comes from the fact that we save the "caption" entered in the image properties popup into the attachment post's excerpt (and the image description is saved in post_content). Then the excerpt is shown in the default gallery and on individual image (attachment) pages, and is inserted together with the image in new posts/pages.
Perhaps we should rename the caption field in the image properties popup to "Default Caption" (as it is used as default in new posts).
#33
in reply to:
↑ 31
;
follow-up:
↓ 36
@
13 years ago
Replying to azaozz:
That would also prevent the user from editing or inserting any HTML there using MCE which is the main purpose if this improvement.
FWIW I think the main purpose of this improvement is to make it possible to add links in captions, not that it be easy to do so using the WYSIWYG tools. The bug is that it's impossible, whereas making it easy would be a more of a new feature request (just like it would be a feature request to give a way to add arbitrary CSS classes to HTML tags in the Visual editor, unlike the ability to do so in the HTML editor without it being destroyed when switching to Visual, which we already have).
The problem is that whatever works in the visual editor looks pretty bad in the HTML editor and vice versa. Having a string with
htmlspecialchars
as entities in the HTML is highly undesirable and a lot harder to edit in the HTML editor.
If we can make it work in the Visual editor then I think having some encoded characters in the HTML editor is harder but not insanely so. [caption] shortcodes are already hard to look at, this won't make it that much worse.
As far as parsing the caption shortcode in PHP is concerned it seems to work well when using single quotes in the HTML in the caption attribute.
Asking users to "only use single quotes" strikes me as unlikely to be successful and it also assumes the use of double-quotes throughout the shortcode, which is how it gets generated by the uploader UI, but not at all a prerequisite of the shortcode otherwise. In theory this could be a partial solution if we could stop the Visual editor from destroying such links (currently any links using single-quotes, as well as any subsequent text in the caption is removed when you switch to visual).
I think on some level despite the fact that &39; is confusing it would also make it clear that "this is complicated and not just plain HTML". Using single quotes would make it look like any html could go in there, but if people wrote html that seemed the same but with double-quotes it would have very unpredictable effects (e.g, looking and working fine but getting destroyed if you ever switch to Visual editor).
It's great to hear you're also working on the wpeditimage plugin. I'm eager to try any patches you have that affect this situation. Thanks!
#35
@
13 years ago
First run. Note that 18311-3.patch is for testing only, it replaces the minified wpeditimage/editor_plugin.js with the non-minified version to make it easier to test (still requires SCRIPT_DEBUG to be defined).
The patch doesn't include changes to the Edit Image popup in the visual editor or the image properties in the gallery (the two places where image captions can be typed in a text field).
#36
in reply to:
↑ 33
@
13 years ago
Replying to jeremyclarke:
Asking users to "only use single quotes" strikes me as unlikely to be successful...
You mean when the users type the whole caption shortcode in the HTML editor by hand, including HTML tags in the caption="..."
? Doubt it anybody would do that. It's much easier to insert a shortcode then edit it in which case the user would simply follow the already set format (double quotes for shortcode attribs, single quotes for HTML in the caption="..."
). Keep in mind the people that use the HTML editor "know what they are doing".
The alternative is to change the format of the shortcode. This would likely break some plugins that manipulate it directly in the DB or do other hacky things with it. Of course we can keep the 'img_caption_shortcode'
filter exactly the same but plugins that are "doing_it_wrong" and not using it would probably break.
...if we could stop the Visual editor from destroying such links (currently any links using single-quotes, as well as any subsequent text in the caption is removed when you switch to visual).
See the patch.
It's great to hear you're also working on the wpeditimage plugin. I'm eager to try any patches you have that affect this situation.
Yes, captions in the visual editor are handled there.
#37
@
13 years ago
Here's another approach we were proposing during last dev-chat, for handling caption text.
Right now the shortcode looks like this:
[caption id="attachment_7" align="alignleft" width="300" caption="Caption Text"]<img src="pic.jpg" width="300" height="199" />[/caption]
Because changes to handle captions inside an attribute will result in less simple and clear code, we can move the caption text into its own shortcode:
[caption id="attachment_7" align="alignleft" width="300" caption="ceva"]<img src="pic.jpg" width="300" height="199" />[caption_text]Caption Text[/caption_text][/caption]
This basically saves us from handling input and escaping data from an attribute, and leaves space for all kind of formatting.
In fact, looking back, we can solve this simply by regexp-ing img
tag and considering the rest caption text
(problem here is that themes that hook into img_caption_shortcode
will have no support for the new version of captions text).
This snippet more or less shows the current shortcode version changes it will require:
https://gist.github.com/1847575
The point of the above snippet, is to prove that backcompat can be preserved.
Once again, changes on the JS side, should be less and cleaner with an approach like this.
#38
follow-up:
↓ 41
@
13 years ago
In fact, looking back, we can solve this simply by regexp-ing img tag and considering the rest caption text (problem here is that themes that hook into img_caption_shortcode will have no support for the new version of captions text).
We're screwed there either way, though. Once we nest the content into caption_text, we're backwards incompatible for callbacks expecting an attribute.
So, we need to figure out how to be compatible here. What we can do is make img_caption_shortcode() reach into $content, remove the caption, and add it into $attr['caption']
. Then pass the modified $attr and $content into the filter.
This would work best with a single [caption] shortcode, and no [caption_text]. The regex would need to account for <a> tags around the <img> tag as well.
#40
@
13 years ago
You are right.
Perhaps something like this makes more sens:
https://gist.github.com/1854115
#41
in reply to:
↑ 38
@
13 years ago
Replying to nacin:
We're screwed there either way... So, we need to figure out how to be compatible here.
Not exactly. The current patch is backwards compatible. All it does is to force the quotes in the newly added HTML inside captions to be single. This works well and doesn't require any changes to the shortcode format or the handling/parsing of the shortcode in the back-end, including plugins.
Furthermore it accepts both quote formats when editing a post, so even if a plugin has added a caption shortcode using single quotes for the attributes (default is to use double quotes), it would handle it properly.
Agree with @sushkov that now would be a good time to tweak/improve the captions shortcode but don't see any way we can do that and keep it 100% backwards compatible.
#44
@
13 years ago
UI/UX in wpeditimage.html is tricky stuff because ed.translate()
replaces existing document.body with a new one, and old elements bindings die.
To get over it, I moved wysiwyg outside the components that require translation, after those are parsed, wysiwyg hides initial textarea, and updates its value in the background.
Looking back, I think we can just instantiate wysiwyg with the textarea name, since all this stuff depends on js and name attributes are irrelevant.
As for now, wpImage.enableWysiwyg()
doesn't convert double-quotes into single-quotes, so using this from tinyMCE might srip formatting. Will take a look later.
Files are not minified.
#46
follow-up:
↓ 50
@
13 years ago
Is it possible to use wp_editor() (HTML editor only)? We implemented it on edit-comments.php and it turned out well.
#47
follow-up:
↓ 51
@
13 years ago
Hmm, in wp-admin/media.php
yes, already done.
In tinyMCE popup, sure, but it would require loading half of WordPress, and even more changes across codebase, so I'm not sure it's worth that.
A month ago I would never believe this ticket would require that many modifications. Right now the diff requires some more cleanup. But a nice working patch will be ready for upcoming meetup.
#50
in reply to:
↑ 46
@
13 years ago
Replying to sushkov:
...tricky stuff because
ed.translate()
replaces existing document.body with a new one, and old elements bindings die.
Yes, unfortunately that's the way TinyMCE translates the iframe body in its popups. The way around it is to bind events after the translation is done.
Replying to nacin:
Is it possible to use wp_editor() (HTML editor only)?
Currently there are five places where captions can be edited and only two use roughly the same code:
- Media->Library screen (single image properties),
- "Media" tab in the uploader popup (single image properties),
- "From URL" tab in the uploader popup,
- "Edit Image" popup in TinyMCE,
- Editing directly in the visual editor.
We can use wp_editor() in the first three as they are generated from PHP. The "Edit Image" popup in TinyMCE is all JS based, so Quicktags must be loaded directly there.
However the question here is mostly UX: by far the best place to edit captions is directly in the visual editor. It is WYSIWYG and the user has access to more tools (other inline tags, remove formatting, etc.).
Was thinking we can add third button that pops up when clicking on an image in the visual editor. Currently when users want to add a caption while composing a post they must:
- Open the "Edit Image" popup,
- Add some text in the caption field,
- Click Update/close the popup,
- Edit the text they added in step 2 and add HTML elements to it.
The new button would add (and remove?) the caption elements around the image directly without the need to open the "Edit Image" popup, i.e. the users would start directly at step 4.
#51
in reply to:
↑ 47
@
13 years ago
Replying to sushkov:
Hmm, in
wp-admin/media.php
yes, already done.
We should avoid using a buffer there. I know there's a "circular dependency" between media.php and wp_editor() in there but we should be able to work around it.
In tinyMCE popup, sure, but it would require loading half of WordPress, and even more changes across codebase, so I'm not sure it's worth that.
Not half, all of the admin as it would require wp-admin/includes/media.php. Not sure it's worth it either as it will make that popup (a lot) slower.
A month ago I would never believe this ticket would require that many modifications...
Nothing we can't handle :)
#52
follow-up:
↓ 53
@
13 years ago
Hey @azaozz could you take a look at line 144 in wpeditimage/editor_plugin_src.js' in the context of
From URL` tab (I'm asking because you already did some changes there).
Thanks.
Regarding ob_start()
, I would leave it for later. To be honest, I don't see any reason why buffers should be avoided...
#53
in reply to:
↑ 52
@
13 years ago
Replying to sushkov:
Regarding
ob_start()
, I would leave it for later. To be honest, I don't see any reason why buffers should be avoided...
ob_start()
is "evil" :) We've had problems with it in the past, seems some hosts run custom handlers that do all sorts of things with the buffers.
#55
@
13 years ago
One reason I would disable wpLink
in image popus, is because wpLink itself is a popup.
While its OK to have it inside single media.php
item pages, in editor it results in some sort of Inception
movie...
Static wpeditimage.html
needs a bunch of js files in header to make it work, and a valid tinyMCEPopup
object.
UI/UX feedback is welcome, please.
#56
@
13 years ago
Hey guys it's come a long way!
TRUNK VERSION
The links work and I'm finding they persist well when switching back and forth between HTML and Visual. It's great that the caption can be edited in the Visual editor and in the popup.
One thing I noticed is it strips out any use of title=
in links. Is that intentional/desirable? Link titles are important for accessibility as well as for SEO, if there's one attribute we should keep other than href it's probably title.
It's nice that if you type double quotes for caption links in the HTML editor then switching to VIsual editor makes them single quotes, but is there a way we could also do the same thing for people who only use the HTML editor? Right now saving the posts keeps the broken double-quotes-inside-double-quotes format and previewing it just makes the caption dissapear. It would be nice if people were protected from themselves even if they only use the HTML editor.
We aren't planning to stick with the "Add link" UI that's in trunk right now are we? I'm a bit confused but it sounds like Sushkov's patches to add quicktags is the right way to go.
PATCH VERSION
Unfortunately I can't seem to get the 18311_qtags_ui.4.diff patch to work. It patches successfully, but the result is very broken. In the uploader popup I can see a loose textarea at the bottom of the popup with the quicktags above it but looking unstyled.
Is there another patch from above that I need to get it working with trunk?
#57
@
13 years ago
Replying to sushkov:
One reason I would disable
wpLink
in image popus, is because wpLink itself is a popup...
...Staticwpeditimage.html
needs a bunch of js files in header to make it work...
The problem is that the default "link" in Quicktags is... quite bad. It opens a browser generated modal popup (for the url) which is most basic (and uglier) UI we can have (unfortunately there's no other way to do it in Quicktags). Also don't think we can use wp-link in some places and not use it in other. The UI needs to be consistent.
The current UX/UI questions are:
- Shall we add Quicktags on the Image Caption fields.
- With Quicktags we can have 2-3 more buttons that insert HTML tags but the "link" button opens an ugly modal popup.
- Without Quicktags we only have a "link" button but the UI/UX is better.
- It may be possible to tweak Quicktags' link button to use the current link UI. Would that be the best option.
In both cases we would be showing the raw HTML code there.
Another inconsistency is that the "Caption" fields would have some sort of HTML editor but the "Description" fields would not. Description is actually the post_content and supports HTML.
- Shall we try to steer the users into editing captions directly into the visual editor. This is by far the best place to edit captions as all is wysiwyg, the image together with the caption usually looks exactly the same as on the front-end. An idea on how we can do that is to add a third button when clicking on an image in the visual editor. That button would be for adding (and removing) the caption "wrapper" on the image.
#58
@
13 years ago
FWIW I'd be satisfied with no UI for adding links in the popups. IMHO the bug was that it was impossible, so as long as me and my users can type in an <a> tag and have it work that's a huge step forward. The fact that people who don't know HTML can use the Visual editor is also very logical. While it may not be a perfect implementation, it's both pretty easy to figure out that the Visual editor can do it and dead-simple to explain to someone ("Oh, just use the WYSIWYG button after inserting the image"). Of course, having a UI would be good for promoting the fact that HTML works in captions now, but that doesn't seem like a huge priority to me especially while it's so new.
Overall, I'd say if we can't get at least the quicktags (if not the full visual/html editor) then we're better off without a link button complicating the UI. To me it's more confusing to have just that one button than to have the whole row, because the row is already familiar and has a clear relationship to the textarea.
Also, in the current 'add link' button hitting enter does not do what I'd expect. Instead of inserting the link it causes the whole popup to close and insert the image into the post. If we keep the 'add link' button (or integrate it into quicktags) it would be good to change that, as it was really confusing and I also lost my work on the link (not much work but still).
#59
@
13 years ago
Replying to jeremyclarke:
One thing I noticed is it strips out any use of
title=
in links.
Titles on links (or other tags) seem to work properly here. Could you try to trace where are they stripped.
It's nice that if you type double quotes for caption links in the HTML editor then switching to VIsual editor makes them single quotes, but is there a way we could also do the same thing for people who only use the HTML editor?
Don't think so. We aren't running any pre-save filters on the content of the HTML editor and I think most people that use it would insist on that. It's a simple case of using the right quotes, I mean $str = "single quote is ok here";
and $str = 'double quote is ok here';
, same as in any html tag. Also people that use the HTML editor usually "know what they are doing" :)
We aren't planning to stick with the "Add link" UI that's in trunk right now are we? I'm a bit confused but it sounds like Sushkov's patches to add quicktags is the right way to go.
Still undecided (see the above comment on that). Using Quicktags would make it better for people that use the HTML editor and have at least some basic understanding of HTML. For the big majority of users it wouldn't make any difference though.
Yes, the current "Add link" button and UI would need some polishing if we decide to keep it.
#60
follow-up:
↓ 61
@
13 years ago
I noticed that highlighting text in the caption in TinyMCE and using toolbar buttons works. However, in HTML it looks like doing the same works with everything but del. It seems that the del button in TinyMCE just ads the tags, but in HTML mode it adds tags AND a param for the date it was deleted. Do you think we could bring those buttons into sync with each other, standardize on no param, and let all the toolbar items in the HTML editor work? If so I can open another ticket.
#61
in reply to:
↑ 60
;
follow-up:
↓ 62
@
13 years ago
Replying to aaroncampbell:
...It seems that the del button in TinyMCE just ads the tags, but in HTML mode it adds tags AND a param for the date it was deleted. Do you think we could bring those buttons into sync...
Good idea. Yes, could you open another ticket for that.
#64
follow-up:
↓ 66
@
13 years ago
I've done some simple testing with this and the change to a text area for entering the "Default Caption" allows the user to enter multi line data which then breaks the insert into post js:
unterminated string literal https://mytestsite/wp-admin/media-upload.php?type=file&tab=library&post_id=2509 Line 4
for
win.send_to_editor('[caption id=\"attachment_2375\" align=\"alignnone\" width=\"...
#67
follow-up:
↓ 68
@
13 years ago
- Cc yoav@… added
The textarea has the code
class, which means it's automatically LTRd, even in RTL environments.
Since I presume most usage will still be with just text, it should be allowed to inherit the default text direction instead.
A few ways to do that:
- Use Another class instead of
code
- Have no class at all?
- An additional unique class that can be defined in the RTL css
- Use a wider selector for the RTL css, such as {{{.media-single textarea.code} maybe
#68
in reply to:
↑ 67
@
13 years ago
Replying to yoavf:
The textarea has the
code
class, which means it's automatically LTRd, even in RTL environments.
Right, will remove the code
class. Still thinking the textarea should have 'monospace' font as that improves readability for HTML tags.
#71
follow-up:
↓ 72
@
13 years ago
I don't think a monospaced font is appropriate here. Reverted in [20282].
#72
in reply to:
↑ 71
@
13 years ago
Replying to nacin:
I don't think a monospaced font is appropriate here.
When using a variable width font the HTML tags are a bit harder to read. As the HTML editor has monospace font, added it here for consistency.
The change of the field's label from "Caption" to "Default Caption" was meant to distinguish between captions that are saved in the attachment post's excerpt vs. captions that are added after the image has been inserted in a post (discussed with Jane).
#73
@
13 years ago
- Resolution set to fixed
- Status changed from assigned to closed
New bugs or enhancements on new tickets. Nice work, everyone.
#75
@
13 years ago
- Keywords ux-feedback has-patch removed
- Resolution fixed deleted
- Status changed from closed to reopened
Quotes cause a lot of problems when used in shortcode parameters (like the quotes on html attributes). Let's try putting captions as part of the content instead. (see IRC discussion)
#77
@
13 years ago
18311-5.patch is support for a new style of caprions shortcode (contains 18311.php.diff):
[caption id="" align="" width=""]<a><img /></a>Caption Text and HTML[/caption]
#78
@
13 years ago
- Keywords has-patch added
Looks ok.
One note, I found this regex a bit shorter:
preg_match( '~((?i:<a\s.*<img\s.*?>.*?</a|<img\s.*?)>)(.*)~is', $content, $matches );
I attached some tests you might want to take a look at and also come with some feedback.
#79
@
13 years ago
18311-6.patch is the same as 18311-5.patch except it adds a space between the <img /> tag and the caption text to make it a bit more readable. The space is trimmed when separating the caption from the image.
#80
@
13 years ago
- Priority changed from normal to highest omg bbq
- Severity changed from normal to blocker
This needs to land prior to Beta 4.
#81
@
13 years ago
Did some quick testing and seems the caption separating regex can be optimized a bit:
preg_match( '#((?:<a [^>]+>\s*)?<img [^>]+>(?:\s*</a>)?)(.*)#is', $content, $matches );
seems to be twice as fast (as there's no back tracking from the .*?
). The image tag + link + caption are generally not very long string but this runs on the front-end and any speedup is good.
#84
@
13 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Got some UX feedback that the label change to "Default Caption" is not better, @nacin was right to revert it :)
#87
in reply to:
↑ 83
@
13 years ago
Replying to sushkov:
Tests are still left aside, any chances we get those merged?
Looking through the tests: wouldn't it be simpler/more robust to feed them a shortcode string and compare the HTML string that comes out to what it should be? Using assertEquals( 1, preg_match_all( "~...*{$content_preg}~", $result, $_r ) );
seems like a complicated way to do it.
#88
@
13 years ago
I don't see how that is different with what is already done.
Maybe you can file a patch, or just update the unit-tests svn, I pushed it already some time ago.
I toyed with using a shortcodes to implement the markup just to fix the problem, but found that having the ] in the caption also breaks the handling, see #15694