Task

Use Twig template instead of theme functions for defining themable HTML markup for the ckeditor module.

Steps to test manually

Navigate to admin/config/content/formats/manage/full_html

Remaining

Theme function name/template path converted
theme_ckeditor_settings_toolbar converted
CommentFileSizeAuthor
#72 interdiff.txt698 bytesstar-szr
#72 1963474-72.patch14.99 KBstar-szr
#69 1963474-69.patch15.04 KBstar-szr
#65 interdiff.txt1.16 KBjoelpittet
#65 1963474-65-twig-ckeditor.patch20.33 KBjoelpittet
#64 1963474-64-twig-ckeditor.patch15.03 KBjoelpittet
#64 interdiff.txt5.3 KBjoelpittet
#59 core-ckeditor-conf-twig-1963474-59.patch14.83 KBinfojunkie
#55 core-ckeditor-conf-twig-1963474-54.patch14.82 KBinfojunkie
#52 core-ckeditor-conf-twig-1963474-52.patch14.88 KBinfojunkie
#50 core-ckeditor-conf-twig-1963474-50.patch14.93 KBinfojunkie
#44 Screen Shot 2013-09-27 at 4.33.30 PM.png155.38 KBdead_arm
#44 Screen Shot 2013-09-27 at 11.28.01 AM.png61.24 KBdead_arm
#43 Screen Shot 2013-09-27 at 4.09.31 PM.png48.42 KBdead_arm
#42 core-ckeditor-conf-twig-1963474-42.patch12.98 KBhussainweb
#29 core-ckeditor-conf-twig-1963474-29.patch12.62 KBhussainweb
#12 core-ckeditor-conf-twig-1963474-12.patch12.66 KBnod_
#11 drupal-1963474-11.patch12.34 KBwidukind
#11 interdiff.txt4.37 KBwidukind
#8 drupal-1963474-8.patch12.49 KBwidukind
#8 interdiff.txt1.62 KBwidukind
#5 drupal-1963474-5.patch11.65 KBwidukind
#5 interdiff.txt7.01 KBwidukind
#3 drupal-1963474-3.patch11.66 KBwidukind
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +Needs manual testing

Tagging.

widukind’s picture

widukind CreditAttribution: widukind commented
Assigned: Unassigned » widukind
widukind’s picture

widukind CreditAttribution: widukind commented
Status: Active » Needs review
FileSize
drupal-1963474-3.patch11.66 KB
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

Wow! Fantastic work @widukind.

I tested this manually and I only found one glitch so far - the markup is not output when twig_debug is enabled. I think we may need to solve that outside of this issue though, looking into it now.

Below are just documentation and code standards tweaks and general commentary :)

  1. +++ b/core/modules/ckeditor/ckeditor.admin.incundefined
@@ -8,7 +8,14 @@
+ * Prepares variables for theme_ckeditor_settings_toolbar().

    Per #1913208: [policy] Standardize template preprocess function documentation and since we are removing the theme_ckeditor_settings_toolbar() function, this should be something more like:

    "Prepares variables for CKEditor settings toolbar templates." 

  2. +++ b/core/modules/ckeditor/ckeditor.admin.incundefined
@@ -18,37 +25,34 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
+  $active_buttons  = array();

    Extra space before equals sign can be removed. 

  3. +++ b/core/modules/ckeditor/ckeditor.admin.incundefined
@@ -57,7 +61,12 @@ function theme_ckeditor_settings_toolbar($variables) {
-      $value = theme('image', array('uri' => $button['image' . $rtl], 'title' => $button['label']));
+      //$value = theme('image', array('uri' => $button['image' . $rtl], 'title' => $button['label']));
+      $value = array(
+        '#theme' => 'image',
+        '#uri' => $button['image' . $rtl],
+        '#title' => $button['label'],
+      );

    Render array conversion looks good, I think you just forgot to remove this commented out line, I do this too when converting to render arrays :) 

  4. +++ b/core/modules/ckeditor/ckeditor.admin.incundefined
