Update: See comment #18 for some analysis by @wimleers of the cause of this regression.
Needs review by form system maintainers
Work around: Whilst the UI isn't saving the caption and alignment tags, it is possible to align inline images by manually adding the 'data-align' and 'data-caption' attributes to the img tag in the HTML source code.
eg:
<img alt="ALT TAG TEXT" data-align="RIGHT" data-caption="CAPTION TEXT" data-entity-type="FILE" src="/IMAGE.PNG" />
Investigation
$ git bisect start 8.0.0-beta12 8.0.0-beta11
At each step, reinstall Drupal standard, login and try to align an image in a page.
Result : 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7 is the first bad commit
Reverting changes introduced by this commit, the problem comes from the file module.
---------------
Myself and others over the past few days have noticed that no matter how many times you select Align options or try to enable Caption for an image the settings do not work.
I have seen this on someone else's machine (they were trying to blame Bartik on the frontend).
I have tried on my local install and also on Simplytest.me whilst testing a Captions related issue for Bartik.
If you select an option and press save in the modal window the settings do not apply. When you double the click on the image to try again the values are set to the default values, ie. none or unticked.
----------------
Issue summary from one of the dupes #2533738: Image align radio and caption checkbox in CKeditor does not work anymore with more details
Problem/Motivation
In beta11 it was possible to align images uplaoded in the CKEditor by selecting a radio button in the popin. Since beta12 it is not possible anymore.
Investigation
$ git bisect start 8.0.0-beta12 8.0.0-beta11
At each step, reinstall Drupal standard, login and try to align an image in a page.
Result : 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7 is the first bad commit
Reverting changes introduced by this commit, the problem comes from the file
module.
Steps to reproduce:
- $ git checkout 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7^
- Install drupal standard
- Login and create a page
- Insert an image in the body with align = right
- the image is aligned in the editor and in the saved content
- $ git checkout 8b4bc7df8f30011a7d6e56d9b575694ee2d29ad7
- Install drupal standard
- Login and create a page
- Insert an image in the body with align = right
- the image is not aligned in the editor nor in the saved content
Beta phase evaluation
Issue category | Bug because that feature used to work |
---|---|
Issue priority | Major because it's a widely used feature |
Prioritized changes | The main goal of this issue is to fix a regression |
Proposed resolution
No idea.
Remaining tasks
Fix that regression.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#54 | 2540850-after.png | 54.44 KB | DuaelFr |
#54 | 2540850-before.png | 57.03 KB | DuaelFr |
#53 | interdiff.txt | 1.19 KB | tim.plunkett |
#53 | 2540850-editor-53.patch | 6.72 KB | tim.plunkett |
#52 | 2540850-screenshot-52.png | 481.79 KB | kattekrab |
Comments
Comment #1
emma.mariaComment #2
Wim LeersYep, reproduced. Something broke this :(
It is not a JavaScript problem; it's the server's response that is missing the
data-caption
anddata-align
attributes.This is why we really need #2395377: Write integration test for EditorImageDialog.
Comment #3
Wim LeersMore duplicates reported:
I wonder if this should be critical?
EDIT: fixed links.
Comment #4
szeidler CreditAttribution: szeidler as a volunteer commentedI found out, that it seems to be related to the $image_element array in
/core/modules/editor/src/Form/EditorImageDialog.php
.When I change line 184
to
the align feature is working again. Same goes for the caption condition.
I compared the module with drupal-8.0.0-beta10 where the align/caption dialog was working. I recognized, that the buildForm() method is also called when submitting the dialog. That seems not the case in the old beta, when I debugged it correctly.
Does someones has an idea, why that is happening?
Comment #5
davidhernandezI just tested this in head. The alt text works, the manual sizing works, but the caption and alignment do nothing.
I agree with Wim; temped to mark this critical.
Comment #6
larowlanHit this in a demo, working on a test
Comment #7
Wim LeersMany thanks!
Comment #8
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedYes, I think this should be critical. It's "in your face" functionality that just doesn't work, and a regression, because I'm pretty sure it WAS working fine.
Bumping.
Core crew, feel free to disagree and bump it back.
Comment #9
larowlanFrom https://www.drupal.org/node/45111
Think that means major is even a stretch
Comment #10
larowlanShould be red/green
Comment #12
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedNicely done @larowlan
Manually tested with simplytest.me - now works as expected.
screengrab:
Comment #13
szeidler CreditAttribution: szeidler as a volunteer commentedThanks for creating the patch @larowlan. Then I identified the right thing. Also I can confirm the caption and alignment is working again out of the dialog window in HEAD.
I'm still a little bit curious why the array properties in $image_elements got lost.
Comment #14
larowlanThe form is being rebuilt and it uses the cached one from the $form_state instead of from the $user_input
> form is built
> values don't exist
> image_elements is stored in form state without those values
> form is submitted
> old values are used instead of new ones from input
fix makes sure they are stored, even if no caption or alignment
edit: this is wrong - see next comment
Comment #15
larowlanActually, that's not right
> form is built with all elements
> file upload via ajax submits to the original url (this used to submit to system/ajax) but no longer does, so in the new code-path the form is rebuilt
> in that instance those values aren't set (you upload the file first)
> so the fields aren't added to the $form
> then you submit again with all values filled
> form is retrieved from cache without those elements
> $form_builder->doBuildForm moves values from $form_state->input to $form_state->values based on fields present in the retrieved form - but those fields aren't there, so it never makes it from the input values into $form_state->values and is hence lost.
> fix is to make sure the fields are always there (if enabled) rather than only if $image_elements contains them in the incoming post values
So based on that I think #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache was the cause.
Comment #16
larowlanor it was #2263569: Bypass form caching by default for forms using #ajax.
Comment #17
Wim LeersThank you so much, @larowlan!
But, like you already wrote in #15 & #16, this is actually wrong:
None of these changes should be necessary; only when the data received from the text editor indicates that the text editor supports
data-align
and/ordata-caption
, only then, these form items should be shown.(And in core specifically: when using only the caption filter OR only the align filter, then the text editor will also only support either, and this form should match that. That's also how we can expand the test coverage to check that.)
Comment #18
Wim LeersSo, here's another complete analysis. That will hopefully allow a form system maintainer to help us out here.
Step 1: open image dialog
Confirmed that the initial building of the form works correctly. The values the form receives are the ones that
Drupal.editors.ckeditor.openDialog()
sends viaThe server receives this:
EditorImageDialog
only cares about theeditor_object
part there, it does this:Note that it wants to remember the data it got from the editor via Form State.
Step 2: upload image
New form state is built. The input now originates from the form, and because a new
FormState
is built, we don't haveimage_element
anymore:Step 3: when saving
When we hit "Save", data analogous to that in the previous step is sent, i.e. again without
editor_object
. Form state is recreated again, and is thus again missing the original data we stored in$form_state->set('image_element', $image_element
like we did in step 1.Because of this,
::buildForm()
omits thedata-align
anddata-caption
form items, and hence also their values, and hence also doesn't send them back in its response.In other words: we literally show form fields to the user that only exist in the first step, when submitting the form, the form is rebuilt and then resubmitted, with the form system blissfully unaware that these form fields even exist, and hence their values are not sent back.
I know that this form does quite unconventional stuff, but that's because it needs to integrate with unconventional stuff. I'm hoping a form system maintainer can give some feedback on how to resolve this!
Comment #19
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedComment #20
DuaelFrI just closed #2533738: Image align radio and caption checkbox in CKeditor does not work anymore as duplicate as this one has seen more work done. Someone not browsing from a mobile could copy the IS and beta evaluation from the mentioned issue and improve it using the content of #18.
Comment #21
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedComment #22
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedComment #23
larowlanhoping @tim.plunkett might know answer to #18
Comment #24
larowlanlooks like @kattekrab already did the issue summary update
Comment #25
larowlanno beta eval needed for major bugs, we can always fix them :)
Comment #26
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedbartik captions component testing blocked by this issue - adding as related.
#2398459: Clean up "captions" component in Bartik
Comment #27
Wim Leers@kattekrab This does not block that issue at all, see #2398459-26: Clean up "captions" component in Bartik.
Comment #28
kattekrab CreditAttribution: kattekrab at Creative Contingencies commented@Wim Leers oh good! thanx :)
Comment #29
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedUpdated issue summary with a work around - editing the source manually to include data-align and data-caption attributed in the img tag.
As suggested by @jibran at https://www.drupal.org/node/2546456#comment-10189750
Comment #30
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedComment #31
tim.plunkettI haven't made any progress on this, but I wanted to chime in to say I am actively working on it.
Comment #32
StuartJNCC CreditAttribution: StuartJNCC commented+1 tim.plunkett
Whilst I agree with larowlan in #9 that it doesn't meet the criteria for "critical", I also agree with #8 that this is such an "in your face" bug that it is going to be encountered almost immediately by anyone reviewing or trying out Drupal. Consequently, it ought to be a release blocker. Not sure how to tag it to express that formally!
Comment #33
DuaelFrI totally agree.
Tagging as Release blocker to keep everyone's attention on that issue :)
Comment #34
Wim LeersOnce Tim is able to pinpoint the root cause, I solemnly swear I'll write the integration test coverage that would've prevented this regression from being introduced in the first place! #feelingguilty
Comment #35
larowlanFwiw I still think 10 is valid
Comment #36
Wim Leers#35: it's not valid, see #17.
Comment #37
kattekrab CreditAttribution: kattekrab at Creative Contingencies commented- making @DuaelFr's git bisect investigations more prominent in the IS.
Comment #38
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedwith the link to the commit this time
Comment #39
Wim Leers@DuaelFr++
Adding #2500527: Rewrite \Drupal\file\Controller\FileWidgetAjaxController::upload() to not rely on form cache as a related issue and left a comment on that issue linking to this regression.
Comment #40
effulgentsia CreditAttribution: effulgentsia at Acquia commented#2521820-22: Update CKEditor library to 4.5.3 says this blocks that issue, so promoting to Critical.
Comment #41
xjmThanks everyone for identifying this issue.
The bug, while indeed nasty, is not in itself critical.
Can we clarify how/why this blocks #2521820: Update CKEditor library to 4.5.3? I can understand the desire to fix it before committing that in order to maybe ensure the regression does not get compounded, or because the changes would conflict, but the worst-case scenario should still be that we update CKEditor without this being fixed, and create RC1 without this being fixed. Thanks!
Comment #42
tim.plunkettI am also interested in the response to #41.
----
I tried a bunch of other fancier things, but every time I tried to distill the patch down to its simplest form, it kept coming back to the fact that
$form_state->getUserInput()['attributes']
and$form_state->getUserInput()['editor_object']
had exactly the same values but at different times during the request.Manual testing with this patch should show it works.
Note the use of #parents in the code (not seen in the patch) that set it to use 'attributes'.
Comment #43
Wim Leers#41: Drupal 8 extends CKEditor's
image2
plugin, and even in a layered manner:core/modules/ckeditor/js/plugins/drupalimage/plugin.js
core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js
(This was implemented and designed in close collaboration with the CKEditor team.)
When we updated to CKEditor 4.4, we had to almost completely rewrite those plugins on top of
image2
(before that they were written on top ofimage
), see #2039163: Update CKEditor library to 4.4.If we commit CKE 4.5.3 without being able to also thoroughly test Drupal 8's extensions to
image2
, then we risk breaking even the JS, making it even harder to fix this bug.Comment #44
tim.plunkettGiven this flow:
Steps 2 and 3 could also be reversed.
Before the AJAX changes, $form_state was cached and would have $form_state->get('image_element') in it.
In certain cases, the stored image_element can get stale anyway, and the formBuilder->processForm() call in FileWidgetAjaxController happens to handle pulling the correct values from user input.
In HEAD, we don't have the extra processForm() call. But we do have 'attributes' in the user input, and this change lets the values automatically get mapped to their respective elements.
Comment #45
tim.plunkett@Wim Leers pointed out that this fix is only coincidental, and does not work for the hasCaption portion. I'm not even going to try to figure out why it worked for some and not others.
Why can't we just mark this form as cached and restore the old functionality? I need to dust off some brain cells before determining if this is actually okay, or someone else can chime in.
$form_builder->buildForm() does two very important things that this test is missing:
It sets the requestMethod on FormState, and it populates the user input from the request.
However, I'm getting a "LogicException: The database connection is not serializable." when serializing the form, which has fileStorage, which has database (the connection).
The changes other than the setCached() in EditorImageDialog are from Wim, thanks!
The interdiff is from @larowlan's test
Comment #46
tim.plunkettAlternately, we could do this, if wanting to continue to bypass buildForm()
Comment #49
tim.plunkettUnlike the other entity handlers that extend from EntityHandlerBase, none of the implementations of EntityStorageSchemaInterface use a high level base class.
Many extend from SqlContentEntityStorageSchema, which stores a connection to the database. However, it does not use DependencySerializationTrait. \Drupal\file\FileStorageSchema was the object blowing up those tests.
Comment #50
larowlanShould we wrap this in a
!isset($image_element)
and add$image_element =
to the start of$form_state->set('image_element', $form_state->getUserInput()['editor_object']);
to save the extra ->get call, or is that micro-optimizing/deck chair shuffling?We need a manual tester here now.
Comment #51
kattekrab CreditAttribution: kattekrab at Creative Contingencies commenteddid someone say manual testing? On it.
Comment #52
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedHooray! It works!
Also tested using quick edit, and editing the image once saved.
Thanks everyone!
Comment #53
tim.plunkettDeck chairs shuffled!
Thanks @kattekrab for manually testing.
Wim and I tested manually as well.
Comment #54
DuaelFrI also tested manually and as the testbot is happy and the patch respects coding standards and includes a extensive documentation in comments, I have the immense pleasure to mark this as RTBC.
Thanks to everybody for that fix!
Before :'(
After \o/
Comment #55
webchickYay, SO happy this got fixed in time for RC1!!
Committed and pushed to 8.0.x. Thanks!
Comment #57
kattekrab CreditAttribution: kattekrab at Creative Contingencies commentedYaaaaaay!!!!!!!
Hi-5s all round!
This bug made me feel really stupid when I first hit it - like I must have been doing something wrong. So it's really great to see it fixed.
Thanks so much everyone :-)