Want to help out? See the Aloha Module Issue Queue, remaining tasks, and how to test.

As part of the Spark project, we've been working on in-place editing, including "true WYSIWYG" editing on the front-end. The reception of Spark by the community at DrupalCon Munich was very positive. Thanks to that, we want to proceed with getting in-place editing in Drupal core.

Roadmap

The first step towards inline editing is getting WYSIWYG in core.

At this time, it looks like we'll have the following rounds (assuming they go well, of course):

  1. Round one: filter types and allowed tags — #1782838: WYSIWYG in core: round one — filter types
  2. Round two: Aloha.module (this issue)
  3. Round three: ability to insert images, and caption images (not Media module)
  4. Further rounds: polish!, the ability to configure the order of the buttons of the WYSIWYG editor, potentially adopt Create.js, macro filter tags, migration path, and more.

Only the first two rounds must be done before feature freeze (Dec 1), the others can potentially be done before code freeze.

After round two, it is probably best to start working on preparing the Edit module for core inclusion.

(For a complete picture, please see this meta issue: #1706688: [meta] In-place editing, inline macros, editables, and Wysiwyg in core.)

Problem/Motivation

What this is about

The core problem: Drupal core is probably the last mainstream CMS on the planet that doesn't have a WYSIWYG editor included out of the box. While it’s certainly possible to configure a fully-featured WYSIWYG editor with contributed modules, it requires both fairly sophisticated knowledge of Drupal core’s filter system security, and is also a lot more work than Drupal’s competitors to achieve the same thing. This is hurting Drupal badly from an adoption standpoint; easier-to-use, less capable solutions are being chosen in many cases where Drupal would be a perfect fit. That makes this a critical feature to resolve in 8.x.

Hand-typed HTML is not only extremely error-prone, but requires advanced knowledge to do by hand. Text has a variety of misspelled tags and attributes.

The problem is, most WYSIWYG editors suck. They generate crappy mark-up. Often, you have to create a custom CSS file to make it look "as much as possible but not quite" as the eventual result while editing. We need a solution that will work with the actual site’s styling, and doesn’t rely on kludgy workarounds like iframes and an approaching reimplementation of the site’s “actual" CSS.

An image of a WYSIWYG editor with bright pink background and red comic sans font.

Furthermore, Drupal has a “preview problem." In order to make a text change on the front end, a content author needs to click a link, be taken to a completely separate form that looks nothing like the content they’re trying to edit (especially if they’re using an administrative theme), and previewing this content displays it on the backend only; not in the context of the rest of their site with sidebars and so forth. What we want to eventually achieve is that previewing content through some sort of action is no longer necessary, because you're editing the content while it has its actual styling, in the exact context where it will be used.

The way to fix this is with “in-place" editing, where a content author just clicks on, for example, the node body and starts typing, with some indications to make it clear that you’re editing the body, plus a formatting toolbar above the node body (which might follow along as you type). So we also need a WYSIWYG solution that will allow for editing the content while it has its actual styling, in the exact context where it will be used.

Proposed resolution

Our goal is to ship Drupal 8 with a WYSIWYG editor that is better integrated than any other CMS's WYSIWYG editor. Through extensive research, and community consensus building we’ve arrived at Aloha Editor to fill this need. You can view a demo of Aloha Editor’s capabilities by its creator, Haymo Meran, at about 15:20 in the DrupalCon Munich Spark Core Conversation video.

Screenshot of Spark distribution showing Aloha Editor on the site back-end form.

Aloha Editor has the following things going for it:

  • was selected together with the community (#1580210: Figure out what WYSIWYG editor to use) to ensure the best choice possible was made
  • can be used on both the front-end (to achieve "editing the content while it has its eventual styling, in the exact context where it will be used") as well as on the back-end
  • has deep Drupal integration for better UX: ability to insert a link to another piece of content in your Drupal site, or any image uploaded to your site (no matter which field or module)
  • has a custom UI, so that it doesn't feel alien
  • has a custom UI that is ready for high pixel density displays ("Retina") through the use of icon fonts
  • has a strong plug-in architecture ("ability to change/override anything"), much like Drupal has
  • aims to be the most accessible WYSIWYG editor
  • is future-proof because it is based on standards, yet as browsers evolve, various compatibility shims can be removed so we can send less bytes over the network.
  • has stellar support for copy/pasting from Microsoft Office, Google Docs, etc.

For even more information about Aloha Editor and why it was chosen, see http://wimleers.com/blog/spark-update-wysiwyg-choice and http://buytaert.net/spark-update-wysiwyg-editing-for-drupal.

What Aloha Editor definitely is not: 100% ready yet, neither in terms of the WYSIWYG editor itself, nor in terms of its integration with Drupal).

Remaining tasks

Known bugs on our end (to be resolved before commit):
- critical: #1786550: Plugin configuration
- major: #1804700: Link/image insertion: autocompletion doesn't show up
- normal: #1783630: CaptionedImage is broken on the back-end

Known bugs/limitations in AE (to be incorporated over time, as they’re fixed):
- critical: #1747930: contenteditable is not accessible to screen-readers — being fixed upstream
- critical: #1789240: Ensure Aloha Editor UI can be *always* present, not only when actively editing ("static toolbar", prevents flashing) — upstream changes for accessibility will likely make this trivial to solve
- major: #1808466: Make Aloha Editor no longer need a SCRIPT tag attribute to defer its initalization — being fixed upstream
- normal: #1803578: AE 0.22.1 is incompatible with jQuery 1.8 — get rid of work-around — being fixed upstream
- minor: #1785732: Aloha causes JS console error in Firefox, but works fine — being fixed upstream

Still to be done before feature freeze:
- critical: #1782348: Improve the custom build of Aloha Editor — being fixed/improved upstream
- major: #1782372: Kick-ass image support: integration for AE's Image + CaptionedImage plug-ins — specifically see #1782372-2: Kick-ass image support: integration for AE's Image + CaptionedImage plug-ins; relates to round 3 in the roadmap

Post-feature freeze:
- #1784710: Polish!
- #1727922: Let aloha_repository_link use the default search module
- #1804440: AE i18n
- #1806492: Make the 'Drupal' plug-in for AE obsolete or at least significantly easier to maintain
- Nice-to-have: make Drupal core use a single, swappable, site-wide icon font (that any and all modules can use so that the icons used everywhere are consistent). This would allow site owners to give their sites a custom feeling by simply building their own icon font. The Aloha core patch in this issue will for now include the Spark iconfont that lives over at http://drupal.org/project/admin_icons, because we don't want to deal with the "swappable, site-wide icon font" feature right now.

Reviews that happened already:
- @sun reviewed the PHP code: #1782326-3: Port Aloha 7.x-2.x to Drupal 8 (i.e. a 8.x-2.x version)
- @nod_ reviewed & approved the JS code for core inclusion already: #1782326-12: Port Aloha 7.x-2.x to Drupal 8 (i.e. a 8.x-2.x version) — the code has changed somewhat since though, but it should be in pretty good shape overall
- @psynaptic reviewed & cleaned up the CSS code: #1800010: Clean up CSS files — more reviews needed, though that could potentially happen post-feature freeze

Besides the already long list above, it is easy to spot more rough edges, minor bugs, major bugs, and so on. The ones above should contain the most important issues. Please be aware that the Spark team is committed to continuing to work on improving the WYSIWYG editor (both in terms of Drupal changes and upstream changes) all the way to code freeze, and beyond.
With the above ambitious goals, it is impossible to get this in "near perfect" shape (which is what is necessary for something to ship with core) either in this patch, or before feature freeze.

This issue aims to get "something that works reasonably well, for some value of reasonable" in Drupal core, as a basis to continue iterating from in the months to come.

User interface changes

Once you enable the Aloha module, it will provide the ability to edit text areas (that have a text format selector) through Aloha Editor, *if* the currently selected text format is compatible. In practice, this is on node, comment, taxonomy term, block body and user signature forms.

Aloha Editor on a comment form.

(To figure out a full list of consumers, this command may be handy to you: egrep -rn "'text_format'" --exclude=filter.module --exclude=form_test.module --exclude=FormTest.php . | grep -v "*")

API changes

- Currently no API changes.
- Addition of the aloha.module to Drupal core.
- Preferably we'd update the standard install profile to configure the "Filtered HTML" and "Full HTML", but we can do that later on. Patch already attached to show what that would look like. (This patch was originally part of the #1782838: WYSIWYG in core: round one — filter types patch, but has been removed from that patch in #1782838-25: WYSIWYG in core: round one — filter types.)

Testing the patch

The easiest way to test this patch is to use the Spark demo site at http://spark.webchick.net/8.x/ which is running the 8.x-1.0-alpha2 version of Spark, which contains all of the below steps already set up.

Note that this patch doesn't get us inline editing; only WYSIWYG editing on the backend, so you have to go to a page like node/2/edit to see it. Edit module is still undergoing clean-up for core proposal.

If you want to test this patch manually against a vanilla copy of 8.x, you’ll need to do the following:

  1. #1782838-26: WYSIWYG in core: round one — filter types (patch link) — this issue is still in flux, so please use the patch in this specific comment
  2. Apply the attached "Aloha module" first, then the "Aloha Editor build" patches. I specifically separated out the "Aloha Editor build" patch so that we can evolve the actual module code separately from the Aloha Editor build patch. (Note that you could alternatively download the 8.x version of the Aloha module and install that. The patches are generated automatically from that.)
  3. OPTIONAL: The attached "install profile" patch if you want to do a fresh Drupal 8 installation and don't want to mess with filter system settings.

Alternatively, we've provided a handy shell script that does all of the above for you (including the optional "install profile" patch).

If you're using core's filter_autop or filter_url filters, it's not compatible (because they generate HTML). If you're using the filter_html filter, then please add the <br> and <p> tags to the list of allowed tags. If you applied the optional "install profile" patch, then you can do a fresh install and all should be pre-configured for you.

Whew! :) That’s everything. We really look forward to your reviews, and help in driving this closer to home.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