@@ -65,98 +74,39 @@ function theme_ckeditor_settings_toolbar($variables) {
     if (!empty($button['multiple'])) {
-     $button['attributes']['class'][] = 'ckeditor-multiple-button';
+      $button['attributes']['class'][] = 'ckeditor-multiple-button';
     }

    Great catch! :) 

  5. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twigundefined
@@ -0,0 +1,69 @@
+ * Default theme implementation for the 'ckeditor_settings_toolbar' theme hook.

    The description here should be less abstract than the theme_test.module ones, what about something like this?

    "Default theme implementation for the CKEditor settings toolbar." 

  6. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twigundefined
@@ -0,0 +1,69 @@
+ * @themeable

    @ingroup themeable per Twig coding standards. 

  7. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twigundefined
@@ -0,0 +1,69 @@
+{% spaceless %}
+<fieldset role="form"
...
+</fieldset>
+{% endspaceless %}

    Let's indent the contents of the {% spaceless %} tag per http://drupal.org/node/1823416#whitespace.

widukind’s picture

widukind CreditAttribution: widukind commented
Status: Needs review » Needs work
FileSize
interdiff.txt7.01 KB
drupal-1963474-5.patch11.65 KB

@Cottser, thanks for the review and feedback.
Made the mentioned tweaks, please see attachments.

In regards to the button picker not showing in twig-debug mode - i think it has something to do with the injected HTML comments.
See Firebug reports this exception

