Problem/Motivation

When an administrator drags in the "Image" button for his text editor configuration, indicating that he wants users to be able to insert images, then the filter settings for the filters enabled for the current text format should be updated to allow for that. (Typically: add the <img> tag to the list of allowed tags of the filtered_html filter.)

Proposed resolution

Implement such a "unidirectional editor configuration -> filter settings syncing" update mechanism. It must also work when JS is disabled.

In other words: an improvement to the Editor module: when the text editor's configuration changes, the user should automatically be notified which tags are going to be added to the list of allowed HTML tags (and preferably attributes will also be supported, as well as the automatic enabling of filters required by CKEditor plugins if they need them) — this was already discussed at #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration but postponed until this issue, see comments 40, 41, 42, 43, 45 and 47.

Remaining tasks

Start from #1890502-2: WYSIWYG: Add CKEditor module to core and make it better.

User interface changes

TBD

API changes

TBD

Original report by Wim Leers

From #1890502-2: WYSIWYG: Add CKEditor module to core:

Notes about "editor config <-> filter settings syncing":

  • This was already discussed to some extent in the Editor.module issue, see #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, comments 40, 41, 42, 43, 45 and 47.
  • The syncing only needs to be unidirectional: FROM editor configuration TO filter settings. quicksketch believes so, and I agree.
  • Ideally, we'd have a nicely abstracted API that allows text editors to indicate that a feature requires A) certain HTML tags (and attributes) and/or B) a certain filter to function at all.
  • We could have a nice and simple JS API, but this also needs to work when JS is disabled. So AFAICT the calculations should happen on the server side, through Drupal form #ajax handling, so that it can easily also work when JS is disabled.
  • Editors are free to build whatever kind of configuration for they need, hence we have no standardized way of knowing what is allowed when, hence it's necessary to extend \Drupal\editor\Plugin\EditorInterface to extract the relevant data from the current form state.
  • Similarly, filters have free-form settings forms and settings data structures, so either we'll have to add metadata or an enforced structure to filter settings, or editor.module will have to extend filter.module's callbacks with new ones to work. I took the latter approach for now.

UI changes:

  • On text format configuration pages, you can select the "CKEditor" text editor, and upon selecting it, a drag-and-drop (yet currently inaccessible) toolbar configuration UI appears. If buttons you've added to the toolbar require HTML tags that are not yet allowed, then they will automatically be added to the list of allowed tags.
    CKEditor configuration to filter settings syncing.png
CommentFileSizeAuthor
#70 unidirectional_editor_to_filter_syncing-1894644-70.patch63.51 KBWim Leers
#70 interdiff.txt812 bytesWim Leers
#69 unidirectional_editor_to_filter_syncing-1894644-69.patch63.51 KBWim Leers
#69 interdiff.txt15.07 KBWim Leers
#66 Screenshot_6_10_13_1_11_PM-3.png161.82 KBjessebeach
#64 unidirectional_editor_to_filter_syncing-1894644-63.patch63.48 KBWim Leers
#63 unidirectional_editor_to_filter_syncing-1894644-62.patch63.59 KBWim Leers
#63 interdiff.txt8.48 KBWim Leers
#60 interdiff.txt11.39 KBquicksketch
#60 unidirectional_editor_to_filter_syncing-1894644-60.patch60.16 KBquicksketch
#55 interdiff.txt4.51 KBquicksketch
#55 unidirectional_editor_to_filter_syncing-1894644-55.patch59.61 KBquicksketch
#52 unidirectional_editor_to_filter_syncing-1894644-52.patch59.77 KBWim Leers
#52 interdiff.txt31.28 KBWim Leers
#51 unidirectional_editor_to_filter_syncing-1894644-51.patch34.03 KBWim Leers
#51 interdiff.txt6.75 KBWim Leers
#50 unidirectional_editor_to_filter_syncing-1894644-50.patch35.26 KBWim Leers
#43 unidirectional_editor_to_filter_syncing-1894644-43.patch34.92 KBWim Leers
#43 interdiff.txt1.87 KBWim Leers
#41 tag-message.png48.49 KBquicksketch
#40 updated message.png13.33 KBWim Leers
#40 unidirectional_editor_to_filter_syncing-1894644-40.patch34.81 KBWim Leers
#40 interdiff.txt864 bytesWim Leers
#35 unidirectional_editor_to_filter_syncing-1894644-34.patch34.89 KBWim Leers
#35 interdiff.txt2.74 KBWim Leers
#33 unidirectional_editor_to_filter_syncing-1894644-32.patch35.35 KBWim Leers
#33 interdiff.txt17.01 KBWim Leers
#31 unidirectional_editor_to_filter_syncing-1894644-31.patch26.49 KBWim Leers
#31 interdiff.txt8.54 KBWim Leers
#31 allowed tags message.png21.16 KBWim Leers
#30 unidirectional_editor_to_filter_syncing-1894644-30.patch24.6 KBWim Leers
#30 interdiff.txt6.11 KBWim Leers
#13 unidirectional_editor_to_filter_syncing-1894644-13.patch23.04 KBWim Leers
#13 1894644-first_stab_at_comment_9-do-not-test.patch2.44 KBWim Leers
#11 ckeditor_load_after_ajax_crash.patch21.18 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Postponed
Wim Leers’s picture

The patch I wrote and posted at #1890502-2: WYSIWYG: Add CKEditor module to core, which provides an initial (but working!) version of this functionality, includes the ability to work when JS is disabled.

The CKEditor guys pinged me about this issue (paraphrased, Wiktor Walc, wwalc on d.o):

1) What if you could have a single invisible instance of CKEditor with all plugins loaded, running in the administration area
that could return a full hash table (object) with information like this:

[button] => allowedContent (HTML tags, classes, styles, attrs)

Would that do the trick?

[…]

I'm trying to understand what API we need to offer in http://dev.ckeditor.com/ticket/9829

I replied that that sounds nice, but that we can't rely on it because this page must also work when JS is disabled…
They asked why this is the case, if there's a specific device requirement behind this? I replied with " all of Drupal must work without JS, unless it's impossible to do so". Which is true, mostly at least.

Then I remembered… that I myself argued that a no-JS version of this administration stuff is nonsensical since CKEditor/any WYSIWYG editor requires JS by design. quicksketch agreed.
See #1872206-1: Improve CKEditor toolbar configuration accessibility and #1872206-2: Improve CKEditor toolbar configuration accessibility.

In e-mail, quicksketch noted that not every text editor might want to use a fancy drag-and-drop toolbar configuration, which is true, so that's why I wrote it in PHP, and made it dependent on (metadata in) PHP.

The downside is though, that it's impossible to capture the metadata in PHP (i.e. button FOO requires HTML tags BAR and BAZ). e.g. the "Bold" button can map to either the <strong> or the <b> tag, depending on configuration. It's impossible (and completely insane) to duplicate all configuration logic that CKEditor has in its JS, and of each CKEditor plugin. Hence we currently rely on the by definition inaccurate metadata in PHP

All of this effort, while CKEditor can readily provide us with the necessary metadata, with the sole requirement that JS must be enabled.

So… do we really need this to work without JS? Especially in light of the fact that when JS is disabled, the CKEditor toolbar configuration will already consist of text input fields containing JSON that must be modified by hand?
Again, does it really make sense to require a no-JS-compatible configuration form for a JS-only feature (every WYSIWYG editor requires JS)?

My vote goes to leverage CKEditor's metadata and writing some JS logic to automatically update the list of allowed tags. Nothing in PHP.


Which brings us to the second aspect, which is not CKEditor-specific. Providing an API to do what this issue says ("Unidirectional editor configuration -> filter settings syncing") is going to be very hard. It essentially boils down to having text editors publishing which tags and attributes they need to be allowed, and whether they depend on a specific filter, and then somehow have filters be updated. The latter implies that filters need to become "smarter" when they're being configured as well…

I think that's all 1) very hard to solve, 2) impossible to do well when we're only integrating with a single WYSIWYG editor.

It would bring added requirements to both text editor modules and filter modules beyond the essentials; all for the sake of easier administration.

That's why I think it's better to make CKEditor have nice/deep integration, but not force contrib text editor modules to the same thing. It would be setting too stringent requirements on such modules (requiring quite a bit of JS to be written); they can do it if they want (for better UX), but there won't be an API for it, so they'll have to do it in their own custom way, like CKEditor module.

Similarly, it should be possible for filter modules to listen for events, and if they want to, automatically configure themselves (in JS) based on the CKEditor configuration. But it shouldn't be necessary.