webchick’s picture

Issue summary: View changes

One day, I will find who invented "smart quotes" and smack them in the face with a giant salmon.

webchick’s picture

Issue summary: View changes

More of that.

webchick’s picture

Switching back to Filtered HTML.

webchick’s picture

Issue summary: View changes

Linking to shell script.

nikkubhai’s picture

Wim, I don't know if this is relevant to topic or not. But, I was just thinking about a couple of things:
1. How will the editor look on mobiles?
2. Are those tabs 'Format' , 'Insert' necessary? If they can be removed and all the options be placed in one bar, it will be easier to use the editor and also, it will save a lot of space.

Bojhan’s picture

Issue tags: +Usability

Obviously from a user experience perspective we have longed for a WYSIWYG editor in core, its a major content authoring experience that we miss out of the box - and quite a site-builder hassle to setup.

I have reviewed the Aloha stuff throughout the process, the interaction model its follows is to put configuration for elements like the URL in its bar. This is quite a unique pattern, I imagine as we test this with contrib it will have some issues - but overall I do not see major/critical UX issues with the editor itself. I agree with point 2, raised here above - but I do not see why we can't change that down the road - if we decide that truly is better.

So at least from a UX point of view, the WIYSYIG editor can be committed before feature freeze.

nod_’s picture

Issue tags: +JavaScript

This will be the biggest piece of JS in core with Views, I've been involved very early in the process and have been looking at the JS code regularly.

It's core material. There might be a few things to move around and some code style details to fix but in the overall scheme of things it's minor and entirely post-feature freeze material.

From a JS point of view, i'm happy with it.

mgifford’s picture

@Wim, wondering how things have been addressed in AE about accessibility. Did the AE folks get @Everett involved (professionally) to look at/improve their semantics & use of ARIA?

Also, always useful to know if the demo of the latest code is up somewhere.

patrickd’s picture

applied all patches, got some whitespace errors

As aloha goes into core - shouldn't the module package be 'Core'?

After installing the standard profile, I went to node/add/article and got this:
Fatal error: Call to undefined function filter_get_filter_types_by_format() in /home/patrickd/www/drupal/core/modules/aloha/aloha.module on line 470

Do I need to apply the patch of "wysiwyg in core round one" too?

webchick’s picture

Yep, please see http://drupal.org/node/1809702#testing. If you want to test the patch against vanilla 8.x, there's some set up involved.

Or, install Spark 8.x-1.0-alpha2: http://ftp.drupal.org/files/projects/spark-8.x-1.0-alpha2-core.tar.gz which has everything pre-configured.

patrickd’s picture

ah okay, (sorry, should read issue summaries more carefully)
I've set up a public sandbox of spark alpha2 for some hours: http://sefbd96aaf8e959e.s2.simplytest.me/
admin/admin

mgifford’s picture

@patrickd - does that work? I logged in fine, but when going to:
http://sefbd96aaf8e959e.s2.simplytest.me/node/2

I didn't see AE pop up as it had in the past. It's also really good to put in specifically what it is that you've put up. I could get to the edit page easily, but we're not supposed to be testing it here, right:
http://sefbd96aaf8e959e.s2.simplytest.me/node/2/edit

I have no way of knowing if the patch in #1 from yesterday is in either your demo or in @webchick's tarball.

webchick’s picture

mgifford: Note that this patch doesn't get us inline editing; only WYSIWYG editing on the backend, so you have to go to a page like node/2/edit to see it. Edit module is still undergoing clean-up for core proposal. I'll update the testing instructions with this info.

webchick’s picture

Oh, and forgot to say patrickd, thank you SO MUCH for setting up a demo site! :D I'm going to try and get to a more permanent solution probably this weekend.

mgifford’s picture

Thanks @webchick. Also pretty neat to see http://simplytest.me - quite a hopeful space to do some testing/development. We did some testing with https://www.getpantheon.com back at the accessibility sprint in Montreal.