Timestamp: 4/13/2013 1:53:18 PM
Error: DrupalBehaviorError: attach ; ckeditorAdmin: Syntax error, unrecognized expression: <!-- THEME DEBUG -->
<!-- CALL: theme('ckeditor_settings_toolbar') -->
<!-- BEGIN OUTPUT from 'core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig' -->
<fieldset role="form"
            aria-labelledby="ckeditor-button-configuration ckeditor-button-description"><legend
        id="ckeditor-button-configuration">Toolbar configuration</legend><div class="fieldset-wrapper"><div id="ckeditor-button-configuration-aria-live" class="element-invisible"
           aria-live="polite"></div><div id="ckeditor-button-description"
           class="fieldset-description">Move a button into the <em>Active toolbar</em> to enable it, or into the list of <em>Available buttons</em> to disable it. Use dividers to create button groups. Buttons may be moved with the mouse or keyboard arrow keys.</div><div class="ckeditor-toolbar-disabled clearfix"><div class="ckeditor-toolbar-dividers"><label id="ckeditor-multiple-label">Dividers</label><ul class="ckeditor-multiple-buttons" role="form"
              aria-labelledby="ckeditor-multiple-label"><li data-button-name="|" class="ckeditor-group-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button group separator" class="ckeditor-group-separator"></a></li><li data-button-name="-" class="ckeditor-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button separator" class="ckeditor-separator"></a></li></ul></div><label id="ckeditor-available-buttons">Available buttons</label><ul class="ckeditor-buttons" role="form"
            aria-labelledby="ckeditor-available-buttons"><li data-button-name="Strike"><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></li><li data-button-name="Superscript"><a href="#" class="cke-icon-only cke_ltr" role="button" title="super script" aria-label="super script"><span class="cke_button_icon cke_button__superscript_icon">super script</span></a></li><li data-button-name="Subscript"><a href="#" class="cke-icon-only cke_ltr" role="button" title="sub script" aria-label="sub script"><span class="cke_button_icon cke_button__subscript_icon">sub script</span></a></li><li data-button-name="RemoveFormat"><a href="#" class="cke-icon-only cke_ltr" role="button" title="remove format" aria-label="remove format"><span class="cke_button_icon cke_button__removeformat_icon">remove format</span></a></li><li data-button-name="Outdent"><a href="#" class="cke-icon-only cke_ltr" role="button" title="outdent" aria-label="outdent"><span class="cke_button_icon cke_button__outdent_icon">outdent</span></a></li><li data-button-name="Indent"><a href="#" class="cke-icon-only cke_ltr" role="button" title="indent" aria-label="indent"><span class="cke_button_icon cke_button__indent_icon">indent</span></a></li><li data-button-name="Undo"><a href="#" class="cke-icon-only cke_ltr" role="button" title="undo" aria-label="undo"><span class="cke_button_icon cke_button__undo_icon">undo</span></a></li><li data-button-name="Anchor"><a href="#" class="cke-icon-only cke_ltr" role="button" title="anchor" aria-label="anchor"><span class="cke_button_icon cke_button__anchor_icon">anchor</span></a></li><li data-button-name="HorizontalRule"><a href="#" class="cke-icon-only cke_ltr" role="button" title="horizontal rule" aria-label="horizontal rule"><span class="cke_button_icon cke_button__horizontalrule_icon">horizontal rule</span></a></li><li data-button-name="Copy"><a href="#" class="cke-icon-only cke_ltr" role="button" title="copy" aria-label="copy"><span class="cke_button_icon cke_button__copy_icon">copy</span></a></li><li data-button-name="Paste"><a href="#" class="cke-icon-only cke_ltr" role="button" title="paste" aria-label="paste"><span class="cke_button_icon cke_button__paste_icon">paste</span></a></li><li data-button-name="PasteText"><a href="#" class="cke-icon-only cke_ltr" role="button" title="paste text" aria-label="paste text"><span class="cke_button_icon cke_button__pastetext_icon">paste text</span></a></li><li data-button-name="PasteFromWord"><a href="#" class="cke-icon-only cke_ltr" role="button" title="paste from word" aria-label="paste from word"><span class="cke_button_icon cke_button__pastefromword_icon">paste from word</span></a></li><li data-button-name="SpecialChar"><a href="#" class="cke-icon-only cke_ltr" role="button" title="special char" aria-label="special char"><span class="cke_button_icon cke_button__specialchar_icon">special char</span></a></li><li data-button-name="Format"><a href="#" role="button" aria-label="Format"><span class="ckeditor-button-dropdown">Format<span class="ckeditor-button-arrow"></span></span></a></li><li data-button-name="Table"><a href="#" class="cke-icon-only cke_ltr" role="button" title="table" aria-label="table"><span class="cke_button_icon cke_button__table_icon">table</span></a></li><li data-button-name="ShowBlocks"><a href="#" class="cke-icon-only cke_ltr" role="button" title="show blocks" aria-label="show blocks"><span class="cke_button_icon cke_button__showblocks_icon">show blocks</span></a></li><li data-button-name="Maximize"><a href="#" class="cke-icon-only cke_ltr" role="button" title="maximize" aria-label="maximize"><span class="cke_button_icon cke_button__maximize_icon">maximize</span></a></li><li data-button-name="Styles"><a href="#" role="button" aria-label="Styles"><span class="ckeditor-button-dropdown">Styles<span class="ckeditor-button-arrow"></span></span></a></li></ul></div><label id="ckeditor-active-toolbar">Active toolbar</label><div data-toolbar="active" class="ckeditor-toolbar-active clearfix"><ul class="ckeditor-buttons" role="form"
              aria-labelledby="ckeditor-active-toolbar"><li data-button-name="Bold"><a href="#" class="cke-icon-only cke_ltr" role="button" title="bold" aria-label="bold"><span class="cke_button_icon cke_button__bold_icon">bold</span></a></li><li data-button-name="Italic"><a href="#" class="cke-icon-only cke_ltr" role="button" title="italic" aria-label="italic"><span class="cke_button_icon cke_button__italic_icon">italic</span></a></li><li data-button-name="|" class="ckeditor-group-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button group separator" class="ckeditor-group-separator"></a></li><li data-button-name="Link"><a href="#" class="cke-icon-only cke_ltr" role="button" title="link" aria-label="link"><span class="cke_button_icon cke_button__link_icon">link</span></a></li><li data-button-name="Unlink"><a href="#" class="cke-icon-only cke_ltr" role="button" title="unlink" aria-label="unlink"><span class="cke_button_icon cke_button__unlink_icon">unlink</span></a></li><li data-button-name="|" class="ckeditor-group-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button group separator" class="ckeditor-group-separator"></a></li><li data-button-name="BulletedList"><a href="#" class="cke-icon-only cke_ltr" role="button" title="bulleted list" aria-label="bulleted list"><span class="cke_button_icon cke_button__bulletedlist_icon">bulleted list</span></a></li><li data-button-name="Cut"><a href="#" class="cke-icon-only cke_ltr" role="button" title="cut" aria-label="cut"><span class="cke_button_icon cke_button__cut_icon">cut</span></a></li><li data-button-name="NumberedList"><a href="#" class="cke-icon-only cke_ltr" role="button" title="numbered list" aria-label="numbered list"><span class="cke_button_icon cke_button__numberedlist_icon">numbered list</span></a></li><li data-button-name="|" class="ckeditor-group-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button group separator" class="ckeditor-group-separator"></a></li><li data-button-name="Blockquote"><a href="#" class="cke-icon-only cke_ltr" role="button" title="blockquote" aria-label="blockquote"><span class="cke_button_icon cke_button__blockquote_icon">blockquote</span></a></li><li data-button-name="Image"><a href="#" class="cke-icon-only cke_ltr" role="button" title="image" aria-label="image"><span class="cke_button_icon cke_button__image_icon">image</span></a></li><li data-button-name="|" class="ckeditor-group-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button group separator" class="ckeditor-group-separator"></a></li><li data-button-name="Redo"><a href="#" class="cke-icon-only cke_ltr" role="button" title="redo" aria-label="redo"><span class="cke_button_icon cke_button__redo_icon">redo</span></a></li><li data-button-name="|" class="ckeditor-group-button-separator ckeditor-multiple-button"><a href="#" role="button" aria-label="Button group separator" class="ckeditor-group-separator"></a></li><li data-button-name="Source"><a href="#" class="cke-icon-only cke_ltr" role="button" title="source" aria-label="source"><span class="cke_button_icon cke_button__source_icon">source</span></a></li></ul><div class="ckeditor-row-controls"><a href="#" role="button" aria-label="Remove last button row"
             class="ckeditor-row-remove" title="Remove row">-</a><a href="#" role="button"
             aria-label="Add additional button row"
             class="ckeditor-row-add" title="Add row">+</a></div></div></div></fieldset>