In short: the CKEditor module should ensure that the allowed HTML tags are updated automatically. But do we really need to go much further than that? Is that really a must-have?

Wim Leers’s picture

Issue summary: View changes

Pull in more relevant information from #1890502: WYSIWYG: Add CKEditor module to core's issue summary.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Postponed » Active

To be able to do what's described in #2, we need to update the included build of CKEditor. Patch at #1905424: Update CKEditor library.

sun’s picture

I wonder how we'll integrate this with the filter_html_image_secure filter + other possible filters in contrib? (like HTMLPurifier)

For starters, the events/API should definitely belong to Editor module (not ckeditor.module).

wwalc’s picture

Just to not forget: on a text format with no security filter enabled (where "Limit allowed HTML tags" is not enabled in default installation), like the default Full HTML it might be worth to consider setting config.allowedContent to true in CKEditor, if one selects CKEditor as a text editor.

Otherwise there will be a pretty strange situation where:

Wim Leers’s picture

The upstream support for this has been completed at http://dev.ckeditor.com/ticket/9994. This is not yet available in the CKEditor version bundled with Drupal core though.

Wim Leers’s picture

As per #6, updating CKEditor library to be able to build this functionality: #1937484: Update CKEditor library to 4.1 RC and remove CKEditor default style configuration.

Wim Leers’s picture

Issue tags: +sprint

Working on this now! :)

quicksketch’s picture

This is probably early to make suggestions on something that doesn't exist quite yet, but this patch should also probably be responsible for adjusting the default button configuration when you first enable CKEditor on a text format. For example if you decide to enable CKEditor on the "Restricted HTML" format, by default CKEditor should *not* include the image button. It should read the available tags and narrow down its editor based on those tags. However adding the image button individually would of course allow the IMG tag. Basically my suggestion is that just enabling the editor shouldn't immediately update your available tags.

Wim Leers’s picture

That's a great suggestion! Thanks :) +1

(And it's great to have that before it's mostly complete, because this will most likely affect how it's built.)

If it's supposed to read the allowed HTML tags (and attributes), then this patch will have to depend on #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings, which introduces more Filter system metadata to allow code to know for each text format which HTML tags and attributes are allowed.

Wim Leers’s picture

Patch available soon. Currently debugging a CKEditor problem. Posting a hidden patch here so the CKEditor developers and I can test it on simplytest.me.

Wim Leers’s picture

LOL! So unchecking the "list" checkbox has zero effect in d.o comments apparently. Yay.

That's a WIP, so please ignore #11 for now.

Wim Leers’s picture

Implemented what was described in #2. Changes:

  • New: editor.admin.js. Contains Drupal.editorConfiguration, with (added|removed)Feature() functions, both of which trigger events (drupalEditorFeature(Added|Removed)) on the document. Both of them accept a Drupal.EditorFeature object, which comes with extensive docblocks. One such object for each "feature" (feature == button, most often) to describe what HTMl that feature needs to work. I won't cover the details of that here. Such an object can hold all metadata related to required (essential) HTML tags/attributes/styles/classes, as well as the allowed (might be generated if you let it) HTML tags/attributes/styles/classes. In core, we don't currently have a use case for the allowed things, but contrib may very well have.
  • In CKEditorPluginManager: getButtonsPlugins(Editor) -> getButtonsPlugins() (the parameter was unused).
  • CKEditor::settingsForm() now builds the CKE configuration for a hidden CKE instance that contains all available buttons and loads all plugins.
  • In ckeditor.admin.js, there's matching code in getCKEditorFeatures() to asynchronously get a list of all CKEditor features. This retrieves the features metadata in CKEditor's own format and then translates them to Drupal.EditorFeatures, this is passed asynchronously to a callback function. We then set up broadcasting: whenever a button is added or removed, we call Drupal.editorConfiguration['(added|removed)Feature'](feature), i.e. with a Drupal.EditorFeature object as parameter.
  • New: filter.filtered_html.admin.js, which listens to the drupalEditorFeature(Added|Removed) events and, upon submit, from the feature metadata of only those features (buttons) that were added, extracts the required tags, extracts the tags that are already allowed, opens a modal (currently a window.confirm because the Dialog API is not yet usable) informing the user which tags will be added. If the user clicks "cancel", then the tags won't be added to the list of allowed tags.
  • Testing all of the above on the case "text format does not yet have CKEditor enabled" caused me to run into what seemed to be a CKEditor bug: when CKEditor's JS is loaded via AJAX (i.e. a dynamically created <script> tag), it would crash. This led to a >3 hour root cause investigation. In short (and it really can't be shorter than this): CKEditor determines its relative path by looking at its <script> tag, but it couldn't find that tag because jQuery before version 1.9 — see http://bugs.jquery.com/ticket/11795#comment:20 — automatically strips inserted <script> tags! Drupal 8 is only on jQuery 1.8.1, so until we have 1.9, we'll see this bug. Then I tried to work around this by setting window.CKEDITOR_BASEPATH via type=inline JS, which also breaks on AJAX requests because AjaxResponse doesn't handle inline CSS and JS yet… ARGH! Libraries don't allow one to use drupal_add_html_head() so that's also out of the question. Hence the only solution was a very evil one: modify editor.module to have some inline JS that sets window.CKEDITOR_BASEPATH. It's guarded by a big @todo comment. (NOTE: this is why those strange #11 & #12 comments exist.)

This all sounds scary, but it's really not that much: just give it a try on simplytest.me! :)


#4:

  • I don't think we can integrate with filter_html_image_secure in any meaningful way? Can you clarify what you would expect to happen there?
  • As you said, by providing generic events, each filter can provide some admin JS (that depends on the editor.admin lib) that listens for editor.module events (which each text editor module is responsible for triggering). If it doesn't do that, then there's no nice updating.

#5: config.allowedContent is handled by #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings.


#9: I started implementing this, but this is somewhat harder than one would initially think. With everything except for this implemented, the API is really simple: the text editor's configuration JS should call Drupal.editorConfiguration.addedFeature() when the user has added a feature/button, and Drupal.editorConfiguration.removedFeature() when the user has added a feature/button.
By adding this, we must also:

  1. be able to detect when you're configuring a certain text editor for the first time for a given text format — I'd hacked this together by checking whether the current CKEditor toolbar buttons configuration matches the default CKEditor toolbar buttons — easy :) This could be done more elegantly though. Now we need to figure out which HTML restrictions exist *without* hardcoding for the filtered_html filter.
  2. do something like Drupal.editorConfiguration.registerRestrictingFilter() for each FILTER_TYPE_HTML_RESTRICTOR type filter. Then, whenever the user manually modifies the list of allowed tags, we must call something like Drupal.editorConfiguration.updateRestrictingFilter().
  3. upon loading the CKEditor configuration form (which may happen later, since the text format may not yet have CKEditor enabled as a text editor), it must know which restricting filters exist, and then it could adapt the default configuration to match the current HTML restrictions (this is the part that you described). So, it should probably call something like Drupal.editorConfiguration.getRestrictingFilters.
  4. with all of the above combined, we'd have all we need. But it'd require quite a bit more code, and a lot of state tracking.

This is also against the unidirectionality that we already agreed upon and is so explicitly present in this issue title :)

All of the above for a fairly small UX win. Hence I'd like to defer this to a follow-up issue, because I expect there will be quite a lot of feedback on the current changes already. Unless everybody magically agrees with what I'm doing so far, then I'd be more or less okay with adding that here as well.

I've included the first steps I'd taken to implement this in the attached "first stab" patch.

wwalc’s picture

It looks like using the invisible instance of CKEditor is discussable, so let me shed more light on why this idea came to my mind.

At the very beginning I think there was an idea to provide static meta files along with CKEditor that could describe certain features, like icons defined by plugins and so on. The problem is that it is impossible to do so once we realize that it's an application which is highly configurable (and which behavior can be modified by 3rd party plugins).

Let's take the Bold button: it can be easily configured to use <strong>, <b> or even <span> with font-weight.
This simple sample shows that shipping any kind of meta information with CKEditor (the editor itself) would be pointless. The correct mapping between a particular instance of CKEditor can be only retrieved using JS.

...and that's it.

Using invisible instance ensures the following:
- the data (mapping between a feature and generated tags) is always as accurate as possible, it works even if someone alters the default CKEditor configuration through it's own module
- there is no data duplication, so there is a reduced risk that plugins.js will define something different than outdated meta information in a 3rd party module that is just a wrapper for a JS plugin

