Port Aloha 7.x-2.x to Drupal 8 (i.e. a 8.x-2.x version) [#1782326]

The 8.x-2.x version will be proposed for Drupal core inclusion.

See #1760386: Migrate Aloha Editor integration from the Edit module and make it work on the back-end for the migration from Spark's Edit module into the Aloha module. This issue takes what's in the 7.x-2.x branch and ports that to Drupal 8, taking every single change record for Drupal 8 into account.

Comment	File	Size	Author
#9	interdiff.txt	5.13 KB	Wim Leers
#3	aloha.port-cleanup.3.patch	8.7 KB	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 12 September 2012 at 21:13

Status:

Active

» Fixed

Done!

Comment #2

nod_

French

Lille

CreditAttribution: nod_ commented 12 September 2012 at 21:26

well that was fast :p

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 13 September 2012 at 08:21

Status:

Fixed

» Needs review

File	Size
aloha.port-cleanup.3.patch	8.7 KB

Thanks :)

However, there's still some work to do. I prepared a patch with some simple things. I've to say that the registered libraries are highly confusing -- I don't think I've understood which library is which and what each library is doing.

Comment #4

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 13 September 2012 at 10:05

Thanks for the review, @sun! :)

I agree that there are *many* libraries and it can be confusing which provides which functionality. The main reason this can be so confusing though, is simply because we're including each Aloha Editor plug-in as a separate library. The "aloha" library shows how it's all tied together and the "aloha-for-textareas" library provides a library for using it on textareas (with a text format selector): it uses the "aloha" library and adds whatever glue is necessary. The Edit module will similarly provide a "aloha-for-in-place-editing" library or something like that.
All other libraries are loaded by the "aloha" library indirectly, in the form of direct or indirect dependencies. Want to load another plug-in? Add it as a dependency to the "aloha" library. Personally, I think this makes most sense.

That being said, I'm of course open to all suggestions for making this distinction more clear. As you've alluded to in your review: we do need a solid naming scheme, and that might help in making everything more clear.

It's in this area that a review by @nod_ and @quicksketch would be most welcome, too!

An area that you haven't touched upon, but which is very important, is the method I'm proposing to tell AE to load more plug-ins. I proposed to do it by using hook_library_info_alter(). I included an example in the form of the Caption module.
By going this route, we also dont' need to introduce more hooks (which would most likely be AE-specific).

I'd like explicit reviews of this functionality by you, @quicksketch and @nod_, because we need to find concensus on this ASAP.

Feedback on your patch:

+++ b/aloha.moduleundefined
@@ -5,32 +5,20 @@
-/**
- * @todo: the currently included build of AE is not yet optimal for Drupal; we
- *        must create a build of AE that bundles the JS and CSS in single JS &
- *        CSS files on a per-AE plug-in basis. Then, we'll be able to define
- *        libraries in hook_library_info() for each of the plug-ins, allowing
- *        Drupal to reason about AE's dependencies.
- *        So: finish + leverage @nod_'s 'build-with-plugin-separated' profile in
- *        AE.

This cannot yet be removed because it has not yet been done!
The current use of hook_library() is a step in the right direction, but to do this "right", we need to improve our build of AE.
See #1782348: Improve the custom build of Aloha Editor.

(I know that it *looks* like it's been solved already, but it's not.)

+++ b/aloha.moduleundefined
@@ -5,32 +5,20 @@
-define('ALOHA_VERSION', "custom build: 0.21.2 + patches"); // @todo: establish a versioning scheme for our custom build
-
+// @todo Establish a versioning scheme for our custom build.
+const ALOHA_VERSION = 'custom build: 0.21.2 + patches';

WOOOT! I didn't know we could do this! :D

+++ b/aloha.moduleundefined
@@ -5,32 +5,20 @@
-    'page callback'    => 'aloha_repository_link',
-    'page arguments'   => array(3),
+    'page callback' => 'aloha_repository_link',
+    'page arguments' => array(3),
     'access arguments' => array('access content'),
-    'type'             => MENU_CALLBACK,
-    'file'             => 'includes/pages.inc',
+    'type' => MENU_CALLBACK,
+    'file' => 'includes/pages.inc',
   );

Why change this? Better legibility is important IMO.

+++ b/aloha.moduleundefined
@@ -38,15 +26,16 @@ function aloha_menu() {
+  $module_path = drupal_get_path('module', 'aloha');

Sensible, thanks :)

+++ b/aloha.moduleundefined
@@ -138,6 +127,7 @@ function aloha_library_info() {
+  // @todo Library names should only contain dots/periods.

Why?

"common/ui" is *exactly* how AE refers to this plug-in themselves. I.e., this is a 1:1 mapping of AE plug-ins: "aloha.
". I'd very strongly prefer to keep it this way.

+++ b/aloha.moduleundefined
@@ -266,11 +256,12 @@ function aloha_library_info() {
+        // @todo Rename the bundle to just 'drupal'.

That's also the name I wanted to use initially :)

But I felt that would imply this is *the official Drupal bundle*, which would make no sense if it is part of the Aloha module, and not of Drupal itself (even though the Aloha module may be part of Drupal core).

If you'd rather see just "drupal", I'm fine with that :)

+++ b/aloha.moduleundefined
@@ -266,11 +256,12 @@ function aloha_library_info() {
+  $libraries['drupal.aloha'] = array(

Now this is where I *do* disagree. It should be "aloha.drupal", not "drupal.aloha".

"drupal.aloha" implies this provides the Drupal.Aloha object. It implies it is "Aloha functionality for Drupal". But it really is "Drupal functionality for Aloha". Hence: "aloha.drupal".

+++ b/aloha.moduleundefined
@@ -307,6 +298,7 @@ function aloha_library_info() {
+            // @todo Usage of file_create_url() here and elsewhere doesn't look right.

Why doesn't this look right?

It sure looks weird from a Drupal POV. But AFAICT, this is the right way to do it.

Let me explain: AE loads all things it needs through require.js. You let AE know where each bundle lives. AE already knows where its included "common" and "extra" bundles live. Above, we told it where the "drupal" bundle lives.

Now, for our custom AE UI, we actually use the *default* AE UI, and override parts of it. *This* is that overriding. Essentially, you're telling require.js that some paths should be remapped to paths that we provide (hence: "requireConfig -> paths").

If you say that we want/need a more elegant solution: I agree! :) But this is the way it must be done today.

Thoughts?

+++ b/aloha.moduleundefined
@@ -355,8 +347,8 @@ function aloha_pre_render_text_format($element) {
-  // the #aloha property to FALSE.
-  if (isset($element['#aloha']) && !$element['#aloha']) {
+  // the #wysiwyg property to FALSE.
+  if (isset($element['#wysiwyg']) && !$element['#wysiwyg']) {

I didn't want to clash with the wysiwyg module, hence I opted for #aloha instead of #wysiwyg.

So … I'm not sure why you prefer this?

+++ b/aloha.moduleundefined
@@ -443,6 +432,8 @@ function aloha_pre_render_text_format($element) {
+ * @todo Remove this.
  */
 function aloha_plugin_js_settings($settings) {

Why? This allows for significantly less crufty code above.

I also contemplated adding this, but I think the win in code legibility trumps the loss in clarity?

Setting back to NR, because more reviews are needed. (I'm not sure if this is the right way to do it, so please don't chastise me for this.)

Comment #5

nod_

French

Lille

CreditAttribution: nod_ commented 14 September 2012 at 22:49

naming, feel free to add stuff to #1778828: [policy, no patch] Update JS coding standards to discuss in a more general way. As for the module:

filenames
```
js/drupal.aloha.js 
js/drupal.aloha.textarea.js
```
Library name
```
drupal.aloha
drupal.aloha.textarea

aloha.drupal/drupal
aloha.drupal/drupal-ui
```
I'd say this is what hook_library_info_alter is for, no need for a special API.

+++ b/aloha.moduleundefined
@@ -5,32 +5,20 @@
-    'page callback'    => 'aloha_repository_link',
-    'page arguments'   => array(3),
+    'page callback' => 'aloha_repository_link',
+    'page arguments' => array(3),
     'access arguments' => array('access content'),
-    'type'             => MENU_CALLBACK,
-    'file'             => 'includes/pages.inc',
+    'type' => MENU_CALLBACK,
+    'file' => 'includes/pages.inc',
   );

I agree with this change, I don't remember core files doing that. The better readability can be questioned (not helping me at least :)

```
+++ b/aloha.moduleundefined
@@ -138,6 +127,7 @@ function aloha_library_info() {
+  // @todo Library names should only contain dots/periods.
```
I'm with Wim on this one, the / makes total sense here. That's how AMD-like/CommonJS-like things will refer to it. And using dots will just end up ugly and confusing (to people who work with aloha/AMD projects, exactly the kind of people we want to get more of in drupal).
+ // @todo Usage of file_create_url() here and elsewhere doesn't look right. once this one is fixed #1782348: Improve the custom build of Aloha Editor there won't be a need for file_create_url. until then we need it. I'd like to point out that the end result of the other issue is to not have to use requireJS to load things. All files will end up being loaded/processed by Drupal if the build is clean making aggregation and all that possible with aloha modules.
about #wysiwyg i'd think he's hoping the wysiwyg module won't be necessary for D8 maybe? :)

Now my own itches:

drupal-aloha.js:Drupal.aloha.init: it'd be nice if we had a consistent way of doing those callbacks list. Drupal.attachBehaviors is using it and that'll probably come up in other places. The jQuery.Callback thing is neat, but don't tolerate failures, can't use it for Drupal.attachBehaviors. That's what their event system is using too, which isn't behaving like the DOM so that'll be confusing anyway.

