This is the follow-up issue for #2014895: Image captions & alignment (for inline images). That issue was about having image captions in Drupal core that don't prevent structured content. However, it was still necessary for users to manually modify the HTML with the right syntax; there was no CKEditor (WYSIWYG) support.
This issue is about providing that: a great UX for adding captions to images and aligning images.
It builds on 2 things:
- #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms
- CKEditor Widgets
It slightly expands the image dialog, and allows the user to directly modify the caption text. Screenshots:
Comment | File | Size | Author |
---|---|---|---|
#83 | llamas-dancing.jpg | 162.64 KB | Wim Leers |
#78 | Screen Shot 2013-09-11 at 11.54.41.png | 97.13 KB | Wim Leers |
#76 | Screen Shot 2013-09-10 at 2.58.25 PM.png | 80.64 KB | webchick |
#76 | Screen Shot 2013-09-10 at 2.59.32 PM.png | 66.78 KB | webchick |
#74 | Create Article | Site-Install 2013-09-10 14-22-48.png | 137.89 KB | webchick |
Comments
Comment #1
Wim LeersHere is the patch showing the current state of affairs.
Reviews welcome, especially usability reviews, but no nitpicky code reviews yet please.
Built on top of:
This includes a new build of CKEditor, of their latest d8-imagecaption branch, built with the same
build-config.js
, with the sole change of having added the "widget" plugin.TODO:
2014895
Test here on simplytest.me, using an uploaded image: http://simplytest.me/project/drupal/712fe2a18dce3581d3301b43777ac6749ca6c502?patch[]=https://drupal.org/files/image_captions-2014895-32.patch&patch[]=https://drupal.org/files/ckeditor_dialogs-1879120-91.patch&patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-CKE-UPDATE-2027181-1.patch&patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-2027181-1.patch
Comment #3
Bojhan CreditAttribution: Bojhan commentedI am wondering, don't you set caption position in your html/css? Seems somewhat weird this would be up to the content creator.
Comment #4
Wim Leers#3: The caption is always positioned underneath the image. You can change the CSS to put the caption on top of or next to the image.
Note that zero new functionality is being introduced here. It only provides a front-end to #2014895: Image captions & alignment (for inline images)!
Comment #5
Wim Leers.
Comment #6
Wim LeersInfuriating.
Comment #7
Wim LeersReroll of #1: I forgot one crucial file.
Use the same CKE UPDATE patch.
Comment #8
Wim LeersAll TODOs mentioned in #1 finished. This is now ready for final reviews, except that #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms needs to get committed *first*.
Inserting new images works well now (it required an API change in CKE Widgets), and using this together with the alignment buttons also works fine.
Built on top of the same patch stack as #1.
Comment #10
Wim Leers#2014895: Image captions & alignment (for inline images) has just been marked as a task, not a feature request by catch, hence this should also become a task.
Comment #11
Wim LeersReroll now that #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms, #1907418: Make CKEditor's alignment + underline buttons available (but not enabled by default) and #1473066: [Meta] Move third-party assets out of core/misc folder have been committed.
Test on simplytest.me: D8 c09d2791a7e75212beb8d51981c3f649983a14fa plus the two attached patches.
Changes:
Now ready for review!
Comment #12
Bojhan CreditAttribution: Bojhan commentedHow do I get the caption?
Comment #13
Wim Leers#12: I'll post a screencast very soon.
Reroll to fix the two minor test failures in #11.
Comment #14
Wim LeersScreencast: http://vimeo.com/wimleers/image-caption-ckeditor-widget-drupal-8.
(Also attached as a .zip file in case the above URL ever stops working. I wish d.o allowed uploading + embedding screencasts.)
Comment #15
quicksketchSo for others that had trouble figuring this out, you have to apply the second patch from #11 (which updates CKEditor) AND #13. Although in hindsight Wim basically said that in #11.
So some experiences I had with this:
I don't think we can use Drupal.t() in CKEditor plugins because they don't pass through drupal_js_alter(), which is how Locale.module adds its translated strings.
The caption filter runs through the theme layer, but this template seems like it's hard-coded. It seems like we should at least use a CKEditor settings variable for creating our template, making it possible to match any overrides to the theming of caption filter.
Usually drupal_alter() gets the last shot at modifying something, what's the reason for moving the theme CSS after the alter?
Yay! Nice to see the "CKEditor CSS files from the info file" feature being used.
Comment #16
Bojhan CreditAttribution: Bojhan commented@Wim I am getting a white area on simplytest.me?
Comment #17
Wim LeersPinging the CKEditor developers about that.
That will be supported once the underlying CKEditor Widgets system supports it. They're still working on that.
You're right, but what you were doing there earlier (referring to
editor.lang.image.menu
also doesn't work: those won't exist anymore once we remove the "image" plugin. So we need to figure out a solution to this problem. In any case, that is broken already in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms so I suggest we work on that in another issue. I created one, and I pinged domain experts Gábor Hojtsy & nod_, as well as the CKEditor developers: #2035721: Figure out how to translate Drupal-specific CKEditor plugins.First: note that it's in
drupalimagecaption/theme.js
. The base logic lives atdrupalimagecaption/plugin.js
. If any theme wants to override the HTML structure, and they want this to be accurately reflected in CKEditor too, then they must overridedrupalimagecaption/theme.js
as well.Second: I know that this is less than ideal, and I would have loved to simply have a
Drupal.theme.filter_caption
function, but the CKEditor developers assured me that this is not technically possible. We've had long discussions about this. If necessary, I'm sure they won't mind explaining that here on the issue.Third: a factor that should have some weight is the fact that the default HTML structure used by
filter-caption.html.twig
is using the HTML5 best practices. It should be sufficiently flexible to simply override the CSS to suit >95% of all use cases. Which means only experts would need to override the JS.So: do you think this approach is acceptable?
This change was necessary to be able to achieve the required CSS order.
First, there is this, to only add the
filter_caption
's filter when it is actually necessary.Second, there is Bartik's
ckeditor-iframe.css
, which overrides certain of CKEditor's iframe styling… more specifically, the one mentioned just above:filter_caption
's CSS.So: modules must dynamically add CSS (
drupal_alter()
) before the theme, because otherwise the theme is no longer capable of overriding the module CSS.If you can think of cases where the opposite is necessary, then I think we will need two
drupal_alter()
's: one before the theme CSS is added, and one after.:) Yes!
Comment #18
Wim LeersThe patch still applies and works fine. Can we please get a usability review to start moving this forward again?
Simplytest.me link to test it: http://simplytest.me/project/drupal/1cb65032844cb12907ed484e3c17ded1528ea44f?patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-CKE-UPDATE-2027181-11.patch&patch[]=https://drupal.org/files/ckeditor_widget_imagecaption-2027181-13.patch
@quicksketch: I also replied to your review — please respond! :)
Comment #19
Bojhan CreditAttribution: Bojhan commentedLooks good, the outline on focus looked a bit wierd here - but perhaps thats because the image is larger than the textarea.
I was a tad bit confused about the interaction to configure it, but Wim told me thats not part of this patch.
Lets get this in :)
Comment #20
webchickAdding images to posts is really awkward currently; raising priority of this to major to reflect its importance.
Haven't tried this yet, but it looks promising! :D
Comment #21
Bojhan CreditAttribution: Bojhan commented@webchick Could you add what is akward? This patch doesn't touch much of that experience.
Comment #22
webchickOops. You are right. This just enhances the existing interface some. Sorry!
RE: Awkwardness, I find it very awkward to upload an image *below* the Body field, which where I want to actually *use* the image, and then have to copy its URL, then scroll back up and paste it into the image button dialog. But I suppose I can file a separate issue about that, if I don't find one that already exists. :)
Comment #23
Wim LeersFor that part of the UX, please see #1932652: Add image uploading to WYSIWYGs through editor.module :) But I see you've already found it, webchick!
Can we move this forward again now? Usability review happened, this just needs code reviews to get to RTBC?
Comment #24
Wim Leers(I wanted to reroll, but this is blocked on https://github.com/cksource/ckeditor-dev/tree/d8-imagecaption being rebased on the CKEditor 4.2 release, since Drupal 8 now ships with CKEditor 4.2. Hence this is briefly blocked on the CKEditor developers.)
Comment #25
Wim LeersWhile working on the update, I ran into a problem. Uploading a temporary patch so I can give the CKEditor developers access to a patched D8 on simplytest.me instance :)
Comment #26
Wim LeersThis is a straight reroll of #13, with an updated build of CKEditor (a specific branch that will become part of CKEditor 4.3, but is based off of CKEditor 4.2 that's already in Drupal core, so there won't be any regressions).
Because the number of changes to CKEditor is now much smaller (e.g. none of the language files are being changed), it now all fits in one patch.
Please review, so this patch can move forward!
(Based on Drupal 8 commit 6400bf8b21cb44338e2af94e25107003cdfeeeb7.)
Comment #28
Wim Leers#26: ckeditor_widget_imagecaption-2027181-26.patch queued for re-testing.
Comment #29
seutje CreditAttribution: seutje commentedI'm concerned about the upcasting system.
Try the following:
Also, I found the interface a bit confusing, I thought the alignment would have effect on the caption.
[edit: masking my poor grammar skills.]
Comment #30
Wim Leers#29:
I've swapped them: now the alignment settings come first. A small change that should subtly help. Also: this is a one-time learning experience, so I'm not too worried about it in general — otherwise Bojhan would have noticed it too :)
RE: bug: great, great catch! Worked around this by disallowing alignment and captions on inline images that are surrounded by text. That makes hardly any sense anyway: the main use case for inline images are smileys, and those should not be captioned. Aligning inline images completely does not make sense: why would you want the image to be in a specific location in the text, if you're still going to change the alignment anyway?
I recognize that this is suboptimal UX-wise, but overall this is still a very big step forward. Only when CKEditor's Widget system is finalized in version 4.3, we'll be able to refine this further, because they're still working on allowing widgets to work both for inline and block elements: http://dev.ckeditor.com/ticket/9764#comment:57.
So, please don't let perfect be the enemy of good!
Comment #31
seutje CreditAttribution: seutje commentedApplying patch from #30 resulted in a partially built page with following markup:
http://pastebin.com/gmXL5WcE
error log in attachment.
Comment #32
seutje CreditAttribution: seutje commentedstatus
Comment #33
Wim LeersAlso because of #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity… Easy fix :)
This should do the trick.
Also: this is why I hate how PHP handles namespaces. If you do
Then PHP will continue just as if nothing is wrong… :(
Comment #34
seutje CreditAttribution: seutje commentedKinda figured it was cause I took too long to test it, mi scsi!
Gave up on PHP a while back...
Will try to give this a run later tonight (have to go have dinner with the folks, so no guarantees)
Comment #35
seutje CreditAttribution: seutje commentedQuick review, not much time. will try to review in further detail later tonight.
kinda annoying to have the vendor stuff in there :/
issue reported in #31 is indeed fixed
it's kind of weird though that until introducing either alignment or a caption, there is no margin applied, but there is after applying it. Due to it gaining a
<figure>
-wrapper, to which default margins are applied (by Chrome). I guess it's a limitation to the fact that we have to turn it into a block-level element, but perhaps we should remove those margins from default themes and the editor?tiny nitpick:
Can we cache the length? like
for (var key = 0, len = foo.length; key < len; key++)
so we don't have to re-query for it?Comment #36
seutje CreditAttribution: seutje commentedsome more nitpicking:
can we space out the object literal with a space after each comma and colon?
this is a bit much, no?
since the
else
isn't really necessary, maybe it's useful to combine theelse if
and theif
, reducing indentation?also, as a side note, I'd like to point out that
<img/>
is interpreted by every browser as<img>
(unless served as application/xhtml+xml), it's a void element and an end tag is actually forbidden according to html 4.01 and 5 spec. It is generally done for XHTML compliance, which is pretty silly in the whole 80/20 picture. But that is not a can of worms I want to open at this point, so feel free to ignore. :)other than these small notes, I have no more concerns on the JS-side of things
Comment #37
Wim LeersOh! Excellent point! :) Thanks! Now fixed.
Done.
Two separate patches also causes problems; core committers don't like that.
I agree, but this is copied verbatim from existing CKEditor plugins. It's not *that* bad. It'd be worse to split it over multiple lines.
Indeed — removed!
With the JS review out of the way (thanks, seutje!), I think the most important remaining thing to be done for this patch is how to deal with the need to duplicate Bartik's CSS for captions in a
ckeditor-iframe.css
file for Bartik. I think the easiest way forward is to just duplicate it, because using@import
is bad for performance, and astyle.css
file importing ackeditor-iframe.css
file also seems pretty bad.This is essential for image captions to look decent on the node add/edit form out of the box. But the rest of Bartik's styling should also be approximated. #2064379: Remove ckeditor-iframe.css and load relevant Bartik CSS files for CKEditor's iframe mode. was created for that a few days ago.
With that, the last @todo is out of the way. Somebody can now RTBC this patch :)
Comment #38
jessebeach CreditAttribution: jessebeach commentedwow, CKEditor plugins/widgets are complex :) I reviewed the JS/PHP/CSS. It all looks great. I did some manual testing and ran into one error with in-place editing:
STR
That's the only issue I found. I don't have time at the moment to look into it, but I'll have a peek later tonight.
Comment #39
Wim Leers#38: That is a bug in our
ckeditor.js
— see #2067427: Drupal.editors.ckeditor.attachInlineEditor() should pass Drupal-specific settings like Drupal.editors.ckeditor.attach() does for the fix :)Comment #40
jessebeach CreditAttribution: jessebeach commentedOk, the bug I raised in #38 was the only blocker for me RTBCing this issue. Since we have an RTBC'ed fix for the bug in #2067427, then my blocking issue is addressed.
Setting to RTBC. I'm excited to see Drupal forms driving CKEditor configuration. Amazing work Wim Leers!!!
Comment #41
eaton CreditAttribution: eaton commentedSeconding the excitement here. This patch also provides an extremely useful demonstration of the technique for other developers interested in building similar editing enhancements for Drupal 8.
Comment #42
Wim Leers#41: Woot! Very happy to have you excited here, eaton :)
CKEditor Widgets will only get better, so I am very happy to see all this excitement!
Comment #43
alexpottPatch no longer applies
Comment #44
Wim LeersThe commit of #2029737: Create hook_help() for ckeditor module before this one caused this patch to no longer apply in a single hunk.
Comment #46
Wim Leers#44: ckeditor_widget_imagecaption-2027181-44.patch queued for re-testing.
Comment #47
Wim Leersd.o tag monster again — as well as testbot failing to retrieve the patch from d.o and hence marking this issue as NW instead of retrying. :(
Comment #48
Wim LeersPatch is green — RTBC again as per #40 & #41.
Comment #49
alexpottYet another reroll is required :(
Comment #50
Wim LeersRun tests, run!
(Changing
array('system', 'drupal.debounce'),
toarray('system', 'drupalSettings'),
did the trick! I wish patches were smarter :))Comment #51
webchickNo longer applies. :( Here are a few Qs in the meantime.
Most of the patch is this. Normally we would do this in a separate issue, but we're actually updating CKEditor to a "dev" release equivalent, not a full release. I checked with nod_ and he said he was ok with this approach.
Does this need to run through the Drupal theme system somehow?
AFAIK these should now be $this->t().
Hm. Interesting. How does Aloha Editor in contrib get Bartik to provide support for it?
Comment #52
eaton CreditAttribution: eaton commentedI'd say it's probably overkill. It only applies to the representation of the captioned item inside the CKEditor edit window, not and that's functionality specific to the editing plugin. Overriding that would change the behavior of the plugin, and would have to be accompanied by CSS and JS refactoring equivalent to writing a different plugin... which people can do. :D
Couldn't it provide its own Bartik-specific stylesheet? The idea that contrib modules should be "first class citizens" is an important principle, but having CSS in a core theme to cover new core modules and capabilities sees perfectly fine. As long as the file is genuinely Bartik-specific, it's equivalent to adding a set of CSS tweaks to cover a new Views plugin style that ships with core.
Comment #53
Wim Leers#51:
EditorImageDialog
to usingFormBase
. This also allowed me to remove the emptyvalidateForm()
method, whichFormBase
already does for us.Besides webchick's apt remarks, quite a bit else has changed in the mean time in Drupal 8 HEAD, forcing further changes upon this patch:
filter_format_load()
is gone, I converted it to useentity_load()
instead.data-editor-file-uuid
attribute to also be handled in the code being added here.managed_file
element to upload images; the AJAX submit that occurs when a user presses the "Upload" button that comes with that form element caused havoc. Fixed now. (Rather than using$form_state['input']['editor_object']
directly, which gets messed up by AJAXy form elements, we now store that information explicitly in$form_state['image_element']
, just like elsewhere in Drupal core.)data-cke-saved-*
, which was introduced in #2071957: Cannot change existing images or links through Text Editor's image/link dialogs, should also be applied here.#52: Thanks for the feedback! :)
Comment #54
jessebeach CreditAttribution: jessebeach commentedThe diff in #53 looks good. I manually tested the feature and found that a caption and alignment are correctly applied for a Full HTML format. While testing the Basic HTML format, I found that while I'm able to add an image with a caption, that caption is not preserved when the node is created.
If I edit the new node, re-add a caption and save it (without changing the format value from Basic HTML), the caption is preserved and presented on the node view page.
This was the only issue I found while manually testing. Let's get this resolved and I'll retest.
Comment #55
Wim LeersGreat find, Jesse! Fixed now.
Comment #57
Wim LeersChasing HEAD. (The commit of #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants.)
Comment #59
Wim LeersUgh. The many many rerolls that I'm doing caused me to contaminate this patch with hunks from another :( Removed the
CommentNew*
hunks.Comment #60
jessebeach CreditAttribution: jessebeach commentedThe issue I found in #54 has been resolved. This is an amazing feature that folks have been clamoring for for years. I've pounded on with manual testing and it's held up. The code has been through numerous rounds of review and refinement. Let's get it in!
Comment #61
webchickI'm really sorry to "needs work" this patch after all the extensive work and reviews, but in trying this out...
I really think that the "Caption" checkbox needs to invoke the States API and show a textfield for the caption to use as a default:
Because otherwise the UI for entering your caption is not remotely obvious at all, particularly on a large image:
But even in a best-case scenario, it doesn't make sense to use an in-place edit UI as a data entry UI.
Secondly, when I erased my old image and inserted a new one, now I get "Inline images cannot be aligned." and "Inline images cannot be captioned." What gives? Why can't I do that to inline images? And if that's true, why didn't I get that the first time?
I'm in Firefox atm, will test it in Chrome monentarily to see if that works any better.
Comment #62
webchickK, Chrome does indeed work much better. Guess this is "needs work" for Firefox, too. :\
Comment #63
Bojhan CreditAttribution: Bojhan commented@webchick This is a little confusing, the whole point of in-place edit is that you can edit everything. I don't think users will understand the data entry UI vs. in-place edit UI concept at all. If we start spitting it out, and some text can only be edited in a config modal like you are proposing, then you are only introducing more inconsistencies in the experience.
There probably should be a placeholder area, with a "placeholder" text to enter a caption there - that sounds more natural to me, than having it in a modal dialog. Even without this placeholder text, I didn't find it particularly confusing - but perhaps thats also because I test on chrome - where you get an empty bar below it.
Comment #64
webchickOooh. Placeholder in the in-place editing UI could work, yeah. It's just that until you get used to what you're looking at and how it works, it's not really obvious that...
If there was text there saying "Edit me" or whatever, then that concern would go away.
Comment #65
Bojhan CreditAttribution: Bojhan commented@webchick Yhea, you are right. You need to know that you can type there, I was already in a frame of mind when seeing that box. Lets add the placeholder :)
Comment #66
Wim LeersTo summarize #61–#65:
Answers:
The second screenshot in #61 does not show the styling that would be applied if you had checked the "caption" checkbox in the dialog. So I think you just forgot to check it? :) Without checking that, you indeed won't be able to enter a caption.
input[type=text]
with HTML tags in there. Plus, two ways for editing the caption. Plus, more cognitive load in the dialog. Plus, then we might almost as well just not use CKEditor Widgets' nice UX at all.Google Docs does this similarly: when the cursor is within a paragraph (anywhere, including at the end), then the image will be inserted inline. Start a new paragraph first, then insert an image, and the image will be a block-level image.
In the attached reroll, I added a placeholder caption. No new JS logic, just one additional CSS rule! Thanks to Piotrek Koszuliński from CKEditor for the tip.
Attached is also a screencast of this placeholder feature in both Chrome and Firefox. When you delete your caption in Firefox, the placeholder reappears, as it should. In Chrome, it does not, because Chrome's
contentEditable
implementation adds a<br>
for an unknown reason, hence the CSS selector does not apply. But the most important thing is of course that the placeholder is displayed initially, without adding any more complex JS logic.Comment #67
Bojhan CreditAttribution: Bojhan commentedThe only outstanding issue I'd see is that it isn't visually clear it is to be replaced. Perhaps making it italic, and adding ellipses helps with this. We can't make it more gray, because that would probably contrast to low.
Comment #68
Wim LeersI discussed this with Bojhan; we're not adding an ellipsis because that would 1) look weird IMHO, 2) be inconsistent with the placeholder at e.g.
admin/modules
.Attached patch makes the placeholder text italic.
Comment #69
Wim LeersD'oh, the patch in #68 *does* include the ellipsis, even though the interdiff indicates it does not. I forgot to update the patch after removing it, but I did update the interdiff. Sorry for the noise :(
Comment #70
webchickThe bug in #61 was from using an exceptionally LARGE image + Firefox. I'll try it again with the latest patch sometime today, if no one beats me to it.
Comment #71
jessebeach CreditAttribution: jessebeach commentedTested in Firefox with an exceptionally large issue.
The issues in #66 have been addressed.
I can't find any more issues with this patch.
My vote is RTBC. I'd like Bojhan to have a chance to review, so I'm leaving this as Needs Review.
Comment #72
Bojhan CreditAttribution: Bojhan commentedI reviewed it, earlier (me and WimLeers tend to chat on IRC about these kind of issues) I was waiting for Angie to chime in. But this looks good marking it back to RTBC.
Comment #73
jessebeach CreditAttribution: jessebeach commentedAngie will bang it with her war hammer Bug Shine, the revealer of truth and instability.
Comment #74
webchickSo! Close!
Here's what happened this time. Again, testing in Firefox since I know you guys use Chrome. ;)
#1: The cursor is weird:
OTOH, on a wide image, the cursor being on the left is the only way I know that clicking into the region did anything:
However, I actually think either way, we want to center the cursor so it appears over the placeholder text. Chances are that's what I would click on when I'm ready to edit, even if I have to scroll to the right to do so.
Then, because #2076847: FilterHtmlImageSecure incorrectly flags images coming from site URLs with ports in them as insecure is still an issue, I remembered I needed to switch my text format from "Basic HTML" to "Full HTML." I was being a jerk and entering HTML like
<script>alert('foo');</script>
into the caption to see what would happen (nothing bad) but when changing the text format, it turned into:This is also the case even if I just post the node and then go back to edit it. So something is not quite preserving the previous contents; seems to be double-check_plain()ing. Not sure if that's introduced in this issue or not, but raising it nonetheless.
Also, escalating this issue to "major" to indicate its importance. This actually exercises the CKEditor Widget API to ensure it can meet our needs, and provides a hook-in point for contrib to do more stuff cool UX like this.
Comment #75
jessebeach CreditAttribution: jessebeach commentedI don't believe there's anything we can do to center the cursor short of inserting a non-breaking space on focus, then removing that nbsp on the first key down. This is a browser content editable node. The behavior is the same in Firefox and Chrome.
Comment #76
webchickOooh, one other thing I found while testing #1741498: Add a responsive preview bar to Drupal core. Captions seem to break responsive image handling.
Large image w/ no caption:
Large image w/ caption:
Comment #77
eaton CreditAttribution: eaton commentedThat would be an excellent followup issue, IMO. The basic approach taken by the module -- figure, figcaption, and image -- should work just as well as the existing setup from a markup perspective. It LOOKS like this doesn't break "responsive image handling" so much as "the current responsive image handling does not properly handle images inside of figure tags."
Comment #78
Wim LeersTo summarize #74–#77:
Answers:
<em>
) is not lost.<
and>
) and ampersand does indeed result in them being HTML encoded.So it's not as bad as I thought it was after reading the comments :), but it's still definitely not good. I talked to the CKEditor developers, and they said this will be fixed in the final version of the Widget API, which will be part of CKEditor 4.3. First we will get a CKEditor 4.3 beta, which will help finalize the API. This beta is coming in the next few weeks.
I think the best conclusion is to commit this as-is, we will then get the improved/fixed/finalized Widgets API in the near future. That will fix this problem automatically: the custom plugin being added here does not do anything related to HTML encoding, it's a bug in the CKEditor Widgets API.
We already have an issue to update to the CKEditor 4.3 beta: #2039163: Update CKEditor library to 4.4.
Overall conclusion: 1 is a non-issue, 2 is an upstream bug that will be fixed for us, 3 is at least out of scope, and possibly not broken. Hence back to RTBC.
P.S.: I did test in Firefox, as you can see in the screencast included in #66. In fact, I also tested that before, and it's always worked just fine!
Comment #79
webchickCool, thanks for looking into my feedback.
Too bad about 1. :(
2. Sounds good.
3 is definitely broken. Just try resizing your browser window and you'll see the problem (at least in Firefox). However, I can concede this is probably an issue w/ Picture module, and not one with this patch.
Just banged this around again and showed it to Dries as well, and he's also +1. So!
COMMITTED AND PUSHED TO 8.X. YEAAAAHHHH!! :D
Thanks all so much for your work on this!!
Comment #80
webchickOh, but let's get a follow-up for that Picture module stuff.
Comment #81
webchickOh btw. This patch is so insanely massive because it updates CKEditor to a "dev" release. I checked w/ nod_ and he was cool with this approach. Updating to a proper release of CKEditor will be done in #2039163: Update CKEditor library to 4.4.
Comment #82
eaton CreditAttribution: eaton commentedAnd the Ewoks danced.
Comment #83
Wim Leers#79: Picture module is not at all in the picture (heh…) here, it only works for image fields that use the Picture field formatter. This is just plain standard browser behavior. Also note that desktop browsers do *not* always handle images in the same way as mobile browsers!
I can reproduce the problem you're seeing on Firefox, but not on Chrome. I also can't reproduce it in the iPhone Simulator (i.e. Mobile Safari). Nor in desktop Safari. So I'm pretty sure this behavior is limited to Firefox on desktops/laptops. Which makes sense, really: on a desktop you usually don't want content to shrink, you just scroll horizontally. Chrome/Safari/Mobile Safari seem to apply the same logic they apply on mobile devices, and continue to show the image.
Not in any way is anything responsive involved: not picture.module, not CSS with media queries, nothing!
#82: the llamas dancing on my desk join your Ewoks!
I think it might be worthwhile to create a combined change notice for #2014895: Image captions & alignment (for inline images) and this issue, no?