There is always more than one way to implement something, so the other possible approach that could help in getting rid of an invisible instance would be returning a tag that is required/produced e.g. in Internal.php / getButtons() (it will be inaccurate however, once user changes config.coreStyles_bold etc.). If we consider changing config.coreStyles_bold as an edge case, then things will start to be more simple.

quicksketch’s picture

Thanks @wwalc for the explanation of the hidden CKEditor instance. Based on Wim's description, I have to say I was rather skeptical. Reading the actual code, it doesn't look *too bad*, but this gets into some pretty tricky manipulation.

- the data (mapping between a feature and generated tags) is always as accurate as possible, it works even if someone alters the default CKEditor configuration through it's own module

Yeah I get it. Though I think the number of places where a custom module is going to modify settings that affect the tag list would be minimal. In a hypothetical approach where the tag registry were duplicated in each CKEditor plugin's PHP definition, if a module were to modify a plugin's behavior, they could also modify the plugin's tag list through hook_ckeditor_plugin_info_alter().

- there is no data duplication, so there is a reduced risk that plugins.js will define something different than outdated meta information in a 3rd party module that is just a wrapper for a JS plugin

This is certainly true. This approach eliminates data duplication between the definition provided by JS and PHP.

I'm sure Wim has weighed this alternative already, but to give a fair representation of the alternative (duplicating the tag registry in PHP), the current approach also has these apparent downsides:
- Loading a CKEditor instance with all plugins enabled on the configuration form of course means increased page weight. I believe this would also result in a somewhat frail configuration page, since if any plugin (enabled or not) had an error in it, the whole interface for configuring your editor would break because it's all JS-driven. Though this situation should only occur during development or with dubious contrib modules, it will occur for users sooner or later.
- This JS-only approach also means we have an apparent ability to really enforce that any required tags are actually allowed. We can't do server-side validation because the PHP side of things doesn't have any idea of the requirements between the plugins and the filters.

---

Actually looks like Wim already outlined these arguments in #2, so there's really nothing new here.

To clarify my position, I had stated to Wim that providing a JS-only integration would be acceptable, but I had anticipated feeding the JS-implementation from a PHP-based registry based on the individual plugin definitions and hook_ckeditor_plugin_info_alter(). The comments above are probably correct when they say that it's "impossible" to replicate the true filtering settings on the PHP side given all the complexities of CKEditor's filtering system, but it would probably be simple enough to come up with a really close approximation. Enough so that we could meet our requirements at least.

But all that may be moot if this approach actually works already. It sounds like Wim managed to work around the hard parts effectively enough, though with a few hacks. If the approach in general is acceptable then I'll support it.

I'll give the patch a general review soon. Even if I don't particularly like the way CKEditor tags are retrieved there's still a lot going on here in editor module's general approach for notifying requirements between filters.

quicksketch’s picture

quicksketch’s picture

(For some reason this patch isn't applying for me on my localhost. Might be my problem though. Having testbot confirm.)

(EDIT: It's just me. Testing now.)

quicksketch’s picture

Status: Needs review » Needs work

Overall this gets the job done. Nice!

Some caveats I've found so far:

- This seems to only work for specific buttons that have immediately known tags. I dragged in the strikethrough button and the tags adjusted fine. However I dragged in the Styles dropdown and then added "h1.title|Title" to the list of styles, but an <h1> tag was not added. We'll need to add JavaScript to specifically handle the style settings with an onchange event that parses the list.
- The confirmation dialog box is pretty obtrusive. Being confronted with it the user may select the wrong option too easily. Although it's not stated quite like this, in 99% of scenarios box basically is saying, "Press OK to have things work. Press Cancel to have things broken." If you do press cancel, the form is submitted anyway (not the behavior I'd expect from a Cancel button).
- If you clicked Cancel and then return to the form later, there is no indication that anything is incorrect. Saving the form a second time does not give you the same warning, despite that things are still not going to work if a tag is missing.

I haven't thought of a perfect solution to this confirm box problem, but here's a possibility to consider: Instead of an alert box at all, notify the user immediately when a new tag is going to be added when a new button is dragged in. We could follow the pattern of tabledrag.js and use a warning (or status) message above the toolbar configuration when a new tag is required. Instead of adding the tags on submit, we could add the tags immediately to the filtered HTML list. If the user so chose, they could manually remove these tags. The user saves the form (no warnings during or after save). When the user returns to the configuration form, we immediately show a warning message above the toolbar configuration saying that a particular tag must be added. Alternatively a less-intrusive option yet would be to show these messages near the tag filter text field. That way they don't distract the user while building the toolbar and the message for fixing/confirming the tag list is placed in a relevant location.

Doing this would require similar functionality that is required by #9, where the filters would need to inform the editor about their current state at page load time, and the editor would respond in some way (either by removing disallowed buttons or showing a warning that the current buttons are not all going to work).

+    // @todo D8 remove this when we upgrade to jQuery >=1.9: jQuery <=1.9 breaks
+    // CKEditor when CKEditor is dynamically loaded, because jQuery <=1.9 hides
+    // <script> tags.
+    // This cannot live in ckeditor.module, because it must already exist
+    // *before* ckeditor.js is dynamically loaded, and AjaxResponse does not yet
+    // support inline CSS/JS, so moving this into the 'ckeditor' library also
+    // does not work.
+    // @see http://bugs.jquery.com/ticket/11795#comment:20
+    '#attached' => array(
+      'js' => array(
+        array(
+          'type' => 'inline',
+          'scope' => 'header',
+          'data' => 'window.CKEDITOR_BASEPATH="' . url('core/misc/ckeditor/', array('absolute' => TRUE)) . '";',
+        ),
+      ),
+    ),

We need to find a way to move this hack to ckeditor.module. Even if it's temporary it shouldn't need to live in Editor module. In the event that this doesn't get removed, we don't want it floating around in Editor module during the D8 cycle.

Overall this works great. I'm still surprised by the general approach but it works effectively enough.

With having this functionality in place I *immediately* dropped into a pattern where I expected the toolbar buttons to always adjust the filter settings for me. However I got into trouble after clicking the Cancel button. I have a habit of *always* clicking Cancel the first time I encounter any dialog; then I do the thing again to make sure it's "safe" then I confirm the second time. Not sure if that's a general human behavior towards computers or not, but it caused a lot of grief for me trying to be "safe" because the form went ahead and saved anyway after I clicked Cancel (just without the required tag). The second time I came back to the form to fix the problem I created by clicking Cancel was not intuitive to fix because it would have required adjusting a textfield I didn't even look at the first time I was on the form. I expected that by configuring the toolbar again it would fix my problem with tags. I even tried removing the Strikethrough button and adding it again, hoping that would fix the problem with the missing <s> tag. It's funny that even though I *knew* how to solve the problem by typing in the <s> tag manually, my intuition told me that since I caused this problem through the toolbar configuration, I should fix it through the toolbar configuration. However the only way to do this currently is to take out the Strikethrough button; Save; Edit again; then add the Strikethrough button. Now the <s> tag is added for me properly upon save (and this time I was sure to hit OK instead of Cancel).

So overall looks great. We just need to clean up the hack in any way possible and improve the behavior. I think #9 is important to implement also, otherwise we're going to end up causing users to accidentally increase allowed tags just by enabling an editor.

Wim Leers’s picture

Thanks for the metric ton load of feedback :)

- This seems to only work for specific buttons that have immediately known tags. I dragged in the strikethrough button and the tags adjusted fine. However I dragged in the Styles dropdown and then added "h1.title|Title" to the list of styles, but an
tag was not added. We'll need to add JavaScript to specifically handle the style settings with an onchange event that parses the list.

Great point! I wish we could not have edge case logic for this. But indeed, by design, this is problematic. The problem here is that dragging in the button doesn't mean anything yet: it's a configurable feature.
Either we can do what you suggest: add JavaScript to parse this setting, but then we'd have created a new set of problems: each configurable CKEditor plugin would need to come with additional JavaScript! Plus, there would need to a boatload of extra event handling etc.
A simpler solution would be to do this: whenever a configurable plugin of CKEditor is modified (change event listeners on the form elements for configuring CKEditor plugins), kill the hidden CKEditor instance, re-instantiate it with the new configuration, retrieve the metadata for this instance, broadcast any changes

Thoughts?

The confirmation dialog box is pretty obtrusive. […]