<!-- END OUTPUT from 'core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig' --> ; verticalTabs: settings.fieldset is undefined
Source File: http://d8.local/core/misc/drupal.js?v=8.0-dev
Line: 35
widukind’s picture

widukind CreditAttribution: widukind commented
Status: Needs work » Needs review
widukind’s picture

widukind CreditAttribution: widukind commented
Status: Needs work » Needs review

Looking into this a bit deeper, it appears to be the line-breaks surrounding the injected HTML comments that are causing the problem.

diff --git a/core/modules/ckeditor/js/ckeditor.admin.js b/core/modules/ckeditor/js/ckeditor.admin.js
index 7abf327..beead83 100644
--- a/core/modules/ckeditor/js/ckeditor.admin.js
+++ b/core/modules/ckeditor/js/ckeditor.admin.js
@@ -200,7 +200,7 @@ Drupal.behaviors.ckeditorAdmin = {
     if ($ckeditorToolbar.length) {
       var $textareaWrapper = $ckeditorToolbar.find('.form-item-editor-settings-toolbar-buttons').hide();
       var $textarea = $textareaWrapper.find('textarea');
-      var $toolbarAdmin = $(drupalSettings.ckeditor.toolbarAdmin);
+      var $toolbarAdmin = $($.trim(drupalSettings.ckeditor.toolbarAdmin));
       var sortableSettings = {
         connectWith: '.ckeditor-buttons',
         placeholder: 'ckeditor-button-placeholder',

Throwing a $.trim() around the generated markup seems to do the trick.

widukind’s picture

widukind CreditAttribution: widukind commented
FileSize
interdiff.txt1.62 KB
drupal-1963474-8.patch12.49 KB

Alright, this time FTW I hope.

Tweaked the JS code some more so that

(a) leading line breaks won't cause jQuery to break when parsing the given markup string
(b) the right container element gets captured for further processing

Tested in FF/Chrome/IE9, works for me.

Please review. Thanks!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -Needs manual testing +JavaScript

Tagging as JavaScript for the JS tweaks - I'm not sure if we can or should be solving those at a higher level. May assign to @nod_ for review if he doesn't see this first.

@widukind - it almost looks like you wrapped the markup in the Twig template at 80 characters. There's no need to do that, please unwrap any lines like this:

+++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twigundefined
@@ -0,0 +1,69 @@
+          <a href="#" role="button"
+             aria-label="{{ 'Add additional button row'|t }}"
+             class="ckeditor-row-add" title="{{ 'Add row'|t }}">+</a>
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

Status for the Twig template tweaks. Thanks for all your work on this @widukind!

widukind’s picture

widukind CreditAttribution: widukind commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.37 KB
drupal-1963474-11.patch12.34 KB

Unwrapped markup as requested.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
FileSize
core-ckeditor-conf-twig-1963474-12.patch12.66 KB

problem was trying to create something detached from the DOM. When we add the HTML directly to the dom then select what we want, it works fine. Twig debug or not.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Reviewed & tested by the community

I diffed the generated source, the only difference is the added ID and it supports the JavaScript fix for the twig debug output. RTBC, thanks @widukind and @nod_!

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

Add conversion summary table

c4rl’s picture

c4rl CreditAttribution: c4rl commented
Title: Convert ckeditor module to Twig » ckeditor.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling
widukind’s picture

widukind CreditAttribution: widukind commented
Assigned: widukind » Unassigned
epersonae2’s picture

epersonae2 CreditAttribution: epersonae2 commented
Status: Needs work » Needs review

#12: core-ckeditor-conf-twig-1963474-12.patch queued for re-testing.

Pierre Paul Lefebvre’s picture

Pierre Paul Lefebvre CreditAttribution: Pierre Paul Lefebvre commented
Issue tags: -needs profiling

Rerunning profiling, forgot something.

jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented
Assigned: Unassigned » jerdavis
Issue tags: +needs profiling
ernie-g’s picture

ernie-g CreditAttribution: ernie-g commented

take to 8.x

OpenChimp’s picture

OpenChimp CreditAttribution: OpenChimp commented

profiling this now

OpenChimp’s picture

OpenChimp CreditAttribution: OpenChimp commented
Issue tags: -needs profiling

=== 8.x..8.x compared (519fe63632755..519ff02bc3dc7):

ct : 40,580|40,580|0|0.0%
wt : 377,694|379,099|1,405|0.4%
cpu : 335,787|334,898|-889|-0.3%
mu : 50,206,616|50,206,616|0|0.0%
pmu : 50,271,728|50,271,728|0|0.0%

Profiler output

=== 8.x..1963474 compared (519fe63632755..519ff0874d0e5):

ct : 40,580|40,580|0|0.0%
wt : 377,694|378,089|395|0.1%
cpu : 335,787|334,637|-1,150|-0.3%
mu : 50,206,616|50,206,896|280|0.0%
pmu : 50,271,728|50,273,112|1,384|0.0%

Profiler output

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The registry has been rebuilt. [success]
'all' cache was cleared in self

jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented
Issue tags: +needs profiling

Still needs profiling as the above results have no difference in calls, looks like it wasn't pointing to the right page.

jerdavis’s picture

jerdavis
Location Minneapolis, MN
CreditAttribution: jerdavis commented
Issue tags: -needs profiling

This one's been tough to profile. The pages at admin/config/content/formats/%filter_format are actually pretty slow, and for some reason invoke a surprising number of database calls. That has made establishing a good baseline impossible. I started down the path of trying to figure out why the pages were slow, but realistically there are probably a lot bigger fish to fry out there than the filter admin page.

I'd almost suggest we skip profiling this with twig, as the page itself (without twig) is pretty slow. The switch to twig is going to be such a small difference it'd be a better use of time to debug the other performance issues here than to worry about twig.

ernie-g’s picture

ernie-g CreditAttribution: ernie-g commented
Issue tags: +needs profiling

MANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/

OpenChimp’s picture

OpenChimp CreditAttribution: OpenChimp commented

Yes. Sorry, we were not aware of the extra steps needed to set up the website so that the profiling tests would be looking at the correct page that will test the patch. I will reproduce this and run the profile again.

steinmb’s picture

steinmb CreditAttribution: steinmb commented
Status: Needs review » Reviewed & tested by the community

I can confirm the results from #24. The profiling results was is just all over the place. I suggest we move this into core as is.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Needs reroll

No longer applies, needs a reroll.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
FileSize
core-ckeditor-conf-twig-1963474-29.patch12.62 KB

Rerolled patch from #12.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work
Issue tags: -JavaScript, -Novice, -needs profiling, -Twig

The last submitted patch, core-ckeditor-conf-twig-1963474-29.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
Issue tags: +JavaScript, +Novice, +needs profiling, +Twig

#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: jerdavis » Unassigned
Issue tags: -Novice

Thanks @hussainweb, reroll looks good to me! Just waiting for testbot.

Status: Needs review » Needs work
Issue tags: -JavaScript, -needs profiling, -Twig

The last submitted patch, core-ckeditor-conf-twig-1963474-29.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review

#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, core-ckeditor-conf-twig-1963474-29.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review

#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +needs profiling, +Twig

The last submitted patch, core-ckeditor-conf-twig-1963474-29.patch, failed testing.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review

I just pulled latest from 8.x branch and ran all User tests, and they passed. It looks like some other problem and hence queuing patch from #29 for retest.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented

#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Issue summary: View changes

Update remaining

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

This needs another reroll.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
core-ckeditor-conf-twig-1963474-42.patch12.98 KB

Re-rolled. The conflict was from changes in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms and #1363112: Simplify names of "element-x" helper classes.

dead_arm’s picture

dead_arm
she/her
Primary language English
CreditAttribution: dead_arm commented
FileSize
Screen Shot 2013-09-27 at 4.09.31 PM.png48.42 KB

The patch in #42 still applies. The only output markup change with the patch applied, also verified in #13, is the id, id="ckeditor-button-configuration-container", added to the fieldset.

For reference, to see this go to admin/config/content/formats/manage/basic_html and inspect 'Toolbar configuration'
Screen Shot 2013-09-27 at 4.09.31 PM.png

dead_arm’s picture

dead_arm
she/her
Primary language English
CreditAttribution: dead_arm commented
FileSize
Screen Shot 2013-09-27 at 11.28.01 AM.png61.24 KB
Screen Shot 2013-09-27 at 4.33.30 PM.png155.38 KB

With dev tools:

Screen Shot 2013-09-27 at 4.33.30 PM.png

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Thank you very much @dead_arm I think this is good to go for profiling next:)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

This definitely needs some reworking after #1872206: Improve CKEditor toolbar configuration accessibility because the markup has changed.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

Update conversion table

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review

42: core-ckeditor-conf-twig-1963474-42.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 42: core-ckeditor-conf-twig-1963474-42.patch, failed testing.

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Assigned: Unassigned » infojunkie
Issue summary: View changes
infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Status: Needs work » Needs review
FileSize
core-ckeditor-conf-twig-1963474-50.patch14.93 KB

Re-rolled for latest version.

Status: Needs review » Needs work

The last submitted patch, 50: core-ckeditor-conf-twig-1963474-50.patch, failed testing.

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Status: Needs work » Needs review
FileSize
core-ckeditor-conf-twig-1963474-52.patch14.88 KB
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@infojunkie thanks for coming to the sprint and this re-roll looks great. I just reviewed the patch quickly and spotted a couple nit picky docblock items.

  1. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
@@ -0,0 +1,63 @@
+ * @see theme_preprocess()

    This is no longer needed. 

  2. +++ b/core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
@@ -0,0 +1,63 @@
+ * @see theme_preprocess_ckeditor_settings_toolbar()

    this should be template_preprocess...

Also wonder if it passes without spaceless as whitespace shouldn't matter and removing it doesn't really make things easier to read. Testbot may complain though, just a warning.

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Status: Needs review » Needs work
infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Status: Needs work » Needs review
FileSize
core-ckeditor-conf-twig-1963474-54.patch14.82 KB
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue summary: View changes
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: +Needs reroll

I was on to profiling this but it's back to needs re-roll. @infojunkie want to snag this?

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Status: Needs review » Needs work
infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Status: Needs work » Needs review
FileSize
core-ckeditor-conf-twig-1963474-59.patch14.83 KB

Re-rolled.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -Needs reroll

Thanks @infojunkie!

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Ok Marking this as RTBC. My test results look a bit funny but if you look at it, the 3ms and 11ms is due to PDOExecute which is unrelated. So if you take that off both times, the real WT difference is 2,716 - 3,273= -557microseconds on the first. 8.x vs 8.x and 14,840-11,887 = 2,953 microsecs on the 8.x vs ckeditor branch.

I'll have to look into fixing my.cnf or switching back to mysql or something...

=== 8.x..8.x compared (52c1d5ca86be3..52c1d613a9327):

ct  : 91,257|91,257|0|0.0%
wt  : 635,511|638,227|2,716|0.4%
cpu : 523,456|522,910|-546|-0.1%
mu  : 31,607,896|31,607,896|0|0.0%
pmu : 31,815,824|31,815,824|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c1d5ca86be3&...

=== 8.x..1963474-twig-ckeditor-theme compared (52c1d5ca86be3..52c1d65bccb31):

ct  : 91,257|91,828|571|0.6%
wt  : 635,511|650,351|14,840|2.3%
cpu : 523,456|526,530|3,074|0.6%
mu  : 31,607,896|31,677,920|70,024|0.2%
pmu : 31,815,824|31,886,296|70,472|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c1d5ca86be3&...

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

Adding steps to test manually.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Reviewed & tested by the community » Needs work

It looks like we're missing a closing </li> towards the end of the template. Doesn't need re-profiling but does need to be fixed up before we can RTBC again.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
interdiff.txt5.3 KB
1963474-64-twig-ckeditor.patch15.03 KB
  • I added
  • around the empty
      because it's inside another
        which isn't allowed.
  • Fixed the missing
  • Fixed the @see in the docblock.
  • Added trans block.
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
1963474-65-twig-ckeditor.patch20.33 KB
interdiff.txt1.16 KB

Did markup comparison, there was some whitespace to fix with the addition of trans block. So this fixes the whitespace for that.

The last submitted patch, 65: 1963474-65-twig-ckeditor.patch, failed testing.

infojunkie’s picture

infojunkie
Location Vancouver
CreditAttribution: infojunkie commented
Assigned: infojunkie » Unassigned
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

65: 1963474-65-twig-ckeditor.patch queued for re-testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: Unassigned » star-szr
FileSize
1963474-69.patch15.04 KB

Some other stuff snuck in, here is #64 combined with the interdiff from #65.

Going to do another manual test as well.

The last submitted patch, 65: 1963474-65-twig-ckeditor.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: star-szr » Unassigned
Status: Needs review » Reviewed & tested by the community

Markup and functionality is a go and I reviewed the patch, looks good to me.

(RTBC when green)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
FileSize
1963474-72.patch14.99 KB
interdiff.txt698 bytes

One line was mis-indented, fixed here.

The last submitted patch, 72: 1963474-72.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

72: 1963474-72.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Looks good! The {% spaceless %} tag name struck me as hilarious for some reason. :P

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.