drupal-aloha.js:

var id = $editable.attr('id');
    // If no ID is set on this editable, then generate one.
    if (typeof id === 'undefined' || id === '') {
      id = 'aloha-' + new Date().getTime();
      $editable.attr('id', id);
    }

could be

// If no ID is set on this editable, then generate one.
    var id = $editable.attr('id', function (i, val) {
      if (typeof id === 'undefined' || id === '') {
        return 'aloha-' + new Date().getTime();
      }
      return val;
    });

drupal-plugin.js: var pluginNamespace = 'aloha-drupal';
unused variable
there is some magic going on with jQuery .data() and data attributes. I'd rather use .attr() when dealing with data attributes values, safer.

And that's it from me :) That's really nice work I'm getting excited about having that in core :)

(I was reviewing 8.x branch)

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 15 September 2012 at 08:16

Thanks for the review, @nod_!
I have only one question for you: yes, I wrote Drupal.aloha.init for lack of an existing API (AFAIK at least). I even don't like it myself :P Which lath do you want me to take there?

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 15 September 2012 at 09:19

Version:

7.x-2.x-dev

» 8.x-2.x-dev

To clarify on #wysiwyg:
This form element property has been introduced throughout the contrib space to allow other modules and alter hook implementations to force-prevent a client-side editor on a particular form element. It is supported by most client-side editor integration modules, and only supports an optional value of FALSE.

(The backstory is actually more funny; Wysiwyg module introduced the property some years ago, but removed it as it appeared unnecessary. However, other standalone editor integration modules picked the idea up before it was removed. Eventually it was re-introduced through a bug report that complained about not being able to force-prevent an editor with Wysiwyg module but with all others. *ggg* Sometimes I'm just making too much sense and don't even know about it ;))

Comment #8

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 15 September 2012 at 10:38

Heh, ok :) I'll definitely change that, then!

(And yes, only now the 8.x-2.x version shows up as a choice — thanks for updating it :) The 8.x-2.x dev snapshot now also appears on the project page, FYI.)

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 17 September 2012 at 10:14

File	Size
interdiff.txt	5.13 KB

@sun: I've committed your patch, albeit with some changes: 1) I've renamed the bundle as you had suggested, 2) I've kept the slashes in library names as per my and @nod_'s comments, 3) I did not rename the aloha.aloha-drupal/drupal bundle to drupal.aloha, as per the same comments; I renamed it to aloha.drupal/drupal instead (as per change #1).
See the attached interdiff.

@nod_: your own itches:

Please advise, I"m not sure what to do there (as I already said in #6).
That does not work, if you set an attribute value, jQuery will return the set of elements it's being applied on (chainability!).
This was added by Rene Kapusta of Gentics (this plug-in was implemented by him, modified by me). AFAICT removing this line does not negatively impact the functionality. Nor can I find anything about it in the docs. So, gone it is. (Note that these plug-ins still need to be cleaned up anyway!)
I've removed the usage of .data() from drupalcontenthandler.js. Do you also want to see it removed elsewhere? The one in tab.js I added myself, the other ones are all AE code.

Commit: http://drupalcode.org/project/aloha.git/commit/eeb8ce2

Comment #10

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 17 September 2012 at 10:22

And another commit for renaming the files as per @nod_'s suggestions: http://drupalcode.org/project/aloha.git/commit/1e4debd

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 17 September 2012 at 10:31

@sun rightfully noted in chat just now that big @todo's shouldn't exist in code, they should exist as issues. We already had an issue for it, so I've removed the long @todo text from the code and linked to the relevant issue instead: #1782348: Improve the custom build of Aloha Editor.
Commit: http://drupalcode.org/project/aloha.git/commit/bb9e569

Comment #12

nod_

French

Lille

CreditAttribution: nod_ commented 17 September 2012 at 17:02

Chatted a bit with Wim here was my review:

for the callbacks things, don't worry just a general comment from me, not really something to be fixed at this level.
you'd need to have all $('selector', scopeElement); turned into $(scopeElement).find('selector');
I'm pretty sure you can remove several classes at the same time with .removeClass()
```
})(jQuery, Drupal, Aloha, this, this.document);
```
can be simplified to
})(jQuery, Drupal, Aloha); otherwise you mix "modules" with window/document, will get messy with the dependencies things.

And after that I'm cool with the JS code for core.

Comment #13

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 25 September 2012 at 16:12

Improvements made over at #1786712: Clean up mark-up/CSS in Edit/Aloha have been ported to D8, see #1786712-3: Clean up mark-up/CSS in Edit/Aloha for details.

@nod_:

ACK
I can't find any occurrences of this. I think it's already okay? I think you might also have looked at Edit's JS, because I'm sure it occurs there?
Done.
Done.

Commit: http://drupalcode.org/project/aloha.git/commit/6aab1b0