No argument there. As I said, I went with window.confirm for now because the Drupal 8 Dialog API is not yet ready. This part needs UX love. It's also the simplest part of all, it's currently 1 line of code :)
This part needs to be drastically better, I just went with the MVP for now.

Instead of adding the tags on submit, we could add the tags immediately to the filtered HTML list. If the user so chose, they could manually remove these tags.

That would definitely be easier :) It's possible to do that.
Please keep in mind though, that if you first add the Table button, then remove it, then no Table button tags will be added. It tracks which things buttons were *added*, and then only adds those buttons' tags. We can still make that work with the above though.

Alternatively a less-intrusive option yet would be to show these messages near the tag filter text field. That way they don't distract the user while building the toolbar and the message for fixing/confirming the tag list is placed in a relevant location.

This I like a lot. It would allow us to always instantaneously update the list of allowed tags, and notify only users that care which tags have been added, yet always immediately giving the user the option to remove tags that they don't want to be allowed, without ever forcing them to be aware, as is currently the case because of my window.confirm ugliness.

Shall I go ahead and implement it this way, then, and get rid of the stupid confirmation box? That would also solve the below, no?

[…] I have a habit of *always* clicking Cancel the first time I encounter any dialog […]

We need to find a way to move this hack to ckeditor.module. Even if it's temporary it shouldn't need to live in Editor module. In the event that this doesn't get removed, we don't want it floating around in Editor module during the D8 cycle.

The only way we can possibly have this in ckeditor.module is by adding a hook_form_alter() implementation to ckeditor.module. That actually sounds a lot better than what I have right now. Agreed?

I think #9 is important to implement also, otherwise we're going to end up causing users to accidentally increase allowed tags just by enabling an editor.

I agree that it's important, but I've already explained why it's non-trivial.
Your "otherwise" claim is wrong. Enabling an editor does *not* cause tags to be added. Only when a user actively *drags* a button into the active toolbar, the list of allowed tags will be updated. So your (rightful) concern here is fortunately not necessary.
Hence I'd like to ask once again for this to be deferred to a follow-up issue.

wwalc’s picture

To be honest I was expecting more or less the same thing as quicksketch.

My first impressions was: nice! let's add a table button. Once I dragged the button, I checked "Allowed HTML tags" field to see if I should do anything there. Unfortunately the <table> tag was not there (and other tags like td/tr).
Then I started wondering if it works at all ;p

So a gentle information that a recently dragged new button may result in new tags being added automatically to "Allowed HTML tags" list could be handy (replaced by an accurate information about added tags once the information is known and once the "Allowed HTML tags" field is dynamically updated). Imho it would be good at least for those unaware users, that got used to the fact that the set of tags needs to be correct and will check that immediately after changing the toolbar, before pressing the submit button.

+1 from me for displaying permanently a warning to the user, when he enters the administration section with CKEditor enabled and some required HTML tags by enabled buttons/features are somehow not on the Allowed HTML tags list.

Not sure if we care about the browser support at this moment, but it seems that it works in Chrome, while in Firefox I get no alert at all, same in IE9. IE9 seems to be a different story though because I even do not get the toolbar builder when I switch the editor to CKEditor in Restricted HTML format.
Chrome is the right browser to check things in general?

Wim Leers’s picture

Yep, we're all in agreement then :)

As I said:

Alternatively a less-intrusive option yet would be to show these messages near the tag filter text field. That way they don't distract the user while building the toolbar and the message for fixing/confirming the tag list is placed in a relevant location.

This I like a lot. It would allow us to always instantaneously update the list of allowed tags, and notify only users that care which tags have been added, yet always immediately giving the user the option to remove tags that they don't want to be allowed, without ever forcing them to be aware, as is currently the case because of my window.confirm ugliness.

Shall I go ahead and implement it this way, then, and get rid of the stupid confirmation box? That would also solve the below, no?

I think you +1'd that, then, right, wwalc?

wwalc’s picture

Yes, even +1111 :)

Wim Leers’s picture

Okay, if quicksketch confirms his apparent +1, then I'll work on this tomorrow afternoon!

quicksketch’s picture

