Problem/Motivation
- Moves markup to the template removing dependancy of SafeMarkup from being needed.
- Fixes a markup boo-boo missing ending UL
User interface changes
N/A
API changes
TBD
Original report by @yukare
In the page admin/config/content/formats/manage/full_html and maybe on another filter formats, we have raw html in the form where the toolbar for ckeditor is:
Move a button into the Active toolbar to enable it, or into the list of Available buttons to disable it. Buttons may be moved with the mouse or keyboard arrow keys. Toolbar group names are provided to support screen reader users. Empty toolbar groups will be removed upon save.
Available buttons
<a href="#" class="cke-icon-only cke_ltr" role="button" title="strike" aria-label="strike"><span class="cke_button_icon cke_button__strike_icon">strike</span></a><a href="#" role="button" aria-label="Styles"><span class="ckeditor-button-dropdown">Styles<span class="ckeditor-button-arrow"></span></span></a><a href="#" class="cke-icon-only cke_ltr" role="button" title="underline" aria-label="underline"><span class="cke_button_icon cke_button__underline_icon">underline</span></a>
Comment | File | Size | Author |
---|---|---|---|
#142 | ckeditor_image_button_rtl.png | 194.09 KB | aneek |
#140 | safemarkup_in_ckeditor-2306515-140.patch | 17.09 KB | hussainweb |
#132 | safemarkup_in_ckeditor-2306515-132.patch | 26.09 KB | siva_epari |
#129 | ckeditor-twig-macros-2306515-129.patch | 25.94 KB | piyuesh23 |
#124 | ckeditor-twig-macros-2306515-124.patch | 25.67 KB | aneek |
Comments
Comment #1
dawehnerFor now, you have to wrap these inline HTMLs with a new SafeMarkup() object
Comment #2
cs_shadow CreditAttribution: cs_shadow commentedBumping the priority since this is broken in latest HEAD.
Comment #3
Wim LeersSo #1825952: Turn on twig autoescape by default broke this (and many other things), and fixing all the brokenness is being tracked at #2297711: Fix HTML escaping due to Twig autoescape.
Comment #4
l0keHere is the patch. Not quite sure if it's a good solution but it works. See attached screeenshot.
Comment #5
Wim LeersIt's perfect, thank you :)
Comment #6
alexpottCommitted baacf9d and pushed to 8.x. Thanks!
Comment #9
chx CreditAttribution: chx commentedThis needs to be redone and SafeMarkup::set removed.
Comment #10
aneek CreditAttribution: aneek commented@chx,
Is it a good approach to remove
SafeMarkup::set
and introduceinline_template
? I've made that change inckeditor.admin.inc
file and it seems to be working as expected.Can you please have a look at the attached patch and then lets decide that whether it's a good approach to mitigate and resolve this issue.
Btw, there was a
</ul>
missing in the ckeditor-settings-toolbar.html.twig template.Comment #11
aneek CreditAttribution: aneek commentedJust removed the unused
use Drupal\Component\Utility\SafeMarkup
from file. Attached fixed patch.Comment #12
cs_shadow CreditAttribution: cs_shadow commentedComment #13
joelpittetHere's another approach that I believe we should really take.
It's not complete and there are a few templates strings that need to be moved to the template and the image uris need to be passed to the button array. Though I hope you agree this looks much more approachable?
Also this uses the translated label for the title/alt instead of the image_alternative string, which could be another fix, imo.
Using a macro to reduce the template duplication: http://twig.sensiolabs.org/doc/tags/macro.html
Comment #15
aneek CreditAttribution: aneek commented@joelpittet, this may be a more convincing way, can you please complete the file and re-queue for review?
Comment #16
joelpittetRan out of time this evening, you're welcome to post improvements. If not I'll finish this up likely over the weekend.
Comment #17
aneek CreditAttribution: aneek commentedOkay, I'll try to work on it :-)
Comment #18
Wim LeersWOW :O
WOW :O
WOW :O
I had no idea this was possible! So this uses http://twig.sensiolabs.org/doc/tags/macro.html… This is indeed an excellent fit for this problem. So much cleaner!
+1 for this alternative solution.
I updated the issue title accordingly.
@joelpittet: Thank you so much for this nudge in a much better direction! I don't know about everybody else, but I had no idea this existed :) AFAICT this would be the first use of Twig Macros in Drupal core, and I'd be happy/proud to have CKEditor be the first :)
Comment #19
aneek CreditAttribution: aneek commented@joelpittet,
I had a quick look and one major issue I found is with
Drupal\Core\Render\Element::children
. Plugin elements are not properly being rendered there. While a bried debugging I found, ckeditor-multiple-button, separator & ckeditor-button-separator elements are not rendered as an array but string.Also, to convert the Image URI to URL do you think $base_url can be used? Please have a look at the attached interdiff file. I also think we don't require directional settings in time of image rendering.
button.image ~ direction == 'rtl' ? '_rtl'
was not working and even if this works then the resulting image will have.png_rtl
extension.I'll try to fix these issues but needs a bit of time, let me know if I can help you in this.
Comment #20
m1r1k CreditAttribution: m1r1k commentedComment #21
Wim Leers#19: never ever ever ever use
$base_url
orbase_path()
. It prevents altering file URLs to serve them from a CDN. For files, always usefile_create_url()
.Comment #22
aneek CreditAttribution: aneek commented@Wim, thanks. I almost forgot about this :-(
Will keep in mind from next time though.
Comment #23
Wim LeersNo problem :) I have to keep reminding everyone of that, including core committers :)
Comment #24
joelpittetYeah that caught me too with the url() vs file_create_url() before, trying to get that in Twig for assets: #2168231: Twig Functions needed in templates and I think there is another issue trying to do this as well.
I think this needs to be built in the preprocess for now and passed in on the button associative array for now. So you have the right idea just add an if statement above the $button_item to check for and generate the url need for the src.
Thanks for spotting that!
Hope that helps put you on a track, thank you for pitching in @aneek
Comment #25
aneek CreditAttribution: aneek commentedThanks @joelpittet.
Let's solve the big issue with the render() function in
Drupal\Core\Render\Element::children
. Elements in that function expects array but some cases due to the modification ofbreakes what is expected. Any thoughts on this regards?
Thanks!
Comment #26
joelpittetI think the big render issue is because left some HTML strings in the image_alternate values over in getButtons method.
The rtl image src should be just built as it was before but instead of using #theme => image we just need the url.
Take this uri, use file_create_url() to produce the URL for the image source, and pass it to the $button_item.
Comment #27
aneek CreditAttribution: aneek commentedYes, you are right.
This bit of code in
/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Internal.php
is one of many that is causing this issue.
Just finished fixing another issue (#2309929) and its already very late at night so I'll look into it tomorrow along with the image URI fix. :-)
Thanks again.
Comment #28
joelpittet@aneek thanks give it a shot tomorrow, I'll be on in the afternoon and likely give this another shot after your go at it.
Comment #29
aneek CreditAttribution: aneek commented@joelpittet,
I had done some changes today and as of the UI point of view its fine. But needs a thorough logical review. Below are some main points I tweaked.
$button['image'] = file_create_url($button['image' . $rtl]);
{{ button.image_alternative|replace({" " : ""}) }
Uploading a patch and an interdiff for your review. Let me know your thoughts.
Thanks! :-)
Comment #30
cs_shadow CreditAttribution: cs_shadow commentedRemember to set the status as 'Needs review' every time you submit a patch so that the testbot can pick it up and run tests.
Comment #31
aneek CreditAttribution: aneek commented@cs_shadow, I do know that, but thanks for reminding me. Actually it's as of now only for logical review. Once the logical review is done by others and we finalize the right approach then I thought to make it ready for "Review" for the bots.
Thanks!
Comment #32
cs_shadow CreditAttribution: cs_shadow commented@aneek, ok, alright. I didn't knew that.
Comment #33
joelpittetThanks @aneek, i'll see if I can't finish it.
Going to remove the "Clone $button and remove unwanted array keys for HTML rendering." The extra context data won't hurt the template.
Comment #35
joelpittet@aneek I wish we didn't have to introduce 'button_text' but I see why you did that, so thank you for pointing that out. If it could be just button.label it could almost be less conditionals, but I think I got it down to the bare minimum at the moment in the macro.
I changed the variable to image_url, so we still have access to the image variable(for whatever reason) and it's clear what the variable is.
@Wim Leers This may very well be, but it has to beat out #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template, which I love as well because it's also a recursive macro from @dawehner!
This works well from a quick test, but needs a bit of another set of eyes on the logic like @aneek mentioned and a manual testing.
Comment #36
joelpittetTesbot engage! (Dreditor should add a little paperclip to the bottom right corner.)
Comment #37
joelpittetI think I offended it somehow... re-uploading.
Comment #38
aneek CreditAttribution: aneek commented@joelpittet,
Applied latest patch and working as expected. Thanks, for making me a part of this issue. It was a good learning experience with twig macros and CkEditor settings.
Btw, can we do good logical & manual review and a performance review if needed?
Comment #39
joelpittet@aneek you sure can. I added Needs manual testing. In this case, it would be good to render that page before and after the patch is applied (remember to clear cache) You should see in the markup the Labels being used instead of the alternative_image text after the patch as a hint to know you are seeing the different page.
There are many ways to do markup comparison, I usually pump the results into a diff tool and take a screenshot:) See here: https://www.drupal.org/node/1987424#comment-8242319
Once something along those lines have been done you can remove that tag from here and post your findings. (crosses fingers the changes hold up)
Comment #40
joelpittetAnd! thank you very much for helping out on this as well! We likely don't need a performance review on this as it's admin facing and we're opting out on things we don't expect to have a significant change in admin facing twig changes.
You can see markup comparisons on nearly every twig conversion issue in #1757550: [Meta] Convert core theme functions to Twig templates
Would like to get someone who wasn't working on the patch to do a logic look over but you're welcome of course to review the logic and let me know any concerns you see. I think I may have had hand even on that missing
<ul>
tag in the first place :SComment #41
aneek CreditAttribution: aneek commented@joelpittet, I'll surely have a look at the way you mentioned, and report if something is missed or needs correction. I totally agree some one as third person should have a review on this.
Thanks!
Comment #42
Wim LeersROFL @ #36 & #37 :D
This looks great! Thank you so much for working on this — this looks much better than what I could've come up with! Just minor stuff:
This
$rtl
variable is confusing. I'd call it$suffix
.This seems to be the only documentation here. Seems kind of lost?
It's not okay to special case these, because contrib will add more buttons. We'll need a flag in the
ckeditor.admin.inc
PHP (e.g.isDropdown
) that the Twig code can use instead.This one *is* okay, because the separator is provided by CKE, and other CKE plugins won't add more of these.
…? :)
What does this do? Perhaps could use a comment?
Comment #43
aneek CreditAttribution: aneek commented@Wim Leers,
Just a thought, can't we move the macro code in a twig file and inport it? Say "ckeditor-buttons-macro.twig" and import it to the main template file? And that file can have the required documentation.
Like,
So the code will be more clearer I guess.
About your point # 5, this is added by default by CKE, if no buttons are found rather no image_alternative or button.image_url are found then all the button texts are shown as "?".
On point #3, don't you think the buttons should supply "isDropdown" flag from the getter methods (getButtons)? So if we make just a small change in the template by removing
button.image_alternative in ['format', 'styles']
and making it justbutton.isDropdown == true
orbutton.isDropdown == false
then we can reduce all the static logical calls.What are your thoughts on these?
Comment #44
aneek CreditAttribution: aneek commentedjust a quick interdiff for more references. It only consists of the new template creation for the macro code.
Comment #45
joelpittet@Wim Leers all great points in #42 I agree with all of them.
@aneek want to take a stab at them? Regarding #43 you can and if it's useful to other templates that makes a bunch of sense but for now let's just keep it inline and add a comment to what it does. Maybe referencing the twig docs on the matter. Keeping it in the same file gives it the context. Splitting the template seems to disassociate them and only see that being a good way to go if it's re-usable.
Comment #46
aneek CreditAttribution: aneek commented@joelpittet: thanks, I'll surely work on it, this week. One point though, isn't that be good if we use
isDropdown
as attributes (array keys) to the elements that need the dropdown type button? Just like,So later on if any contrib modules pass the same then we can have a dynamic way to generate
<span class="ckeditor-button-dropdown">
.In macro code we will only change the conditions accordingly.
May be you can have a better idea to do this. What are your thoughts in this regards?
Comment #47
joelpittet@aneek yes that is good I think but be consistent with naming convention there, we use _ separators not camelCase. Though dot syntax makes that camelCase look nicer we should be consistent still.
Comment #48
Wim LeersIndeed, let's keep the macro in the same file for context (though I really liked the import statement in #43 — makes it much clearer what that import is for), and +1 to #46 and #47 (
is_dropdown
).Comment #49
aneek CreditAttribution: aneek commentedThanks @joelpittet & @Wim Leers for sharing your valuable suggestions. I'll start implementing a new patch and post it soon. :-)
Thanks!
Comment #50
aneek CreditAttribution: aneek commented@joelpittet & @Wim Leers,
While working on the patch I've made a few changes.
is_dropdown
to the elements where needed. And also added'is_separator' => TRUE,
. It's unlikely that other modules will add separators but its to make the conditions in twig more specific.same as
test. http://twig.sensiolabs.org/doc/tests/sameas.htmlSo the code becomes,
Let me know if you have any feedbacks.
Thanks!
Comment #51
Wim Leers#50: you forgot to attach the patch? :) Both points look fine to me :) (Joël will be better able to answer your second point.)
Comment #52
aneek CreditAttribution: aneek commented@Wim Leers, no it's not complete yet. Once done I'll upload it. Also awating Joel's suggestions.
Thanks!
Comment #53
joelpittet@aneek You can post a patch, we'll read it just the same, but feedback on #50
1. That makes sense to me so far.
2. It's much simpler, we aren't using strict variables in twig for this very reason.
{% if button.is_dropdown is same as(true) %}
becomes
{% if button.is_dropdown %}
Comment #54
aneek CreditAttribution: aneek commented@joelpittet, thanks for the feedback. I'll post a patch soon enough for review.
Comment #55
aneek CreditAttribution: aneek commentedFew observations, if we change the language direction from LTR to RTL.
image_rtl
instead ofimage
in respectivegetButtons()
methods. Resulting "?" to appear in the UI. This needs to be fixed in thegetButtons()
methods.$button['language_direction']
. So usages of'image_alternative_rtl' => TRUE
is not required in the respectivegetButtons()
methods.This
"cke-icon-only cke_{{ button.direction|default('ltr') }}"
needs to be"cke-icon-only cke_{{ button.language_direction|default('ltr') }}"
to show the language changes properly.I'll upload a patch by tomorrow for you guys to review.
Thanks!
Comment #56
aneek CreditAttribution: aneek commented@joelpittet & @Wim Leers,
Please find the attached patch & interdiff for logical review.
Let me know if any further fixes are needed. :-)
Thanks!
Comment #57
aneek CreditAttribution: aneek commentedComment #58
joelpittetI think everything you did except the two things mentioned below are spot on. Just need a bit of clarification as I couldn't put two and two together in these cases:
How does it know that the rtl exists? Maybe the scope of this went a bit far or maybe this is an improvement? ... hard to tell
Some of these had image alternatives that are RTL (image alternatives is usually a CSS sprite or something it seems) But I don't think all of them have this, I couldn't notice how you accounted for these removals.
Comment #59
aneek CreditAttribution: aneek commented@joelpittet,
Please find the explanations below.
About #1: If you check the CKEditorPluginButtonsInterface (/core/modules/ckeditor/src/CKEditorPluginButtonsInterface.php) image_rtl is mentioned as
What I've done is, I just took the default language direction from the configuration and appended to the image URI. My thought was we may not need RTL specific images for these buttons (link, unlink & image).
So if the language direction is RTL then image URI becomes
'image_rtl' => $path . '/link.png'
.Other modules may explicitly specify "image_rtl" if they need different images for the buttons depending on language change. Yes, you may call this is as an improvement. But if you don't have this and change the language direction in your site, then you will not get the image buttons working and will see "?" instead of images. (Refer to attached screenshot on #55)
About # 2: Again if you follow the interface CKEditorPluginButtonsInterface, you will see that its written,
So yes you are right about they using sprite images or something. As an example, in the CkEditor Toolbar, "BulletedList" button has different images for image_alternative and image_alternative_rtl. Since in your code from #34 patch, you already taken care of this on this line,
And I just changed in twig from,
to,
In this case "button.direction" was an empty variable and I thought it should be "button.language_direction". The main difference it makes is toggling the HREF class "cke-icon-only cke_ltr" for LTR or "cke-icon-only cke_rtl" for RTL direction. So do we actually require to add the key in getButtons() method?
I hope these explanations above clears your doubts? Let me know your thoughts.
Thanks!
Comment #60
Wim Leers#58.1/#59.1: These changes are also factually wrong because no RTL variant used to exist for them, it still doesn't exist, it doesn't need to exist, but the code changes make it seem like they do or may exist. They also make a call into global state (current language direction), which should be avoided.
Why not use the previous behavior? Default to
image
, notimage_ltr
, which can then be used for both LTR and RTL — because not all buttons need a RTL version.If the concern is that Drupal favors LTR in its code, well, that's something that happens everywhere (e.g. in CSS), and we shouldn't start changing that here.
#58.2/#59.2: Indeed, these were using a sprite. The changes described in #59.1 indicate that a
image_ltr
version must be present, which is an API change that we can and should avoid, for the reasons mentioned above.I hope that all made sense and that you agree with it — ping me on IRC if something isn't clear!
Comment #61
aneek CreditAttribution: aneek commented@Wim Leers, had a discussion with @joelpittet about the implementation and about your comments. Here is the summery.
@joelpittet, please add your thoughts if I've missed something.
Comment #62
aneek CreditAttribution: aneek commented@joelpittet, created a patch to check the image button load if the language is changed from En to Ar. So the language directions will also change.
This patch will product 3
Undefined index: image_rtl
in the test result. Please have a look.Soon, I will post the fix patch and the testonly patch for further reviews.
Comment #63
aneek CreditAttribution: aneek commentedComment #65
aneek CreditAttribution: aneek commentedUploading patches for the fix.
Added a new element in
$button
array as "language_direction_class" which will handle the LTR to RTL language direction conversion and'image_alternative_rtl' => TRUE
key as well.@joelpittet & @WimLeers, please review and let me know your thoughts.
Thanks!
Comment #66
aneek CreditAttribution: aneek commentedComment #68
joelpittetWow, your test is awesome!
It goes to the page and picks up 3 rtl images broken:) I like how it doesn't assert anything, well truthfully that makes me uneasy a bit but it still does a great job of showing the error!
Maybe add one assert to what you expect to be on the page for the rtl button to be working?
And one thing I think was mentioned before but may need @Wim Leers to confirm, the buttons shouldn't need to define an rtl version (especially the same ltr version) we should fallback to the ltr image if there is no rtl defined.
So pseudo simplified logic:
Comment #69
aneek CreditAttribution: aneek commentedComment #70
aneek CreditAttribution: aneek commentedJust had a go on the test case again. This time I tried to add xpath() check to get the faulty image. Suggested by @joelpittet.
What I was trying to do is to get the IMG SRC from one of the images that comes in CKEditor toolbar. So I did the following,
But I was not able to get the
SRC
attribute. @chx and @berdir helped me in this. While debugging, @berdir found out that the toolbar was not actually generated via HTML but with JavaScript. A code snippet looks likeSo I removed the xpath checks and added assertPattern() instead.
This patch checks for if any *_rtl.png is present in the RAW HTML. But, if we follow the algorithm mentioned in #68 then this test case also needs alterations.
@joelpittet & @Wim Leers any thoughts on this one?
Comment #71
aneek CreditAttribution: aneek commentedComment #73
aneek CreditAttribution: aneek commentedCreated a patch considering @joelpittet's pseudo code from #68. The test doesn't assert anything though but gives warnings as before.
Logical code review needed.
Thanks!
Comment #74
aneek CreditAttribution: aneek commentedComment #76
Wim LeersFor now, only a review of the test coverage — full review will come though!
s/Definition of/Contains/
Missing
{@inheritdoc}
Don't use
entity_create()
. UseFilterFormat::create()
,Editor::create()
, etc.You don't need to assign these newly created entities to a variable, since you're not using them anywhere anyway. And you can invoke
->save()
upon them immediately.Unnecessary newline.
This comment has a typo and can be removed.
I don't see any actual tests here yet? :)
There must be a newline after the last method's closing brace and the class's closing brace.
Comment #77
aneek CreditAttribution: aneek commentedThanks @Wim Leers, I'll work on the test again to fix these issues. Btw, please have a look at comment #70 for the test and why its not working with xpath() and images.
Let me know your thoughts on this.
Comment #78
lauriiiRerolled, fixed the language direction classes and added assertion for the test.
Comment #79
aneek CreditAttribution: aneek commented@lauriii, thanks.
Can you please provide us an interdiff? Also, have you addressed review comments by Wim Leers? Lets have a quick look into this. Such as not not using entity_create and using FilterFormat::create() methods.
I haven't had the time, but I will work on this weekend. :-)
Comment #80
lauriiiI fixed the points pointed by Wim. It is not possible to create interdiff for reroll because the original patch doesnt apply
Comment #81
joelpittet@lauriii, I think #78 is just a re-roll and you are working on the changes from #76?
Comment #82
lauriiiI don't know what has happened there since im pretty sure I fixed those other points also.. Now they are there.
Comment #83
lauriiiComment #85
lauriiiComment #86
joelpittetHere's some improvements, removed some extra bits that I expect aren't needed.
I still need to review the inside the macro with a bit more rigor but we'll see what testbot says here.
I tried to make the code about the language direction more clear and just refactor it a bit @aneek.
Also, due to how that language direction check for image works, we don't need to provide the exact same image to the 'image_rtl' key if it's the same as the 'image' key. Both with the way you had it and my refactor. So I removed those additions.
Comment #87
aneek CreditAttribution: aneek commented@joelpittet, the re-factor is good. I will also have a good look later on.
Comment #88
joelpittetThanks @aneek, this feels really close and a relief it is green:)
There is a few minor bits here that I missed on that go, you're welcome to roll them in on your review if you have a moment?
One space indent needed here that I missed.
Should probably read "A list" as we are purposefully avoiding data types in twig.
https://www.drupal.org/node/1823416#datatypes
Don't think this exists anymore.
Comment #89
lauriiiComment #90
aneek CreditAttribution: aneek commentedThanks @lauriii for addressing the reviews. I had a quick look and it seems all are quite okay.
@joelpittet, should we wait for more reviews by others or can we make this RTBC?
Comment #91
Wim LeersVery sorry for the delay in reviewing this!
s/URI/file URI/
The function name says it's for building a button item. This says it's a button.
You probably want to update the function name to be
$build_button()
rather than$build_button_item()
.From
CKEditorPluginButtonsInterface::getButtons()
's documentation:This is not an HTML string.
You basically moved all of this into the Twig file. I didn't realize it before, but this is very problematic, because what you moved into the Twig template is not generically usable; it's only usable for buttons that are part of the CKEditor build!
It's entirely possible that an additional CKEditor plugin is installed that also needs the ability to specify a HTML string to simulate its button in the admin UI, but because it's not built in to the CKEditor build that Drupal 8 ships with, it can't use this same mechanism, because it needs different HTML.
However, I don't want to block the awesome work you've done here.
So I think we should keep
image_alternative</ code> as it works in HEAD, and then add a new <code>internal_button
.So, from:
to:
I hope that made sense.
For the same reason, this his highly problematic. Would need to become
internal_button_has_rtl
.These are very much approximations of what the real deal looks like. Why can't these use
image_alternative
and pass in HTML, just like in HEAD?These descriptions are wrong.
Create text format, associate CKEditor.
Let's remove this empty line. That makes it more clear the comment above applies to both of these statements.
Once the default language is changed, go to the tested text format configuration page.
s/twig/Twig/
s/to create the CKEditor buttons/to generate the HTML for internal buttons (those included with CKEditor) — we can't use regular image URLs here because these images are part of a sprite. By generating the correct HTML, we can use the images in that sprite, just like actual CKEditor instances./
Comment #92
lauriiiComment #93
subhojit777Comment #94
subhojit777Marking this issue as fixed. There is no raw HTML in text format setting pages, and it is working without applying any patch. May be some other commit have resolved this issue (I guess this one #2324371: Fix common HTML escaped render #key values due to Twig autoescape). Image attached for reference.
Comment #95
joelpittet@subhojit777 thanks for testing, though this patch does a bit more than just fix the auto-escape issue so I think it still needs to be considered as we've been working hard to make this easier to use.
Needs an issue summary update though and may need to be split into a couple patches if the approach is not viable because there is a bug fix in there as well.
Comment #96
joelpittetComment #97
joelpittetComment #98
jibranComment #99
Wim LeersThanks lauriii, much better already :) Almost there:
Trailing space and 80 col rule.
s/uses Twig macro/uses a Twig macro/
s/Indentifier/Identifier/ :)
Should also include the text I proposed for the second substitution for point 10 in #91.
See #91.5. I think we may need to keep
image_alternative
around for that reason.(It is possible for a non-internal CKE button to not have an image but require an alternative HTML-based presentation, but with this change, you're saying that's only allowed for buttons included in our CKEditor build).
Comment #100
subhojit777Comment #101
subhojit777@Wim Leers Uploading patch with corrections you suggested in #99
@lauriii correcting your spelling mistakes :p
Comment #103
aneek CreditAttribution: aneek commented@subhojit777, thanks for the patch. I am not sure I am getting the following,
I don't see any implementation of this image_alternative in theme layer. I am a bit confused here. @Wim Leers, can you help me with understanding this implementation of
image_alternative
thing?Thanks!
Comment #104
Wim Leers#103: From #91:
Comment #105
aneek CreditAttribution: aneek commentedThanks @Wim Leers, for the explanations. About the patch #101, just by adding RAW HTML to
image_alternative
won't serve the purpose. Just had a checked at the failed test results and in/core/modules/ckeditor/src/Tests/CKEditorTest.php
@ line 98 the values which are being compared are not same in the variables$expected_config
&$this->ckeditor->getJSSettings($editor)
. Please have a look at the pastes for more reference.$expected_config
and
$this->ckeditor->getJSSettings($editor)
So based on the two debug dumps, we have a difference,
We may need to address addition of "image_alternative" in a different way. I hope this may help to create a new patch. If I get some good time today or tomorrow I may make one. But other are welcome too !! :-)
EDIT #1
I think even we pass raw HTML, it should pass via
SafeMarkup::checkAdminXss()
.Thanks.
Comment #106
subhojit777@Wim Leers @aneek is there any way I can run these tests locally?
Comment #107
aneek CreditAttribution: aneek commentedDefinitely, just enable the testing module in your local D8 installation. And then run the tests from configuration menu.
Comment #108
Wim LeersSo something changed in this patch that allows
image_alternative
to make it into JS settings.This is the culprit. It's simply a wrong hunk, and it causes the problem described in #105.
Comment #109
subhojit777@Wim Leers I dont get this :S You have asked to add
image_alternative
in #99. So finally do we needimage_alternative
or not. Sorry if I am not being able to understand something straightforward.Comment #110
aneek CreditAttribution: aneek commentedWhile having a look at the code and the total thread, these following came to my mind.
1. In patch #101, the place where "image_alternative" is added is not quite right. Instead of adding it to
getConfig()
method it should be placed ingetButtons()
method. All these deals withCKEditorPluginButtonsInterface::getButtons()
interface.2. @Wim Leers, on your review #91, you suggested to change
'image_alternative' => 'bold',
to'internal_button' => 'bold',
and to keepimage_alternative
to send HTML as is the HEAD version. The main idea was, if any external plugin supplies raw HTMLimage_alternative
then this button needs to be printed. So I am guessing thatinternal_button
only supplied via the internal buttons from Drupal core. If this is the idea then external plugins should not supplyinternal_button
in theirgetButtons()
methods.So the template "ckeditor-settings-toolbar.html.twig" code may have,
Regarding the language directions, if
image_alternative
has to be present then why we want to deleteimage_alternative_rtl
. This is same as theimage_rtl
. External plugins may supply different class based HTML or different markups based on different RTL versions.3.
image_internal_has_rtl
this key doesn't have any implementation in the twig template or in the preprocess function to generate the buttons. If we want to introduce new key then there should be one implementation of the key also. Isn't it?4. While checking, I found out that the system deals with only "internal" & "stylescombo" as the internal plugin. The others that are shown in the toolbar is treated as external plugin like "drupalimage" or "drupallink" etc. It may also happen that other external plugins can supply "internal_button" key in their
getButtons()
method. So the name is quite confusing. Can it be changed to other like "text_button" or simply "default_button"? See, currently we have 3 types of buttons,isInternal()
method). So I suggest to change the name to another.@Wim Leers, please suggest your thoughts. Also, love to hear from other contributes as well.
Comment #111
aneek CreditAttribution: aneek commentedComment #112
aneek CreditAttribution: aneek commentedComment #113
lauriiiI'm not a professional on this one. This is the only issue I've been working on CKEditor. I'm sure that @Wim Leers is better one to answer most of the questions.
In my patch #92 I replaced
image_alternative_rtl
withimage_alternative_has_rtl
so they should have the same functionality. Anything haven't been removed at least intentionally.I also think that button_internal is a bit confusing and we need to find something better for that.
It would be nice if someone could summarize the discussion here to the issue summary.
Comment #114
aneek CreditAttribution: aneek commentedThe patch removes multiple calls to
SafeMarkup::set()
method fromtemplate_preprocess_ckeditor_settings_toolbar()
function.Comment #115
aneek CreditAttribution: aneek commentedPatch #101 doesn't apply now. Something has changed in
ckeditor.admin.inc
Comment #116
YesCT CreditAttribution: YesCT commentedactual tag has no dash.
Comment #117
ankitgarg CreditAttribution: ankitgarg commentedRerolled.
Comment #119
ankitgarg CreditAttribution: ankitgarg commentedRerolled
Comment #121
joelpittetCreated a followup #2409817: CKEditor toolbar configuration UI missing ending UL for the UL that was being fixed here but is really not part of this issue. It was in #101 and it was lost in #117. So we can just leave it out of this issue and fix it in the other.
This still needs a period at the end, not sure where that went from the reroll.
Comment #122
adci_contributor CreditAttribution: adci_contributor commentedFixed the corruption in #119 patch file.
Updated in according to comment #121.
Comment #124
aneek CreditAttribution: aneek commentedNew patch with some changes.
Sorry for the interdiff though. Can't generate it with patch #122.
Review!!
Comment #125
aneek CreditAttribution: aneek commentedComment #128
davidhernandez#2329781: Move CKEditor toolbar classes from preprocess to templates got in a few weeks ago, so the templates will at least need some rerolling.
Comment #129
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedRe-rolled. Uploading the patch here.
Comment #131
rpayanmComment #132
siva_epari CreditAttribution: siva_epari commentedPatch rerolled.
Comment #133
siva_epari CreditAttribution: siva_epari commentedComment #134
siva_epari CreditAttribution: siva_epari commentedSorry. Added wrong related issue by mistake
Comment #136
YesCT CreditAttribution: YesCT commentedthere is overlap between this issue and #2501711: Remove or document SafeMarkup::set in template_preprocess_ckeditor_settings_toolbar
Comment #137
joelpittetOk that safe markup was committed today! We should see where this is at now. The template we were going to use a macro for was turned into an inline template, which is cool too. Maybe we can just re-roll and remove the macro bit and ensure we didn't miss anything that we've done in here that could be of use?
Comment #138
Wim Leers#137 sounds excellent!
Comment #139
aneek CreditAttribution: aneek as a volunteer commented@joelpittet, what are your thoughts with this patch now? Can you give me some details on this please. :)
Comment #140
hussainwebRerolling and following comments from #137. I hope this is what you meant.
Comment #142
aneek CreditAttribution: aneek as a volunteer commentedI've compared with #2501711: Remove or document SafeMarkup::set in template_preprocess_ckeditor_settings_toolbar and all seems to be working except the the language direction change effects the image buttons. If the language is changed to RTL then image buttons disappear. Also, all the other buttons show inline texts inside them. Refer to the attached image.
To solve this I am opening a new issue, and we will try to close the above described there. If we can find we are missing something in here that was not committed with #2501711: Remove or document SafeMarkup::set in template_preprocess_ckeditor_settings_toolbar we can keep this open else I don't think we need this issue anymore. Please correct me if I am wrong.
Thanks.
Comment #143
aneek CreditAttribution: aneek as a volunteer commentedComment #144
Wim Leers@aneek++
@aneek++
@aneek++
@aneek++
@aneek++
Comment #145
aneek CreditAttribution: aneek as a volunteer commented@Wim Leers & @joelpittet do we need this issue any more? Can we close it?
Comment #146
Wim LeersI think we can mark it as a duplicate of #2563543: CKEditor Toolbar - various button issues when the language is RTL?
Comment #147
aneek CreditAttribution: aneek as a volunteer commentedAs discussed with @WimLeers, lets try to find anything from the existing patch (code and test cases) that we might have missed. Like the issue #2563543: CKEditor Toolbar - various button issues when the language is RTL. If we don't find any issues then lets make this issue a duplicate of #2563543: CKEditor Toolbar - various button issues when the language is RTL.
I'll have a go this weekend or may be soon. Others are also welcome!!
Thanks :)
Comment #148
Wim Leers@aneek Is there anything left to actually do here? AFAICT the bulk of the work was actually done in #2563543: CKEditor Toolbar - various button issues when the language is RTL, right? Hence marking this as minor.
Comment #149
Wim LeersAnd consequently, I think this can be deferred to 8.1.
Comment #150
aneek CreditAttribution: aneek as a volunteer commented@WimLeers, yup, I think we can better close this issue. A lot of new things were done in this issue but those are covered in more simpler ways in #2563543: CKEditor Toolbar - various button issues when the language is RTL & #2563543: CKEditor Toolbar - various button issues when the language is RTL. I also checked the test files in this issue. All seems to me is covered.
So, closing this issue as this is now duplicate of #2563543: CKEditor Toolbar - various button issues when the language is RTL & #2563543: CKEditor Toolbar - various button issues when the language is RTL
Thanks to all of the contributors in this issue. :-)
Comment #151
Wim LeersHurray! :)