So great to see tools like this coming along to make development & testing of core (and distro's) easier.

mgifford’s picture

Issue summary: View changes

Critical issue before commit #1786550: Plugin configuration was missing.

larowlan’s picture

Congratulations wim et al on such a thorough issue summary

moshe weitzman’s picture

Kudos on this great effort. Some reactions upon using it:

  1. There should be some help for using the editor. For example, it looks like CTRL-B and CTRL-I work for bolding and italicizing. I wonder what else the editor does. I imagine we could have a blue question mark that takes you to a help page.
  2. The insert tab looks pretty bare. If we have no immediate plans for it, we should remove it, as it only duplicates the hyperlink button right now.
  3. The down arrow on the paragraph button looks a bit odd. Not a big deal. Dunno if we can the new dropbutton here.
  4. Is there no way to view the HTML source that is composed for you? How would one edit HTML attributes as suggested by this filter help text: Captions may be specified with a data-caption="Image caption" attribute. Images can be aligned with data-align="left".
moshe weitzman’s picture

Issue summary: View changes

Updating testing instructions.

webchick’s picture

Just a note that I've set up a more semi-permanent place to demo the 8.x components of Spark (including this patch) at http://spark.webchick.net/8.x/. I've updated the testing instructions to point here as well.

nikkubhai’s picture

@webchick:
I checked out the demo. Its really awesome and everything worked great!
Thanks and Congratulations to the Spark team !

Wim Leers’s picture

FileSize
311.48 KB

@all:
- Format + Insert tab: right now there's only a "Insert link" button on the Insert tab. Soon, there will also be "Insert image". See #1782372: Kick-ass image support: integration for AE's Image + CaptionedImage plug-ins for details. There's a conceptual difference between formatting and inserting as well. That being said, it is not an absolute necessity, and it could easily be changed. We could even make it configurable.

#2: 1. Excellent question. Attached is the current design for Aloha Editor on mobile. We haven't gotten around to implementing that just yet. However, we did make the CSS for the toolbar as good as possible already. Try it on a mobile device and it won't look perfect yet, but it also won't be entirely broken.

#5: Right around now, the Aloha Editor guys (Gentics.com) should pretty much have an agreement ready with Everett Zufelt's employer, to hire Everett as an accessibility consultant. They're very serious about moving this forward, and I'm following up closely, but the business details are not mine to know & manage, of course. As soon as there is news, I will share it in the relevant d.o issue (#1747930: contenteditable is not accessible to screen-readers).

#14:
1. Good point. Created an issue for that: #1810844: A help page or plug-in is needed to teach users how to use Aloha Editor.
3. How does it look odd? Using the new dropbutton in there is impossible.
4.a Currently, there is indeed no way to view the HTML source. There already exists an AE plug-in that makes this possible though, and otherwise it wouldn't be too hard to write this ourselves. If you want a "quick 'n dirty" solution for now: switch from "Full HTML" to "Plain text", for which Aloha Editor won't load, meaning you'll get to see the HTML AE generated.
4.b

Captions may be specified with a data-caption="Image caption" attribute. Images can be aligned with data-align="left".

If you see that, that means you've installed the Caption module. The Caption module comes with an AE plug-in that allows you to align images and set captions *right inside of Aloha Editor*, without having to mess with those attributes manually. So that will be a side-effect of using Aloha Editor: the filter help text below a text area with processed text will now explicitly mention allowed tags, attributes that you can use, arcane syntaxes, etc, without the user actually having to use them manually! Maybe we should just *hide* the filter tips as soon as Aloha Editor is active?

falcon03’s picture

Issue tags: +Accessibility

@Wim Leers: I have only a little concern...

I think we shouldn't commit something that isn't accessible!
So, in my opinion, we shouldn't commit this before the contenteditable issue is solved...

webchick’s picture

We're working with Haymo Meran (project lead of Aloha Editor) on resolving accessibility issues upstream and trying to ascertain their timelines for this. We will hopefully have more news on that front next week. But I think this is a case where we may need to defer 100% gate conformance in order to get this critical feature in before feature freeze. As laid out in the issue summary, "it is impossible to get this in "near perfect" shape (which is what is necessary for something to ship with core) either in this patch, or before feature freeze." However, we are fully committed to ensuring it is so before release.

webchick’s picture

Also, #1782838: WYSIWYG in core: round one — filter types was just committed, so there are no more core blockers for this. Help on the referenced sub-issues and/or patch reviews on this patch are very welcome!

falcon03’s picture

Hi Webchick,

ok, I can understand...

However, we should use the "views approach" also for this feature: all accessibility issues should be marked as "critical" and, eventually, the most important of them must be release blockers...

When the "Contenteditable" issue will be solved, I'll start to report accessibility issues for Aloha Editor...

BTW, aren't missing "modal dialogs" another blocker for this issue?

webchick’s picture

Yep, I don't disagree with that approach at all.

And no, we aren't making use of any modal dialogs in our implementation of Aloha Editor. The toolbar just toggles on/off visibility of various fields. You can try it yourself at http://spark.webchick.net/8.x/.

Bojhan’s picture

I am not really convinced we need a Insert tab either, opened an issue at #1817550: Minimize usage of Aloha Editor UI's tabs: don't show them when only one tab is available

mgifford’s picture

Just even trying to get this to work without a mouse is still a pain. Looking at the demo now there are problems jumping back/forth between the editor & the content to format text. It needs more testing with actual keyboard only users and people using Assistive Technology.

Great that the demo is there.

falcon03’s picture

@Webchick (#22):

I tried to use the demo site, but I didn't understand something: what can I edit to test the Aloha editor?
When I tried to edit the "about us" page I received an "access denied" error (I was logged-in as sparkles)...

And, of course, I have to confirm Mgifford comments!

webchick’s picture

Sorry, to test click into the main node and click the "Edit" tab there. http://spark.webchick.net/8.x/node/2/edit is a direct link.

Everett Zufelt’s picture

I oppose this being committed unless there is basic screen-reader and keyboard only support. Only a promise from Dries to fully resolve post feature freeze would change this. IMO issues that are "critical" get demoted nearing release.

draftkraft’s picture

Hi, I'm Haymo, from the Aloha Editor team. We are very glad the drupal community chose Aloha Editor to implement inline editing in drupal and we are following the progress with high interest and constant contact with the spark team, especially Wim Leers. Thank you Wim at this point for your pull requests and help improving Aloha Editor.

Accessibility is very important to us, and we are committed to make inline editing as good as possible with todays screen readers. There are some technical hurdles we're running into as accessible inline editing as a technology is very young. We try to provide the best possible solution. We will have a first accessible Aloha Editor version (standalone) for testing soon and your feedback is very welcome!

Thank you all.

effulgentsia’s picture

Thanks Haymo! The Drupal community is fortunate enough to include several people who are very knowledgeable about and passionate about accessibility. Some of whom are following this issue. For those who are and want to help, I highly encourage you to contact Haymo about how you can help their upstream work. Thanks in advance to anyone who does this.

quicksketch’s picture

Not sure if we're looking for more usability feedback at this point, but things I've noticed:

- The process of editing/creating a link was clunky on my first couple attempts. I expect to be able to highlight text, click the link button, type the link, then return to where I was typing without using the mouse again. This isn't for "accessibility", it's just the expectation I have when using an editor (even BUEditor here on Drupal.org behaves this way). Unfortunately when I'm done editing a link, I repeatedly hit "tab" to get back to the editing interface. My cursor starts back at the beginning of the editor, instead of where it was when I started inserting the link. I found that using the "return" key instead behaved the way I expected, but considering hitting return on forms will frequently submit the entire form when in a text field, I can see this causing a lot of frustration.

- Clicking on an image gives no indication that it is selected in the editor (in both Firefox and Chrome). The toolbar options for editing the image are missing. For some reason the "Image" tab is displayed before the "Format" tab. The "Format" tab should always be first, IMO.

- Upon page load, the editor rudely claims focus. I expect to start at the start of the form, not on the first WYSIWYG editor.

- The disappearing/reappearing toolbar on the editor when focus is lost/gained is jarring, especially as I'm moving down the form and the position of the remainder of the form is shifted as the toolbar disappears. Can we make the toolbar always visible when on the node form?

- There's a small styling issue with the block formatting dropdown (the Paragraph symbol) in that the down arrow is not shown on the same line as the icon. It's pushed down to the next line.

quicksketch’s picture

Additionally the ability to resize the text area is lost while Aloha is active. Oddly I can disable Aloha by selecting Plain Text, increase the textarea size, then enable Aloha again by selecting Filtered HTML and the editor maintains the new size. If Aloha has a plugin to enable resizing natively, it should be enabled.

Wim Leers’s picture

@quicksketch: Thanks for your review!

  • Hm, I'm not sure why that didn't work for you. I just tried it again myself, wondering how I could've missed that myself and not having received that feedback previously. So, I selected a piece of text, clicked the link button, typed a URL, pressed return, then the cursor is restored just behind the link. Works just fine? So is the problem you're seeing the fact that in the step where I use the "return" key, you expect that that is going to lead to unintended side-effects such as submitting the form, or is there more to it?
    To be perfectly clear: we didn't focus at all on keyboard navigation. We focused on raw functionality. Keyboard navigation also relates to accessibility, and that we still have to tackle, as can be seen from the other comments on this issue. I'd say it is related to #1747930: contenteditable is not accessible to screen-readers, which links to issue 590 for Aloha Editor on GitHub, which is precisely about keyboard accessibility.
  • The image handling is still very rough around the edges. The plans are clearly laid out in #1782372: Kick-ass image support: integration for AE's Image + CaptionedImage plug-ins. Currently, only the most minimal MVP has been implemented: #1782372-11: Kick-ass image support: integration for AE's Image + CaptionedImage plug-ins. The idea is to improve this post-feature freeze. It also depends on upstream fixes going in.
  • The automatic focus claiming and disappearing/reappearing are one and the same problem: in the current UI implementation, Aloha assumes there's only ever one active instance on the page. We're working with them to solve that (i.e. to allow any number of simultaneously visible toolbars): #1789240: Ensure Aloha Editor UI can be *always* present, not only when actively editing ("static toolbar", prevents flashing), but it's most likely going to have to be a post-feature freeze thing. The automatic focus happens on the Drupal JS side, to ensure that at least one instance is visible from the beginning. It's a hack that will go away. It's extremely annoying indeed. Overall, this very much depends on upstream changes.
  • That styling issue I cannot reproduce in Chrome, but I can see it in Firefox. Maybe I should've stated that it currently should only look flawlessly in WebKit browsers — I apologize for that. We still have to work on making it look exactly the same across all browsers.
  • The ability to resize Aloha Editor-powered editing on the back-end indeed breaks the resizing functionality. There wasn't an issue yet for that, so I created it: #1821538: Ability to resize Aloha Editor vertically on the back-end.

Every single problem you've raised we absolutely need to solve. No doubt about that. The bigger/harder ones require upstream changes, and we're working with the Aloha Editor folks to address them.

That being said: what is your overall verdict at this moment? Would it be acceptable for you for this to go in so that we can make it perfect over the 4 months after feature freeze, before code freeze? If that's not the case, what are the minimum things that should change for this to get committed within a few weeks?

Dries’s picture

@all: It seems like this issue is not getting enough reviews despite this being one of the biggest improvements/necessities for Drupal 8. I'd like to commit this so would love to see some more reviews. We are committed to this, and it doesn't break anything else, so committing this sooner than later would be better -- just like we did with Views.

@Everett: We have every desire to make this accessible and we're working with the ALOHA team to do so. However, the WYSIWYG editor not being completely accessible shouldn't hold up the commit. The WYSIWYG editor can be disabled. It is also still valuable in environments where there are known not be visually impaired people. We will work hard to make it accessible after the first commit and are tracking this in #1747930: contenteditable is not accessible to screen-readers.

cosmicdreams’s picture

Are the testing instructions listed here up to date?

cosmicdreams’s picture

I tried testing this issue and ran into the same issue as http://drupal.org/node/1809702#comment-6588810

That person was instructured to use the testing instructions to get vanilla 8.x ready to test but those instructions seem to be out-of-date as they instruct the test to install an already committed patch.

Here's how I tested:

  1. I applied all the patches here in order listed in the testing instructions on top of a recent 8.x.
  2. Dropped by database
  3. Removed my settings.php
  4. Started installation
  5. Didn't see the new installation profile
  6. Performed a new Standard installation
  7. After installation, turned on the Aloha module
  8. Tried to create a new Article
  9. Received "Fatal error: Call to undefined function filter_get_allowed_tags_by_format() in C:\Users\cweber\Sites\d8\core\modules\aloha\aloha.module on line 471" error

Am I doing it wrong?

webchick’s picture

As the testing instructions note, I would recommend just downloading the latest 8.x release of Spark http://ftp.drupal.org/files/projects/spark-8.x-1.0-alpha2-core.zip if you want to test the patch, or go to http://spark.webchick.net/8.x which is running it live.

cosmicdreams’s picture

OK, using the http://spark.webchick.net/8.x test I noticed a couple of things off from a User Experience perspective:

Currently the only way to interact with aloha is on the edit page and on that page it should behave differently that if it were to be used as an inline editor.

1. The text editors controls ("Format", "Insert" and their actions) go away when I focus on another element.
2. When the controls disappear is double-awkward because the text format drop down is still on the page along with the extra help text. This might just need to be a "expectation setting" task but it seems out of the ordinary.
3. I can't expand the textarea

That's all the time I have for now, but I don't really have more quibbles than that. I have a lot of questions about how I would build into the aloha editor for custom styles, or other crazy things. I'll see if if I can answer those myself my diving into the code.

CB’s picture

I think from an accessibility point of view, there is still clearly an issue that needs to be resolved and we've received confirmation that the Aloha guys are working on it. Considering this is a third party we're dealing with, theres no need for us to halt our progress while we wait.

Using the spark demo with Chrome & VoiceOver is iffy, but not a complete loss. The reader still reads out the contents of the body, it just gives no feedback to editing/updates. I'm not saying its not a problem, its just not a deal breaker (imo) as the user can still change to plain text and VoiceOver behaves perfectly again.

From a UX perspective, I dont like that the controls disappear when focus is lost. It looks pretty, but could cause some confusion.

quicksketch’s picture

Sounds like @cosmicdreams basically ran into the same issues I did. The disappearing/reappearing toolbar is difficult to accept.

@Wim Leers: On the link-editing problem: I was saying that the Enter/Return key works fine when entering a URL, but most users have been trained to *not* use the Enter/Return key on textfields because it will submit the form 99% of the time. So my instinct was to use the "tab" key to try to get back to where I left off, but the cursor starts at the beginning of the field. This issue is minor overall though and can wait for later.

@Dries: I'm not sure "commit it now to get more feedback" is the right thing to do. Clearly we already know there are a lot of problems. Is more feedback needed to address those problems? Or... well if we're already 100% on this (which seems like we have no choice) then I suppose committing it in its half-done state is acceptable, worst-case scenario we git rm aloha if it fails.

Everett Zufelt’s picture

AE is currently fundamentally broken for screen-reader, and possibly keyboard only users. Committing it in this state is a serious UX regression for these users. I am opposed to this choice by core maintainers.

Is it possible to switch text formats on a node? Yes. Does the average user know how to do this? I don't know, I'd guess not. Is the out of box experience broken for these users? Yes.

That being said, if it has already been determined that AE will be in core regardless the problems that currently exist I'll save my energy for other issues.

CB’s picture

@Everett, I'm wondering what the alternatives are? AE was picked for various reasons and the only option I can see is to revert the decision and pick something else. The basis of this is that there seems to be (at least at this point in time) no way to resolve the issue we are faced with. If contenteditable is not accessible, then another method must be used.

Everett Zufelt’s picture

@Christian

AE was picked from a list of alternatives, some of which have a better track record with accessibility. Based on a set of priorities AE was selected.

The Drupal community is working with AE to attempt to resolve the accessibility barriers present in its solution. I don't know when that will be possible. I do think that inclusion is a value of the community that needs to be respected.

If the decision is to keep AE, even if the accessibility barriers cannot be overcome, then we need to find the best possible solution to communicate the method of disabling AE to all users who may wish it to be disabled. Screen-reader users are not the only ones that face problems with these types of interfaces. How can we deliver this messaging in an effective, yet not terribly intrusive way? Or better yet, make it easy to use an alternative editor that is more accessible than AE?

webchick’s picture

Implementation-wise, AE is just a module, and can easily be either turned off altogether, or swapped out with another editor very easily. The changes that went in with #1782838: WYSIWYG in core: round one — filter types are only going to help in that regard.

I think we could explore options like we did with the Overlay in D7 of making it clear to screenreader users how to turn it off, if it indeed can't be made accessible, but we have a good while before RC1, and before we go down that road I'd really love to give the Aloha folks a chance to suss this out upstream. They've been nothing but awesome and responsive to our needs, and as Haymo (project lead of Aloha Editor) pointed out in #28, they're actively working on improving the accessibility of the editor as we speak.

webchick’s picture

Btw, for those who missed the links in the issue summary, here's the 160+ comment discussion about how we arrived at Aloha: #1580210: Figure out what WYSIWYG editor to use as well as Dries's announcement (where he noted that they changed the license of the project to GPLv2+ so we could use it) and you can also view a demo of Aloha Editor’s capabilities by its creator, Haymo Meran, at about 15:20 in the DrupalCon Munich Spark Core Conversation video.

Everett Zufelt’s picture

@webchick

I agree with everything you've said. The one thing I will point out is that swapping editors may be easy for site builders, yet impossible for users. It is not the job of Core to solve all problems on all sites. But, let's work together to brainstorm a way of ensuring a reasonably easy out of box content editing workflow for users with disabilities.

Bojhan’s picture

It seems like to make this accessible, we have to help out the Aloha guys to make it accessible. All this talk of committing without it being accessible seems a little premature, if we can simply help them make this upstream movement towards an accessible Aloha. I track the numerous issues around A11y on github, and there seems to be little activity even on the main á la carte issue (although it might be the activity is elsewhere, their community moves in different ways). And #1747930: contenteditable is not accessible to screen-readers seems to still be in exploratory mode. I hope WimLeers can perhaps help out, and try out technical implementations that make this accessible?

We should not consider disabling or overlay like toggle, as a viable option - there are accessible WYSIWYG editors. The idea is that out of the box Drupal is accessible, is something we committed to before - unless the core maintainers want to revisit that decision, I'd say anything that makes Drupal out of the box not accessible is critical and release blocking.

It doesn't seem this is like Views, that it has to be committed within the next week - so couldn't we just keep on working on the issues outlined? @quicksketch outlined a number of valid problems.

falcon03’s picture

@dries: unfortunately I have to disagree with you. I think Aloha mustn't be committed because we have lots of usability issues (in addition to the contenteditable issue).
Maybe we should solve them in the Aloha module and, when it will pass an accurate usability and accessibility review, it should be committed to core...

IMO we cannot take the "overlay" approach to accessibility as an example, since we must solve this problem (maybe for D8). And, in addition to this, let's say that a site administrator decides to use aloha for its users, since it is the default Wysiwyg editor. And let say that the site administrator doesn't allow users to use the "plain text" format: this would be a critical accessibility issue for blind users! And I don't think a CMS that makes of accessibility one of its strong points should allow this!

draftkraft’s picture

Hello, I would like to say, that Aloha Editor is not accessible, because we did not make it accessible yet. It does not provide ANY aria functionality as of the version implemented in drupal currently. That does not mean, that it is not possible to make it accessible, it just needs some time. We are experimenting with contenteditable in different screen readers such as JAWS, NVDA and voice over. You can follow the progress here http://aloha-editor.org/a11y.html. Please note that the toolbar is not yet fully aria enabled. The first step is to make editing possibile and the second step will be to make the toolbar accessible in a good way. Your feedback and help is very welcome!

rooby’s picture

I do a reasonable amount of fixing accessibility bugs when we do accessibility reviews and in this case (as a developer) I think I would be happy enough if there was at least one accessible solution. Maybe the site builder who wants out of the box experience might be less happy, but I don't know.

In this case, you can disable it.
It would be disaappointing if you had to but there is the option.

I also think that it is more time efficient to work on this in drupal at the same time as the aloha guys work on accessibility, instead of twiddling thumbs here while we wait for accessible aloha, then rushing or running out of time on integration later.
If we ended up in a situation where aloha was fully accessible but we couldn't implement in time that would be unpleasant.
We have the advantage of being able to work on the two concurrently here so why not do it.

I get that it is not ideal to ship drupal core with things that aren't accessible, but accessibility bug fixes can be rolled out in minor releases, whereas whole new features cannot.

So even in the unfortunate event that we don't have full accessibility in 8.0, it could be in 8.1 or 8.2 instead of 9.0.

So my vote is +1 for aloha.

eigentor’s picture

So how about focusing on easy toggling the rich text editor for Screen-reader users? Since as said an accessible Wysiwyg is probably rather novel as a concept, it is little surprising to find a good match to those users and seeing users.

Because of these reasons I do not really think a screen reader / tab-using user would want to use the wysiwyg at all.
So they want to swap it out, fair enough, give them a very easy process for that.
Iterate on making Aloha the mose accessible Editor ever - fine, I guess the Aloha team is fully behind this.

But putting into danger Drupal getting a wysiwyg because of this appears slightly out of proportion to me.

falcon03’s picture

@eigentor: I quite disagree with you. As a blind user, I want to use a wysiwyg editor, I want to use an accessible wysiwyg editor.

At this time the Aloha Edito team is working to make this product more accessible.

However, I don't think we are putting in danger having a wysiwyg in Drupal: isn't the feature freeze on December 1th? Why cannot we go on working on these issues in the Aloha module and, before feature freeze, decide if we should or should not commit the module to drupal core?
If the editor will become accessible, committing the module would be as easy as applying a patch or merging an appropriate sandbox as it happened for the views module....

effulgentsia’s picture

If we wait until Nov. 30 to commit this, then that interferes with preparing/evaluating other features for core inclusion that depend on this, such as Edit module. Also, if we try to squeeze 4 big features in on Nov. 30, because each of them wants to get as good as possible before initial commit, that has its own set of problems.

I don't think today is the right time to commit this, but ideally, in about 1-2 weeks would be. Hopefully, the AE team can demonstrate sufficient progress by then to make us feel good about that.

catch’s picture

I don't think this has had nearly enough review to be committed today, and we're way over thresholds at the moment regardless.

falcon03’s picture

@effulgentsia: ok, I understand these reasons. I can agree with you when you say that we shouldn't commit this today, but maybe we could do this by two weeks.

But I think that a critical accessibility issue for wysiwyg or in-place editing must be considered as a release blocker; how can we try to comply with WCAG 2.0 AA while offering some inaccessible features for the site builder and, eventually, for the site end-user? (See my previous comments for more explanations)...

effulgentsia’s picture

But I think that a critical accessibility issue for wysiwyg or in-place editing must be considered as a release blocker

I agree with this. And I think we and the upstream projects we work with will work hard to make wysiwyg and in-place editing accessible. If several months go by with no solution in sight (due to fundamental limitations of HTML5, current browsers, and screen readers), then I think an acceptable fallback plan is to make these features easily (and accessibly) disabled on a per-user basis, as we did for D7 Overlay. In #47, it seems you disagree with this being preferable to not including the features at all, but I don't understand why. And if this is an acceptable fallback, then I don't see the value in holding the initial commit up on accessibility concerns, when we can have that be a critical follow up.

Holding this up for another week or two to see how far AE gets with accessibility, and to allow more general usability reviews and improvements makes sense to me.

webchick’s picture

To catch's point in #53, it would be lovely if folks could do some code/UI reviews of this patch. If we need to have a larger discussion on "When does the accessibility gate block a feature and and when does it create a follow-up and what do we classify as critical?" that seems best done as a separate policy meta-discussion than here.

webchick’s picture

Also, as mentioned (and probably easily missed) in the issue summary, the code has already been reviewed in the Aloha module issue queue prior to posting this patch, specifically:

Reviews that happened already:
- @sun reviewed the PHP code: #1782326-3: Port Aloha 7.x-2.x to Drupal 8 (i.e. a 8.x-2.x version)
- @nod_ reviewed & approved the JS code for core inclusion already: #1782326-12: Port Aloha 7.x-2.x to Drupal 8 (i.e. a 8.x-2.x version) — the code has changed somewhat since though, but it should be in pretty good shape overall
- @psynaptic reviewed & cleaned up the CSS code: #1800010: Clean up CSS files — more reviews needed, though that could potentially happen post-feature freeze

falcon03’s picture

@effulgentsia: I don't think we can use the "overlay" approach for Aloha since this feature (IMO) is more important for every drupal user. Moreover, let me clarify my concerns with an example. Let's suppose that a new user that doesn't know anything about accessibility decides to start a new blog using drupal. Since his/her website is a blog, he/she decides that anonymous users can post comments to his/her posts. But, unfortunately, he/she assigns to anonymous users only the permission to use filtered HTML, which uses the Aloha Editor. If the editor has important accessibility issues, how could a blind anonymous user leave a comment to that website? How could a blind anonymous user disable the wysiwyg editor permanently (since it would be a pain to manually disable the Editor every time)?

I am not saying that AE won't become accessible (I am working with Draftkraft and Deliminator to solve the Contenteditable accessibility issue and improve the editor UI accessibility). But at this time I am very concerned by the Aloha commit. However, if every critical accessibility issue will be a release blocker, then I could agree with this commit...

So, to summarize my opinion:

  • Let's wait for one or two week to see what happens with Aloha accessibility (and general usability) issues;
  • If Aloha is more accessible and a blind user can at least type into a contenteditable element (also without considering text formatting options) receiving appropriate feedback by a screen reader, commit Aloha to core and keep working on other accessibility issues;
  • If Contenteditable cannot be used properly by a blind user, let's see what we can expect for the next months and what we cannot and make the decision to commit or not to commit
  • If we cannot expect better Aloha accessibility for the release date of Drupal core, leave the editor in contrib (I also wouldn't like to loose this feature, but accessibility is more important than having a new feature for me).
Wim Leers’s picture

Just to be perfectly clear: I do not expect, hope, anticipate to commit this within one week. There haven't been reviews, so that wouldn't make any sense.

What we do need, is to get more eyes on the PHP, the JS, the CSS and the overall state of the module. I want to know which things besides accessibility would need to be fixed before this could be considered to be committed.

Finally, I can't stress enough how much I would like to nail accessibility. A very significant portion of my time post feature freeze, pre code freeze would be spent on getting accessibility to a top-notch level.

webchick’s picture

Jess/xjm reviewed this and gave me a report in IRC, but I didn't see it represented here. There were two key pieces of feedback that came out of that:

#1825214: Block-level tag drop-down not correctly showing/filtering options based on enabled tags, which I think we should figure out before commit
#1825218: Provide interface to hand-edit HTML (comparable to "HTML vs. WYSIWYG view" in other WYSIWYG editors), which I'd prefer to discuss/tackle after commit

Wim Leers’s picture

A patch reroll at last. Despite being low on number of reviews, we've not been sitting on our hands doing nothing :)

Highlights:

Hopefully this will convince more people to start doing reviews :)

Related: in-place editing for Drupal 8 core patch has been posted too: #1824500: In-place editing for Fields.


Details:

Commits since e31dd5d (19 commits):

  • #1827626 by Wim Leers: Fixed jQuery UI 1.9.0 has a new .ui-helper-hidden-accessible class that our CSS doesn't yet support.
  • #1803578: Removed work-around for jQuery 1.8.2 compatibility that was introduced in commit e95172d; no longer necessary because it's been fixed upstream.
  • Updated Aloha Editor build to another post-0.22.2, pre-0.22.3 build; now comes with jQuery UI 1.9 and several bugfixes I'd files upstream.
  • #1825214 by Wim Leers: Fix undefined index errors.
  • #1825214 by Wim Leers: Add < blockquote>, and to the blackbox text format testing.
  • #1825214 by Wim Leers: Fixed Block-level tag drop-down not correctly showing/filtering options based on enabled tags.
  • #1806492 by Wim Leers: Make the 'Drupal' plug-in for AE obsolete or at least significantly easier to maintain.
  • #1824492 by quicksketch | Bojhan: Fixed Icons contrast fails WCAG validation.
  • #1808466 by quicksketch: Get rid of 'data-aloha-defer-init' attribute in the JS library definition.
  • #1808466 by quicksketch: Add temporary work-around to avoid needing a core patch to set Aloha Editor's currently required SCRIPT tag attribute to defer its initalization.
  • #1811350 by Bojhan: Added Show hand cursor (cursor:pointer) upon hovering options.
  • #1811346 by Bojhan: Added Lessen gradient.
  • Refactor and improve testability of aloha_add_format_settings().
  • WYSIWYG in core: round one — filter typeshttp://drupal.org/node/1782838 got committed, woohoo!
  • As of the previous commit (for issue #1815246), you now need to apply a different core patch.
  • #1815246 by Wim Leers: Stop relying on filter_get_allowed_tags_by_format(), figure out which tags & attributes are allowed through blackbox testing.
  • #1782372: Initial, rough support for the Image plug-in. Comes with searching all images on the site.
  • Updated Aloha Editor build to a post-0.22.2, pre-0.22.3 build, including my latest Image plug-in bugfixes that were merged.
  • Fixed #1813288 by Tobias Steiner: drupalcontenthandler.js uses Sanitize incorrectly
eigentor’s picture

So to do a review from a users standpoint: are the latest changes on some demo site or another way to get it work without applying all the patches one by one?

Wim Leers’s picture

Issue summary: View changes

Updating with link to demo site.

Wim Leers’s picture

All but the last 7 commits (they're listed newest to oldest) can be seen at http://spark.webchick.net/8.x/. We'll try to update that ASAP.

I have tried to make it easier to apply the patches though:

Alternatively, we've provided a handy shell script that does all of the above for you (including the optional "install profile" patch).

patrickd’s picture

btw, simplytest is now public, a current demosite can be build on http://simplytest.me/project/spark

Wim Leers’s picture

@patrickd thanks for the reminder! :) Here's a test site with alpha 4 of Spark: http://sc1c414b09eaec05.s2.simplytest.me/node/2, user/pass: admin/admin.

eigentor’s picture

On Webchicks Demo site http://spark.webchick.net/8.x it does not work at all at the moment. It looks forever like this: http://screencast.com/t/xczupQ6Q1k The spinner spins and spins...
Maybe this could be updated.

Wim Leers’s picture

#66: That's Aloha + Edit, and there's something off with Angie's demo site there. That's also not what this issue is about; this issue is about Aloha Editor on the back-end, i.e. on node/%/edit and node/%/add forms. So please go to http://spark.webchick.net/8.x/node/2/edit and evaluate that instead :)

cosmicdreams’s picture

Some of the issues i mentioned above still exist (the fact that the toolbar hides when the you click out of the text area.) but the other controls seem to work great. I love the insert, link and edit tabs, and I think they are good for now. My hope is that they can be revised after user testing.

When weighing having this in core and not having this in core, having this in core would be a great improvement for the many Drupal first-timers that I have worked with.

P.S.: is the fact that the tools hide when you click out of the area a "feature" or an unintended consequence of using Aloha without modifications.

Wim Leers’s picture

#68: thanks for the review! The fact that the Aloha Editor UI (i.e. toolbar) disappears when you click out of the textarea is more or less a feature of Aloha Editor, but at the same time it's a deficiency. For Drupal (i.e. on back-end forms), it most definitely is a deficiency. Support for multiple "always on" toolbars needs to be added upstream, and is referenced in the issue summary: #1789240: Ensure Aloha Editor UI can be *always* present, not only when actively editing ("static toolbar", prevents flashing).

mgifford’s picture

Issue tags: +a11ySprint

Looking forward to seeing if we can resolve the accessibility issues with this over the weekend.

quicksketch’s picture

I'd be very happy to get input on #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, which moves a good chunk of code out of this Aloha patch and into a separate "Editor" module. The matching patch for Aloha is at #1833720: Leverage new Drupal 8 text editor bindings.

quicksketch’s picture

Issue summary: View changes

Applying the patch of #1664602 is no longer necessary.

Wim Leers’s picture

A few, but important bugfixes.

  • #1809620: Browser scrolls to first Aloha Editor instance.
  • #1845264: Aloha stripping out all the things.
Gábor Hojtsy’s picture

Looked at the non-JS/CSS portions of the patch (text files and PHP :). Some minor and some more involved review points:

index 0000000..74f794d
--- /dev/null

--- /dev/null
+++ b/core/modules/aloha/README.txtundefined

+++ b/core/modules/aloha/README.txtundefined
@@ -0,0 +1,164 @@

@@ -0,0 +1,164 @@
+Installation instructions
+-------------------------
+
+Install as any Drupal module (so also install its dependencies), with the
+following additional steps:
+
+* Apply this core patch: http://drupal.org/files/drupal8.script-tag-attributes.4.patch
+* Also apply this core patch: http://drupal.org/files/drupal_wysiwyg-in-core-round-one-1782838-44.patch
+* Make sure you do a FRESH INSTALL of Drupal 8! (The default text formats have

Core modules don't have README files, especially not ones explaining how to patch core :D

The general API docs (PHP/JS) and the architecture docs would go to an aloha.api.php, the main docs in a main docblock and the API docs as hook definitions, etc.

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+/**
+ * @todo Improve Aloha Editor build for Drupal.
+ * @see http://drupal.org/node/1782348
+ */
+
+// @todo Establish a versioning scheme for our custom build.

It would be important to qualify these @todo's whether you want them to keep being in the code on commit or these are pre-commit todos.

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+    'file' => 'includes/pages.inc',

It is very uncommon for core modules to use includes/* directories. Core modules use $module.pages.inc type files usually.

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+  // Aloha.settings.plugins) to see the fullly expanded list of Aloha Editor

fully (only two l-s :)

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+        'type' => 'setting',
+        'data' => array('aloha' => array('settings' => array(
+          'plugins' => array(
...
+        'type' => 'setting',
+        'data' => array('aloha' => array(
+          'settings' => array(
+            'toolbar' => array(

The code style you use for the settings array is not consistent. I'm not 100% sure of the good code style here, I'd naturally just wrap lines even more so then in the second example. Did not look at other existing library definitions.

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+    'title' => 'Register the "drupal" bundle, this is the bundle that ships with this module, the "Aloha" module.',

Is this the title of the bundle? Sounds more like something I'd put in a code comment :)

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+  $ui_path = $module_path . '/aloha/drupal-ui/lib';
+  $libraries['aloha.drupal/drupal-ui'] = array(

It looks odd that the Drupal specific UI would be in the build(?) of the 3rd party library. At least the location makes me think so that is what is happening. Is that a requirement for how Aloha works that this needs to be in the build or is not in fact there, just looks so?!

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+ * Get a list of HTML tags and attributes that are allowed by a given text
+ * format, by performing black-box testing.

Ideally this would be on one line. Something like "Get allowed HTML tags and attributes with blackbox testing."

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+ * @param  string $format_id

extra whitespace

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+  module_load_include('inc', 'aloha', 'includes/format');

Again this organization of files to an includes dir is not common in Drupal core, would it make sense to implement this as a class on its own or another include with the module?

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+ *    - 'verdict': whether this text format is compatible with Aloha Editor
+ *       based on the two measures above

Whitespace problem on second line.

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+ * Adds Drupal.settings.aloha.formats and Aloha.settings.plugins.drupal. Ensures
+ * Aloha Editor has the metadata to deal with formats.

I think the second sentence would suffice. If you want to include the details make it an explanation after a newline.

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+ * @todo refactor; how to make the mapping of Aloha cleaner?
+ * @todo revisit mapping when this Aloha issue is solved:
+ *       https://github.com/alohaeditor/Aloha-Editor/issues/794

Is this pre-or post commit?

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+      // Drupal does not approve <b>.
+      'b' => FALSE,
...
+      'h6' => 'format',
+      // Drupal does not approve <i>.

Does not allow? Instead of approve. Also it just depends on the input format settings, right? Is that relevant or overly complex to deal with?

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+        // For some, there also exists an attribute mapping.

"there *is also* an attribute mapping"? I don't think "there also exists an ..." is proper English. Although in this case a foreign language speaker reviews a foreign language speaker's patch :)

+++ b/core/modules/aloha/aloha.moduleundefined
@@ -0,0 +1,701 @@
+/**
+ * Implements hook_process_html().
+ *
+ * A temporary work-around to avoid needing a core patch. Aloha currently
+ * requires a custom attribute on its script tag.
+ *
+ * @see http://drupal.org/node/1664602
+ */
+function aloha_process_html(&$variables) {
+  if (strpos($variables['scripts'], '/lib/aloha.js') !== FALSE) {
+    $variables['scripts'] = preg_replace('/(\/lib\/aloha\.js[^"]*["])/', '$1 data-aloha-defer-init="true"', $variables['scripts'], 1);
+  }
+}

Erm, just include that core patch here?!?

+++ b/core/modules/aloha/fonts/aloha_icons.svgundefined
index 0000000..ddf1146
--- /dev/null

--- /dev/null
+++ b/core/modules/aloha/includes/format.incundefined

+++ b/core/modules/aloha/includes/format.incundefined
+++ b/core/modules/aloha/includes/format.incundefined
@@ -0,0 +1,153 @@

@@ -0,0 +1,153 @@
+<?php
+
+/**
+ * @file
+ * Aloha Editor Repository API page callbacks.
+ */

Repository API page callbacks?! I think this was a bad copy-paste here.

Also, this is just 2 functions. Looks like it could just as well go to to the module instead of this non-standard code organisation technique.

+++ b/core/modules/aloha/includes/format.incundefined
@@ -0,0 +1,153 @@
+/**
+ * Provides the test cases used by _aloha_calculate_allowed_html().
+ */

I'd elaborate a bit on this because it looks like related to simpletest tests, but it is not at all.

+++ b/core/modules/aloha/includes/format.incundefined
@@ -0,0 +1,153 @@
+ * @todo  Refactor.
+ * @todo  Unit tests. Start from http://drupal.org/node/1782838#comment-6562024.

You plan this before or after commit?

+++ b/core/modules/aloha/includes/format.incundefined
@@ -0,0 +1,153 @@
+  // Run all the above tagtests to determine which tags and attributes are

tagtests => tag tests

+++ b/core/modules/aloha/includes/pages.incundefined
@@ -0,0 +1,68 @@
+/**
+ * Page callback: looks up relevant links in the site given a keyword.

links => nodes? :)

What happens here if no search module is enabled? Or if the site uses apachesolr instead of core search?

+++ b/core/modules/aloha/includes/pages.incundefined
@@ -0,0 +1,68 @@
+ * Note that also files with status = 0 are returned, i.e. those who were
+ * uploaded at some point in time, but not necessarily used.

who were => which were

+++ b/core/modules/aloha/includes/pages.incundefined
@@ -0,0 +1,68 @@
+  $result = db_query("SELECT * FROM {file_managed} WHERE SUBSTRING(filemime, 1, 6) = 'image/' AND filename LIKE :lookup ORDER BY timestamp DESC", array(
+    ':lookup' => "%$lookup%"
+  ));

I think its theoretically possible that file entities would use separate storage from the SQL system?! Not yet? Should this use EFQ2 instead?

Wim Leers’s picture

Addressed most concerns. Where relevant, I've written feedback:

Core modules don't have README files, especially not ones explaining how to patch core :D

:)

- Updated README.
- Excluded README from the core patch.
- BUT: I can't move all of what is currently in the README into an aloha.api.php file, for the simple reason that the Aloha module does not add new hooks; to e.g. add new Aloha plug-ins, one must use existing Drupal hooks!

The code style you use for the settings array is not consistent. I'm not 100% sure of the good code style here, I'd naturally just wrap lines even more so then in the second example. Did not look at other existing library definitions.

I did not yet change this. I absolutely agree that it looks weird, but it in fact *is* consistent, so I'm soliciting feedback first :)
- Wrapping everything on a single line will result in enormously long lines, hence I'm not in favor of this.
- Nesting every single level on a new indent level yields super deep nesting.
- Hence I've opted for a hybrid: A) for almost every plug-in, there's a aloha.settings nesting (since we want to put plug-in settings underneath that), hence I did not create a new indent level for that, B) most plug-ins also have a aloha.settings.plugins.load setting, hence I also put that one on a single line.

It looks odd that the Drupal specific UI would be in the build(?) of the 3rd party library. At least the location makes me think so that is what is happening. Is that a requirement for how Aloha works that this needs to be in the build or is not in fact there, just looks so?!

The build of Aloha Editor lives in the build directory, not the aloha directory. (This itself may be considered confusing of course.) Initially, the Aloha module didn't ship with its own build, and it made sense for Aloha Editor plug-ins that the module ships with to live in the aloha directory. Hence when the time came to add a custom build, we did that in the build directory.
So: build contains the custom Aloha Editor build, aloha contains our own (Drupal's) Aloha Editor plug-ins.

Also it just depends on the input format settings, right? Is that relevant or overly complex to deal with?

Correct. The problem is that Drupal users expect to never see a button in a WYSIWYG editor for either the <b> or <i> tag, and if we'd do blackbox testing on the "Full HTML" text format, they'd be allowed, and thus also show up in the WYSIWYG editor. Hence I'm explicitly disallowing them here, to prevent them from ever showing up.
This is very clearly not ideal, but until we have a way to configure the WYSIWYG editor on the back-end (@quicksketch made a great start on that with #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration), this is the best solution I can think of, even though it's hardcoded.

Erm, just include that core patch here?!?

I didn't do that because that was a separate API change (hence it would complicate reviewing this patch, no?). In any case: that patch has been committed, so I was able to simply remove all that code (and update aloha_library_info()) :)

Also, this is just 2 functions. Looks like it could just as well go to to the module instead of this non-standard code organisation technique.

aloha.format.inc containss 150 lines of relatively dense code. It's not specific to the Aloha module. It's conceptually separate from the Aloha module. Hence I though it'd make sense to have that in a separate file, especially because it's not needed on every page load (i.e. less code to load). If you still think it's a bad idea, I can definitely move it into aloha.module.

links => nodes? :)

What happens here if no search module is enabled? Or if the site uses apachesolr instead of core search?

Great question. It's intended to not just search nodes, but any content on the Drupal site that has a link. So: any entity type.

I reached out to @nick_vh to ask what the recommended method is to automatically use Solr when Solr is available, or really, any custom search system. He told me the solution is search_get_default_module_info(). I had a post-feature freeze issue for this: #1727922: Let aloha_repository_link use the default search module. Unfortunately, that's not really a solution: it's only intended to let its callers know at which path it provides search results, it does not provide a function to call to retrieve search results!
So, I went with the best approach possible, which is hook_search_execute() AFAICT. Now it also searches users, but it's still not as complete as I'd like it to be, but AFAICT it's as good as it can be given the current APIs in Drupal.

I think its theoretically possible that file entities would use separate storage from the SQL system?! Not yet? Should this use EFQ2 instead?

I think this can happen post-commit. I've added a @todo to mark it as such.

Gábor Hojtsy’s picture

1. On README: I don't know of core examples of code documentation like this, where it is reusing existing core mechanisms. The best I can suggest from prior experience is to start a drupal.org section with the docs. The plugin and the DBTNG systems are documented there. I don't see a trend in core of textual README files for invidual modules. I can see the usefulness, but due to how other things work, I don't think people would expect it there.

2. On settings array code style: the two examples I quoted were already not consistent, that is why I picked those :) Maybe I just found one that was not on the same line but wrapped extensively :) I don't have a strong feeling either way, would have probably not mentioned if I did not spot the inconsistency internally in the file.

3. I think the build/aloha directory naming is confusing. Why not build => aloha and aloha => plugins or build => vendor and aloha => plugins? I think its misleading that we name a local thing after the project but the place we actually put the vendor code is not named after the vendor.

4. On b and i: all right, makes sense.

5. On aloha.format.inc, I don't think 150 lines is that much, but if you feel strong about having a separate file, keep it in the same directory. Core modules don't have a pattern of subdirs (that I know of). Also you only have two files in that include dir and core modules usually have more in the same dir as the module.

6. On search: indeed, hook_search_execute() sounds fine. Much better than doing node searches directly. This would only find other entities which implement the search API (such as users), but is better than just having nodes.

7. On file storage: all right, sounds fine.

Bojhan’s picture

Title: WYSIWYG in core: round two — Aloha.module (Aloha Editor) » WYSIWYG: Add Aloha Editor to core
attiks’s picture

I'm confused, I just read this issue: #1260052: Candidate WYSIWYG editors so does it means this one has to be postponed?

webchick’s picture

Until/unless CKEditor can address an equivalent of the Aloha Blocks system, my interpretation is there's not community buy-in around CKEditor, so no, we're continuing forward here. The comparison though is posted to see if the data we came up with is valid. The Spark team is currently distracted trying to get #1824500: In-place editing for Fields to RTBC, hence the slower progress on this issue. But if anyone else wants to dig in here (or help there), we'd really appreciate the help!

effulgentsia’s picture

I just read this issue: #1260052: Candidate WYSIWYG editors so does it means this one has to be postponed?

Per #78, nope, work can still proceed here. But, given that regardless which editor eventually lands in core, there will certainly be some people who prefer a different one, another issue worth helping on is #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, which can help partition wysiwyg code better between editor dependent and editor independent code.

Sylvain Lecoy’s picture

You guys should checkout ckeditor 4.0. It went out on nov. 27 and now support "in-place" editing !

http://ckeditor.com/demo

webchick’s picture

We have. :) It's off-topic for this issue, but see #1260052: Candidate WYSIWYG editors where it's being actively discussed.

webchick’s picture

Status: Needs review » Postponed

Marking postponed on #1260052: Candidate WYSIWYG editors. The plot thickens...

effulgentsia’s picture

Status: Postponed » Closed (won't fix)

See http://buytaert.net/from-aloha-to-ckeditor.

Note that this can be re-opened for D9 or at any time there's reason to as these editors evolve.

effulgentsia’s picture

Issue summary: View changes

Fixed link to issue 1580210, "extensive research, and community consensus building".