Great, sounds good. Yeah after I thought about putting the message near the tag list last night some more I like it even more than when I first suggested it. It seems like winner in every way (just as long as we don't make it too ugly) :)

Your "otherwise" claim is wrong. Enabling an editor does *not* cause tags to be added. Only when a user actively *drags* a button into the active toolbar, the list of allowed tags will be updated. So your (rightful) concern here is fortunately not necessary.

There's still a concern here when enabling a editor on a format. If we don't update the tag list upon enabling an editor, then we're providing the user with buttons that might not work properly. For example if an admin enabled CKEditor on the Restricted HTML format and then saved the configuration without adjusting anything at all they would run into problems. The default toolbar includes an Image button, but no IMG tag is allowed.

The only way we can possibly have this in ckeditor.module is by adding a hook_form_alter() implementation to ckeditor.module. That actually sounds a lot better than what I have right now. Agreed?

Although unfortunate, if we're going to have this hack in place then doing it via CKEditor module is much better, even if it requires a dedicated form_alter(). In some ways that's better because it stands out even more clearly as a work-around.

A simpler solution would be to do this: whenever a configurable plugin of CKEditor is modified (change event listeners on the form elements for configuring CKEditor plugins), kill the hidden CKEditor instance, re-instantiate it with the new configuration, retrieve the metadata for this instance, broadcast any changes

That seems fine to me. Though each individual plugin is still going to have to have customize event handlers if they're going to be affecting the tag list in some way. The styles dropdown just so happens to use a textarea (but one in a specialized format) while other modules may need to enable tags if a button-less plugin is enabled via a checkbox. With either approach of reinitializing CKEditor or broadcasting the required tags directly, we'll need plugin-specific event handlers to deal with each plugin's situation.

Wim Leers’s picture

There's still a concern here when enabling a editor on a format. If we don't update the tag list upon enabling an editor, then we're providing the user with buttons that might not work properly. For example if an admin enabled CKEditor on the Restricted HTML format and then saved the configuration without adjusting anything at all they would run into problems. The default toolbar includes an Image button, but no IMG tag is allowed.

I realize that, but that's a *different*, and less big problem than the one you were pointing out before, which was my point :) Obviously that's important too.

Though each individual plugin is still going to have to have customize event handlers

How's that? It's possible to do something like jQuery('#ckeditor-plugin-settings').find(':input').on('change', function() { REINSTANTIATE(); }) AFAICT?

quicksketch’s picture

It's possible to do something like jQuery('#ckeditor-plugin-settings').find(':input').on('change', function() { REINSTANTIATE(); }) AFAICT?

Because CKEditor doesn't take a list of properties that are formatted as h1.title|title. We'll need to convert that to the JSON format used for the stylesSet setting. Unless we do an AJAX request to the server to reuse the existing conversion we have there, we'll need to convert this list somehow. Since using AJAX introduces latency and other problems (running into validation errors for example), we'll need to rebuild this conversion on the client-side.

Wim Leers’s picture

#26: oh, right. I was going to go the server-side/AJAX request route. The latency here doesn't matter: this is a "rare administration" type task. The validation aspect is a good point though. Nevertheless, I'd still much rather support server-side validation in a generic manner rather than having each CKEditor plugin define a piece of custom JS just to integrate with this. Because then you have to reimplement validation in JS as well. (Or maybe you want to make the JS implementation more forgiving — ignoring any invalid rules?)

quicksketch’s picture

(Or maybe you want to make the JS implementation more forgiving — ignoring any invalid rules?)

Yes that would be absolutely fine, since a validation error should be thrown when the user tries to save the form. So the JS implementation only needs to deal with valid parsings since the validation will prevent an invalid value from being saved. While true that every plugin will need to add its own small amount of JS if it provides tags in some other way, the code to notify the filter system of required tags is fairly simple from what I've seen. After we provide an example with the style list, other modules should be able to emulate.

Wim Leers’s picture

Okay, I'll work on this this afternoon :) Thanks for all the valuable feedback!

Wim Leers’s picture

  • While working on this, I also found a bug: the CKEditorToolbarChanged event's action parameter was wrong when using the keyboard. This caused the functionality introduced by this patch to also not work.
  • Another bug fixed: some buttons don't have matching feature metadata (e.g. the "Remove formatting" button). Yet it was still triggering the event that caused it to be analyzed for required tags.
  • Moved the inline JS hack to make dynamically loaded CKEditor work with jQuery <=1.9 to ckeditor.module, but unfortunately this also required a hook_module_implements_alter() implementation.

Still TODO: get rid of the confirmation window and make it work with StylesCombo, as discussed in preceding comments.

(Posting this in two steps should make the interdiffs a lot simpler.)

Wim Leers’s picture

  • Fixed two edge cases.
  • Message: implemented. Much cleaner JS, too. Uses div.messages.warning, like the table drag warning and locale.module's translation changed warning.

allowed tags message.png

Still TODO: make it work with StylesCombo, as discussed in preceding comments.

(Another interdiff because that simplifies reviews even further.)

Wim Leers’s picture

Status: Needs work » Needs review

Screencast

1-minute screencast: https://vimeo.com/wimleers/unidirectional-editor-conf-to-filter-setting-... (video is currently still being transcoded)

Changes

  1. Addressed quicksketch' big concern: When CKEditor is selected as the text editor, then this is interpreted as "all (the default) buttons have been added", which will cause drupalEditorFeatureAdded events to be fired. So if you enable CKEditor for Restricted HTML, the Image button will be enabled by default and thus the <img> tag will also be added to the list of allowed HTML tags.
  2. Paired to this: when CKEditor is no longer selected as the text editor, then this is interpreted as "all buttons have been removed", which will cause drupalEditorFeatureRemoved events to be fired. So if you enable CKEditor for Restricted HTML, then immediately disable it again, then no changes will be made to the filters (ensures that any filter_html modifications & messages are removed when the text editor is disabled). This required a detach() implementation, that works on trigger==='unload'.
  3. Made it work with configurable text editor features. (Currently only StylesCombo.) Steps taken:
    1. Added new event: drupalEditorFeatureModified. Must be called whenever a text editor feature that already existed before is modified (for whatever reason, e.g. the "Bold" button is configured to use the <b> tag instead of the <strong> tag, or when the "Styles" functionality now is configured to have a different set of styles). This is all that has changed in the API.
    2. Drupal.behaviors.filterFilteredHtmlUpdating now listens to this event, and if a feature is modified that it is remembering as one of the new features, then it will be stored, and the "Allowed HTML tags" setting and the accompanying message will be updated.
    3. Now for the actually meaty part:
      1. ckeditor.stylescombo.admin.js: whenever the list of classes for StylesCombo is changed, Drupal core's formUpdated event fires. We bind an event handler to that, debounce it so we don't redo a bunch of calculations many times per second, and when the calculated stylesSet setting is different from the previous hidden CKEditor configuration, we fire a CKEditorPluginSettingsChanged event
      2. ckeditor.admin.js: listens for CKEditorPluginSettingsChanged events, updates the hidden CKEditor configuration with the settings changes it receives, destroys the existing hidden CKEditor, initializes a new one, retrieves its feature metadata, compares this to the existing feature metadata, and fires a drupalEditorFeatureModified event with the modified feature as a parameter, finally overrides the old feature metadata with the new one.

On point 1: this is the simplest approach possible. I think it would be slightly nicer to have the text editor adjust the buttons that are enabled by default based on the allowed HTML tags, but this is going much further. It's also against the issue title: it'd be bidirectional syncing, not unidirectional. I don't think it's worth the extra complexity. If we do it, I'd much rather do that in a follow-up issue, since the current patch already covers unidirectional syncing.


The accurate, always up-to-date message is very close to the actual setting that's going to be changed: great! But:

  1. that setting is very far away (vertically/scroll distance) from the CKEditor plugin settings. To fix that, we'll have to do #1834682: Consolidate filter options in the UI when configuring a format.
  2. the vertical tab that contains the setting may be hidden…

I don't think the latter can be solved without major filter admin UI changes: we want the message to be close to the setting being changed, but the filter UI requires condensing, hence the need for vertical tabs.

Wim Leers’s picture

And of course, after so much meticulous preparation, I forget the most important thing: the patch.

Bojhan’s picture

I have looked at this patch, the behaviour is how I expected it to work but still a bit funky. Given that we don't have good patterns to handle kind of inline messaging, this solution looks like the best we can do.

I propose to change the message slightly, as it reads somewhat strangely - it references to "also" ? Why doesn't it just say "Added tags: ". Personally I dont really believe we need to message this information to the user, but I can understand that one might want to.

Wim Leers’s picture

There was one @todo in the patch so far, that was going to be fixed in CKEditor 4.1 (based on Drupal feedback). Well, CKEditor 4.1 was released today, so we can get rid of the @todo. The @todo covered needlessly complex "feature metadata" from CKEditor, which required needlessly complex parsing.

Now: less code, simpler code.

This patch now depends on #1950098: Update CKEditor library to 4.1.

Wim Leers’s picture

#34: I'd love it if that message were so succinct! However, I wanted it to be clear that tags would be added because of text editor configuration changes, and if it just reads "Added tags: <foo>.", then the user might start to wonder why those tags would be added.

How about "Added tags (due to text editor configuration): <foo>."?

Bojhan’s picture

I am not sure, I could sway either way. The reason I think that explaining why would be rather hard is because saying (due to text editor configuration) is rather ambiguous. Since people are already in the text editor configuration screen, they might be confused - e.g. is it referring to configuration outside of this screen I am looking at? After all people do not see it as "text editor configuration", but as "configuring the WYSIWYG" - given that we cant name it that, adding this text would potentially be more confusing than clarifying. You really want to say "because you moved some stuff around in the WYSIWYG configuration above, we added tags X, Y, Z at the end of these tags". But frankly, I assume people that actually get to this screen will have some clue as to whats going on - therefor going with a limited label and, or not at all would be enough from my point of view.

Wim Leers’s picture

going with a limited label and, or not at all would be enough from my point of view

Either works for me. For now, I'll go with a limited message, i.e. "Allowed tags: <foo> <bar>.", unless others agree it's not necessary at all. I personally think it's necessary.

Other reviews? :)

wwalc’s picture

I must say I really like how it works already!

Below a few minor things that I noticed while I was playing with the admin interface:

1) Regarding the message that appears above "Allowed HTML tags", I'd use something like this:
"The following tags were added to match the editor configuration:"
but I'm fine with anything that *explains what has happened*.
For me personally, "Added tags: " is too short for the very same reason as Wim explained.

2) When specifying styles for the Styles combo I got a very cryptic error message:

The provided list of styles is syntactically incorrect.

Maybe it would be good to display the line that is invalid?

In my case the problem was in... code|Computer Code

Note that CKEditor works fine if just a tag is used to style text.
For example this is a small part of the default (sample) styles.js file:

	{ name: 'Computer Code',	element: 'code' },
	{ name: 'Keyboard Phrase',	element: 'kbd' },
	{ name: 'Sample Text',		element: 'samp' },
	{ name: 'Variable',			element: 'var' },

3) Enabling CKEditor on Restricted HTML should not enable <img> tag. If the <img> tag was not specified in "Allowed HTML tags" why is it forced there when the editor is changed? Ideally, the act of switching the editors should not result in any tags being added automatically. User should simply get the editor that is prepared for the format he's configuring.

If I'm moving buttons to be able to insert new stuff, I'm ok with adding new tags for me.
However, when I just switch the editor to spend less time on editing raw HTML, I just want it, nothing else.

The following issues were spotted in Firefox (did not check other browsers). They can be definitely moved to separate issues once the patch will be committed and the issues will be still valid.

4) Maybe some day in the future: dragging buttons at the end of the toolbar is a bit cumbersome. Whenever I try to do this, it goes back to available buttons. I want to add it at the end so I slowly move it to the right outside of the active toolbar, and then I drag the button down to the right place (it does not work).

It's a bit counterintuitive that to add the button at the end, one have to move it exactly over the last button.

5) The form in admin/config/content/formats/basic_html is quite big so I have a scrollbar on the right side on this page. It looks like this is causing some problems with d&d operations.

When I scroll the page down and back, then start dragging buttons, strange issues start happening like: dragged button is not visible at all, dragged button appears 200px above the mouse pointer etc.

[OT]

The little "Allowed HTML tags" field is depressing for anyone that actually cares what's there ;)

Wim Leers’s picture

#37/#38: implemented, it now reads: Allowed tags: <foo>. Screenshot:

updated message.png


#39:

  1. Noted :)
  2. Off-topic. See #1954520-1: Drag-and-drop CKEditor toolbar configuration: rough edges for a reply.
  3. I personally agree with that, but that's a whole lot of work. I'll repeat what I said in #32:

    I think it would be slightly nicer to have the text editor adjust the buttons that are enabled by default based on the allowed HTML tags, but this is going much further. It's also against the issue title: it'd be bidirectional syncing, not unidirectional. I don't think it's worth the extra complexity. If we do it, I'd much rather do that in a follow-up issue, since the current patch already covers unidirectional syncing.

  4. Off-topic. See #1954520-1: Drag-and-drop CKEditor toolbar configuration: rough edges for a reply.
  5. Off-topic. See #1954520-1: Drag-and-drop CKEditor toolbar configuration: rough edges for a reply.
  6. Your OT remark: See #1954520-1: Drag-and-drop CKEditor toolbar configuration: rough edges for a reply.

For all off-topic problems raised: I created a new issue: #1954520: Drag-and-drop CKEditor toolbar configuration: rough edges.


What else should be done to get this to RTBC? :) I think it's close?

quicksketch’s picture

FileSize
48.49 KB

Enabling CKEditor on Restricted HTML should not enable <img> tag. If the <img> tag was not specified in "Allowed HTML tags" why is it forced there when the editor is changed? Ideally, the act of switching the editors should not result in any tags being added automatically. User should simply get the editor that is prepared for the format he's configuring.

I personally agree with that, but that's a whole lot of work. I'll repeat what I said in #32:

I still think it's very important. Automatically changing the allowed tags by simply enabling the editor would qualify as a bug IMO. It doesn't matter if the issue title says "unidirectional" or not. We shouldn't knowingly be introducing such errant behavior.

In any case, even in the current iteration, there seems to be a problem with automatically added tags:
- Add a new button. Message shows up that a matching tag has been added.
- Remove the button. The message goes away, but *the tag is still there*. You have to add another button to remove the previous tag and add the next new one.

So even if I enable an editor on Restricted HTML and then *remove* the Image button, it *still* adds the <img> tag.

Regarding the message appearance, the current approach is sort of a combination of both too visually demanding and not descriptive enough. I'd like to see it explain *why* tags are being added, but at the same time not demand so much attention.

Flipping it around to get rid of the parethesis sounds good to me: "Based on the current configuration, the following tags have automatically been added: ". Of course that's really long, but instead of displaying it as a message, it would be less obtrusive if displayed as part of the description. Although this isn't a too common Drupal pattern, generally it's fairly widely used to display information about a field next to it/below it.

Wim Leers’s picture

Status: Needs review » Needs work

I still think it's very important. Automatically changing the allowed tags by simply enabling the editor would qualify as a bug IMO. It doesn't matter if the issue title says "unidirectional" or not. We shouldn't knowingly be introducing such errant behavior.

I'm not sure I would call it a bug per se, but I totally agree it's very poor UX-wise. You might even call it "deceiving". Which would mean it's a bug. Oh well :)

Back to NW to address this.

Remove the button. The message goes away, but *the tag is still there*.

Wow! Great find! Reproduced. Should be a trivial fix :)

I'd like to see it explain *why* tags are being added, but at the same time not demand so much attention. […]

I'm fine with this. I'll check with yoroy/Bojhan too.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
34.92 KB

Remove the button. The message goes away, but *the tag is still there*.

Fixed.


(No "warning message", but an extended description.)

Implemented.


I still think it's very important. Automatically changing the allowed tags by simply enabling the editor would qualify as a bug IMO. It doesn't matter if the issue title says "unidirectional" or not. We shouldn't knowingly be introducing such errant behavior.

This is what I replied to that:

I think it would be slightly nicer to have the text editor adjust the buttons that are enabled by default based on the allowed HTML tags, but this is going much further. It's also against the issue title: it'd be bidirectional syncing, not unidirectional. I don't think it's worth the extra complexity. If we do it, I'd much rather do that in a follow-up issue, since the current patch already covers unidirectional syncing.

My concern was rooted in "live bidirectional setting syncing". I still think that's an insane undertaking.
But, it doesn't have to be that complex:

  • when no text editor is enabled yet, and then one is enabled: unidirectional syncing from filter settings to text editor settings
  • while a text editor is enabled: unidirectional syncing from text editor settings to filter settings

That is workable.

What is still extremely tricky though, is the fact that there's always going to be only one text editor, but there may be multiple HTML-restricting filters… which means that as soon as just 1 filter says "this would require changes to my configuration", then the button should not be enabled by default. It's this 1:many:1 relationship (1 text editor to many filters back to 1 text editor) that makes this a very tricky thing to implement. It's impossible to simply use JS events; we either need an intermediary object/arbitrator/manager or at the very least a registry of all enabled HTML-restricting filters (which can change on the fly), where we need to ensure we have responses from all filters. Very tricky when you're trying to KISS.

I'd like a definite +1 that we really want to do that, before actually implementing it.

webchick’s picture

But, it doesn't have to be that complex:

  • when no text editor is enabled yet, and then one is enabled: unidirectional syncing from filter settings to text editor settings
  • while a text editor is enabled: unidirectional syncing from text editor settings to filter settings

That is workable.

This would work for me, and I think addresses quicksketch's concerns. I agree with him, btw, that magically switching the allowed tags down below just for having the audacity to *select* an editor is a pretty big WTF. Given the stage we are at in the release, I'd prefer to fix it prior to commit so we don't need to open up anymore major/critical tasks to clean things up.

This, however, makes zero sense to me:

Description magically changes

Nowhere else in Drupal does changing a form element in point A change the field description at point B (off the screen, btw). We might as well do nothing over this, because we already know that users ignore field descriptions. They will certainly ignore field descriptions that change without their knowing. IMO we should go back to the yellow alert box because that at least happens in multiple other places in Drupal (e.g. drag and drop) so it's a common pattern.

quicksketch’s picture

Nowhere else in Drupal does changing a form element in point A change the field description at point B (off the screen, btw). We might as well do nothing over this, because we already know that users ignore field descriptions.

You might consider text formats changing descriptions based on the currently selected text format to be a precedent. I think it strikes a better balance than a warning message (I can see the eye tracking dots getting stuck on that warning message all too easily). If we've only got the choices of showing a big alert or showing nothing at all, then nothing at all is preferable to me.

But, it doesn't have to be that complex:

Great @WimLeers, yeah that's more along the lines that I was thinking.

effulgentsia’s picture

It's this 1:many:1 relationship (1 text editor to many filters back to 1 text editor) that makes this a very tricky thing to implement.

Once #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings is resolved, couldn't we do this by setting the fake/hidden CKEditor's allowedContent to what the filters allow, and then ask CKEditor which buttons/plugins are left enabled?

Given the stage we are at in the release, I'd prefer to fix it prior to commit so we don't need to open up anymore major/critical tasks to clean things up.

Does that make this effectively postponed on that issue?

Other than that, it would be great to see some more detailed JS reviews here.

effulgentsia’s picture

Status: Needs review » Needs work

Also, I think the second part of #44and #45 makes this a "needs work", but please correct me if I'm misunderstanding.

Wim Leers’s picture

Yes, NW :) I just wanted to get feedback from quicksketch in particular :)

Wim Leers’s picture

I'll reroll this next week. I've been wanting to reroll it for many weeks now!

Wim Leers’s picture

Straight reroll of #43 to make it apply to HEAD again. Now working on addressing the last few things.

Wim Leers’s picture

This is the basic clean-up patch. The next one will contain that what everybody's been waiting for… :)

Changes

  • I was able to get rid of the messy work-around we had to ensure a dynamically loaded CKEditor works with jQuery <1.9, because we now have jQuery 2.0. :) 30 lines of (crappy) code less!
  • Fix JSHint errors.
  • JS coding standards fixes.

Also

For now I'm sticking with the extended description rather than the warning message, until we reach consensus. webchick is strongly in favor of using a Drupal message, quicksketch is strongly in favor of an extended description because of potentially confused/scared users. Bojhan believes no message is necessary at all, which puts him more in quicksketch's POV.
I'm also in favor of a message, though I understand that less technical users won't understand this. OTOH, less technical users probably won't change the text editor/format configuration anyway.
This is a detail in the grand scheme of things, so I'll just stick with quicksketch's POV, which is in between webchick's and Bojhan's.

Wim Leers’s picture

Assigned: Wim Leers » quicksketch
Status: Needs work » Needs review
FileSize
31.28 KB
59.77 KB

(Assigning to quicksketch for review.)

Changes

From #43:

  • when no text editor is enabled yet, and then one is enabled: unidirectional syncing from filter settings to text editor settings
  • while a text editor is enabled: unidirectional syncing from text editor settings to filter settings

That is workable.

The first bullet is precisely what I implemented here. It's only 5–10% of what the user gets to see, but boy, was it a lot of work… I spent almost 2 full days on this.

What you should know

First read this again from #43:

What is still extremely tricky though, is the fact that there's always going to be only one text editor, but there may be multiple HTML-restricting filters… which means that as soon as just 1 filter says "this would require changes to my configuration", then the button should not be enabled by default. It's this 1:many:1 relationship (1 text editor to many filters back to 1 text editor) that makes this a very tricky thing to implement. It's impossible to simply use JS events; we either need an intermediary object/arbitrator/manager or at the very least a registry of all enabled HTML-restricting filters (which can change on the fly), where we need to ensure we have responses from all filters. Very tricky when you're trying to KISS.

This was completely true. The code is not trivial. If we only had to support the Filtered HTML filter's "allowed tags", then sure, it could've been simple. But… the Filtered HTML also in fact disallows any style attribute and any on* attribute (e.g. onClick). So it'd have been a lie.
Furthermore, we want contrib filters to also be able to leverage this. So if a contrib filter only allows a specific attribute (or maybe even no attributes at all) on a specific tag, then that default button should no longer be enabled by default when the text editor is enabled.

To do this, in a nutshell, I'm using the following strategy:

  1. For a given filter, check for each active filter:
    1. given the feature's HTML requirements, build a "universe" of all possible values
    2. given a filter and the things it forbids: if anything in the universe is forbidden, then stop analyzing and indicate that this feature is disallowed by the text format
    3. given a filter and the things it allows: mark all things in the universe that are allowed, once that's done check if there's anything that's not yet allowed, if that is the case, then this feature is still disallowed by the text format

Consequently, this interdiff adds about 650 lines of code and comments, net probably about 300 lines of code. (The interdiff is almost as large as the patch in #51…)

Ideally, we would have test coverage for this. But we can't do that yet because we don't have JS testing in Drupal core. However, I will write test coverage for this in the https://drupal.org/project/fat module.

So please evaluate this patch based on:

  • code quality/documentation (both of which should be great)
  • how well it works within the use case of Drupal core (perfectly)
  • keep in mind that there's inherent complexity to this. This is as simple as it gets.

Screencast: http://vimeo.com/wimleers/initial-editor-conf-auto-adjusts-to-text-format-filter-settings — shows before vs. after (available in 43 mins)


#46: using allowedContent could work in theory, but one of the problems with that approach is that it depends on the server-side state. What if the user has already added one tag and removed another one from the list of allowed HTML tags? Then this approach would yield incorrect results, since it relies on the stored configuration, not the "currently being configured" configuration.

Bojhan’s picture

Issue tags: +Needs usability review

Adding this tag, reviewing later - before vacation.

Bojhan’s picture

Issue tags: -Needs usability review

Having looked at this, it seems to work quite fine. From a usability perspective I would call this RTBC, its quite nice that we introduce this kind of automagic for our users!

I understand if some people would like more aggressive messaging that something changed in this list. I am still under the opinion this isn't really needed, most of them really don't need to be made very aware of these changes. However its no show stopper for me, if it becomes a commit blocker for webchick - I understand the position of making it more bold. Though I think WimLeers his compromise is fine, it notifies people but not too aggressive.

quicksketch’s picture

Status: Needs review » Needs work
FileSize
59.61 KB
4.51 KB

I took a look at this and overall it looks really good. The code is pretty difficult to follow in places, mostly because there is just a lot of infrastructure being added here. Complex rule and tag filtering logic never looks very pretty. I rerolled with a few code cleanups and adding docs in places where it took me additional research to figure out what was happening.

I fixed a small bug where dragging in a new separator caused a JS error.

There's an additional bug that needs to be resolved that I haven't been able to resolve. When you add the "Styles" button to the toolbar, the tag list is updated as you add new tag.classname|Label pairs (which is great!), but every time the tag list updates, focus is lost on the text area where you're typing tags. This only seems to occur in Chrome (27.0.1453.110, Mac OS X). I've narrowed it down to coming from the getCKEditorFeatures() function. My speculation is that it's caused when a new CKEditor instance is destroyed/created after every update to the tag list.

I still feel like the whole invisible CKEditor instance thing is a little bit over-the-top (even after re-reading #14). I'm not sure this Chrome problem is a browser-specific bug or a problem in CKEditor's code, but I'm pretty sure it's a result of our elaborate approach to getting the filtering rule list from CKEditor through the fake instance.

quicksketch’s picture

Status: Needs work » Needs review

Might as well let testbot rerun it. I think this might be "Needs work" based on the Chrome-losing-focus issue.

jessebeach’s picture

While testing, I was unable to get a status message to appear like the one shown in #40 or the issue summary. I tried moving buttons from the active CKEditor toolbar to the available buttons group. I also tried changing the list of allowed tags. I didn't see either the buttons or the list of allowed tags changing in real time as I changed the other. If I saved the changes then the two lists were synced up. Was this real-time updating of the two lists (the buttons in the editor and the list of allowed tags in the filter's config textfield) removed from the patch?

quicksketch’s picture

@jessebeach: The message about additional tags is shown as pictured in #41 and #43, as description text on the list of allowed tags.

I also tried changing the list of allowed tags.

This patch does not change the button configuration based on the tag list, it only goes one-way, basing the tags on the buttons.

Not sure why the tag list isn't updating. Are you sure you're adding a button that adds tags? i.e. the strikethrough, superscript, or subscript buttons. Several buttons don't require additional tags.

quicksketch’s picture

Status: Needs review » Needs work

Hm, another bug: If I manually add or remove a tag from the tag list (i.e. removing <cite>), then I either add or remove a button, the <cite> tag always immediately comes back in its original position in the tag list. It's not mentioned in the help text as a tag that has been added.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
60.16 KB
11.39 KB

Here's a reroll that fixes the issues I've ran across so far, including those mentioned in #55 and #59:

  • Fixed JS theme function call from Drupal.theme.filterFilteredHTMLUpdateMessage() to Drupal.theme('filterFilteredHTMLUpdateMessage')
  • Changed the formUpdated event on the style list to use blur instead. This fixes the Chrome-loosing-focus problem at the small expense of needing to click outside the field before the tag list is updated.
  • Changed the strange use of $.extend() to add prototype methods to simply adding prototype methods directly by variable assignment. There's no need for jQuery to merge in an object there, since it's where we're originally defining the methods.
  • Fixed the problem in #59 where user-defined changes were getting reverted if the buttons were changed after page load.
  • Reversed the returned tag list so that the added tags are returned in the order (FIFO) they're added to the filter, instead of being reversed from the order added (LIFO).
  • The style list regex was too strict. Specifying h1|Title should be valid option for the styles dropdown, but we were requiring a class name with all tags. Even the default.js style list CKEditor ships with uses tag-only definitions for most of the list. So this patch relaxes the regex requirements slightly. The element portion of the pattern is still required.

Status: Needs review » Needs work

The last submitted patch, unidirectional_editor_to_filter_syncing-1894644-60.patch, failed testing.

Wim Leers’s picture

#55:

The code is pretty difficult to follow in places, mostly because there is just a lot of infrastructure being added here. Complex rule and tag filtering logic never looks very pretty.

Exactly. Inherent complexity.

I fixed a small bug where dragging in a new separator caused a JS error.

Thanks :)


#59: You're right, that should be fixed. The current code assumes the user will not manually change the list of allowed tags, and continues to use the initial value as the list of user-defined tags.


#60:

Changed the strange use of $.extend() […]

I agree it's weird, and I've been moving away from it in later JS patches, but it is a pattern used in Drupal core's JS ;)
(This pattern is clearly superior in terms of readability when adding multiple prototype methods because it conveys the logical grouping. Here there's only one method per object, so your changes are fine.)

Fixed the problem in #59 where user-defined changes were getting reverted if the buttons were changed after page load.

Lovely! Thanks! :)

Reversed the returned tag list so that the added tags are returned in the order (FIFO) they're added to the filter, instead of being reversed from the order added (LIFO).

AFAICT it was FIFO before, and it is LIFO now… Tested both in Chrome & Firefox to make sure it's not browser-specific. Hence, I reversed your reversing :)

The style list regex was too strict. Specifying h1|Title should be valid option for the styles dropdown, but we were requiring a class name with all tags.

This is very clearly out of scope… but okay. If you help push this patch to the finish line, that's fine by me :)
I've fixed and expanded the test coverage for you.


With those very minor bugs out of the way, I think this is almost good to go? :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.48 KB
63.59 KB

Forgot to attach the patch :/

Wim Leers’s picture

And… D8 HEAD has changed; #62 won't apply.

This one should. (Straight reroll, hence no interdiff.)

quicksketch’s picture

AFAICT it was FIFO before, and it is LIFO now… Tested both in Chrome & Firefox to make sure it's not browser-specific. Hence, I reversed your reversing :)

Hm, looks like you're right for *buttons* but for the style list, it comes out backwards. The reason I added the reversal is because if you type this list into the styles dropdown:

h1.a|Title 1
h2.b|Title 2
h3.c|Title 3

The tag list without the reverse added was as such:

<h3> <h2> <h1>

But the reverse I added made it come out in order:

<h1> <h2> <h3>

But at the same time it ended up truly reversing the tag list created by other buttons.

jessebeach’s picture

Ok, I see the notice now below the textfield that indicates what elements have been added to the list of allowed elements based on the configuration of the active editor's toolbar buttons.

I propose we change that text from this

Based on the current configuration, these tags have automatically been added:

to this

Based on the text editor configuration, these tags have automatically been added

I have to admit, I completely missed this line of text and its relationship to the buttons in the text editor. And I was even looking for it, or something like it, to change as I changed the configuration of the editor toolbar buttons.

Here are some questions I have about the syncing.

  1. I have a bold button in the editor toolbar, and a <strong> tag listed in the allowed tags. But the <strong> tag is not listed in the tags that were automatically added based on the configuration of the text editor. The same for italic. I had been testing by moving these two buttons in and out of the editor's toolbar configuration and see no change to the list of allowed tags, which is why I thought at first that the status message just wasn't appearing for me.
  2. The <p> tag is listed as a tag that was added to the list of allowed tags based on the configuration of the text editor, yet I don't see a button for the <p> tag in the list of buttons in the editor's toolbar configuration.
  3. I have a <blockquote> button, but <blockquote> is not listed as a tag that was automatically added to the list of allowed tags based on the configuration of the text editor buttons.

A screenshot illustrating the mismatch between the buttons in the editor toolbar and the list of allowed tags that were added automatically based on the configuration of the editor toolbar.

quicksketch’s picture

The way the message works currently is it only shows you the tags that were added *in addition* to the current list. The message doesn't include <strong> because that tag was already there in the existing list. It's only when you add a button that requires a new tag that the message is shown.

Perhaps it would make sense if the entire list of automatic tags were shown at all times, but then you'd just end up with the entire list displayed twice in many cases. I'm not sure that kind of ever-present redundancy is preferable.

jessebeach’s picture

quicksketch, personally, I'd rather see the whole list. I agree, it's verbose, but it doesn't leave me with a confused mental model. As a first-pass solution to this issue, I'm ok with more text that is accurate rather than less text that's clever. I bet we can improve this incrementally over time.

Wim Leers’s picture

#65: I think that's actually really the tiniest of tiny details. If the order of allowed HTML tags mattered, then it'd matter. But really, here, it's a *set* of things, and order doesn't matter. Correctness matters. User guidance matters. This is a tiny nitpick. I hope we can just let that be and move on to real problems? :)

#66:

  • Text change implemented!
    1. What quicksketch said in #67. I strongly disagree with your assessment that "all tags required by buttons should be listed". Note that Bojhan also expected it to work this way. quicksketch did. I did. Also, the text very specifically states "tags have automatically been added". Added. Not "merged" or "unioned" or "ensured to be listed", but "added".
      Note that this text is not targeted at the user who doesn't know how to configure text format filters. It's targeted at people who actually know what they're doing with text formats, and then the current behavior is the most sensible one.
      The premise is that users who don't understand text format filters won't have to look at the settings, they can just drag in the buttons they want and be done with it. Hence they won't see this text. And it is from that perspective that e.g. Bojhan signed off on this. Again, Bojhan specifically stated he believes that text is not even necessary, which is again from that perspective.
    2. This is only possible by doing two things: 1. ensure the <p> tag is not in the list of allowed HTML tags, 2. add the "Format" dropdown, which includes the "Paragraph" format (optionally also "Heading 1", and so on).
    3. That must be because the <blockquote> tag is already listed in the list of allowed HTML tags then. This is the same confusion as 1. :)

Overall, really, probably about half of the comments on this issue have been about the tiniest detail: the message. There is a solid rationale for the current approach. If it turns out to be problematic later, it's easy enough to change it. Let's not let this patch be held up by a detail :)

Other changes

  • In trying to reproduce #66.2, I emptied the list of allowed HTML tags, and found that the JS handling the filter_html settings wasn't capable of dealing with zero allowed tags yet. I fixed that.
  • I noticed that the JS file for the filter_html filter is called filter.filtered_html.admin.js instead of filter.filter_html.admin.js. Similarly, the behavior is named wrongly. Fixed that too.

What's preventing an RTBC here?

Wim Leers’s picture

#65: With the help of Wiktor Walc (wwalc) from CKEditor I found that the cause! It was not actually CKEditor that was doing this, it was the way we're parsing the data from CKEditor (which was based on http://dev.ckeditor.com/ticket/9994#comment:4).

Attached patch makes the order precisely like you want it :)

Status: Needs review » Needs work
Issue tags: -Usability, -wysiwyg, -sprint, -Spark, -CKEditor in core

The last submitted patch, unidirectional_editor_to_filter_syncing-1894644-70.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Usability, +wysiwyg, +sprint, +Spark, +CKEditor in core

The last submitted patch, unidirectional_editor_to_filter_syncing-1894644-70.patch, failed testing.

quicksketch’s picture

Note that Bojhan also expected it to work this way. quicksketch did. I did.

I think @jessebeach's viewpoint is valid, even if it's different than what I expected. A persistent message would tell you which tags are required by the editor at all times. Although it would be uncommon, it's definitely possible for a user to delete a tag that is "required" by the editor and save the form. Because this is all JS manipulation with no server-side equivalent (or even the ability to have a server-side equivalent, since we're dependent on CKEditor translating the settings), we don't have any validation. So if a user feels inclined to futz with the allowed tag list, it will result in some buttons simply not working at all because their associated tags won't be allowed.

One suggestion @jenlampton had on this problem was that we could mitigate the "which tags have been added" problem by simply making the friggin' textfield big enough to show all the tags. Then you wouldn't be lost as to which tags are added already. Right now when new tags are added, after you've saved the form it's difficult to actually find the tags that have been added to the end of the list. Anyway, that's something that's a problem separate from this patch. It's been a constant annoyance since we started working on this page.

Overall, I think this is a significant improvement and doesn't add any complexity for the end-user. There is still potential for users to screw up their configuration, but they have to go out of their way to mess it up. I think this is a good step forward and (once the tests are going again), this is good to go.

Bojhan’s picture

@quicksketch LOL, yes increasing the size would be a really good idea. I agree, that this is an overall good improvement, jesse should feel free to make an issue about the message thing. I think we should get this improvement in already.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

#74:

I think @jessebeach's viewpoint is valid, even if it's different than what I expected. A persistent message would tell you which tags are required by the editor at all times. Although it would be uncommon, it's definitely possible for a user to delete a tag that is "required" by the editor and save the form. Because this is all JS manipulation with no server-side equivalent (or even the ability to have a server-side equivalent, since we're dependent on CKEditor translating the settings), we don't have any validation. So if a user feels inclined to futz with the allowed tag list, it will result in some buttons simply not working at all because their associated tags won't be allowed.

That's the explanation/background I was missing — now Jesse's POV totally makes sense :) But, as Bojhan says, we can easily change the messaging in a follow-up, it's a 1 LoC thing. I just want to stop moving around in circles on that tiny aspect of the much bigger problem that this solves :)

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

I added #2020967: Improve the design of the Allowed HTML tags filter form in terms of how it interacts with Editors' configurations to address the design issues with the Limit allowed HTML tags filter form.

Otherwise, I propose we get these changes committed to lock in a solid incremental improvement to editor/filter configuration and allow the folks in this issue to turn their attention to other pressing problems.

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed db0e911 and pushed to 8.x. Thanks!

Wim Leers’s picture

Assigned: quicksketch » Wim Leers
Issue tags: -sprint

Woot!

Big usability win for configuring text formats & text editors! :) And one less major, too!

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

Anonymous’s picture

Issue summary: View changes

Show+explain UI changes.

apaderno’s picture

I deleted a spam comment. I apologize for bumping this issue.

apaderno’s picture