Support from Acquia helps fund testing for Drupal Acquia logo

Comments

batigolix’s picture

Status: Active » Needs review
FileSize
1.62 KB

First modest attempt

jhodgdon’s picture

Status: Needs review » Needs work

Good start! A few comments:
- In the user interface, I do not think "textarea" fields are called "textarea". Aren't they "Long text" and "Long text and summary" or something like that? Help should be consistent with what the user sees... although since there are several types of "text area" fields, maybe we should say "plain text area" and leave it at that.
- Do we need to expand out the WYSIWYG acronym?
- I don't think the "for more information" line needs to be its own paragraph.
- I think rather than saying "replaces" plain text area fields, we might say "enhances"... or something... It doesn't really replace the field... hmmm...
- I also think rather than saying this editor "controls" the HTML ... I think the point you are trying to get across is that depending on the input format, you can configure what HTML tags the editor will insert... Hm....
- In the Uses section, are those actually the names of the default Drupal 8 text formats (and with that capitalization)? In Drupal 7 the default formats were "filtered HTML" and "full HTML" I think? Also I'm sure this "default" is only if you install with the standard profile, right? So if people are using different install profiles, they might not see this default.

So... How about something like this for the About section:

The CKEditor module provides a visual HTML editor, also known as a WYSIWYG (What You See Is What You Get) editor, for text area fields; specifically, (link)CKEditor(/link). The visual editor is normally configured to have different behavior depending on which text format is being used for the text area, so that it produces appropriate HTML for each text format. For more information, ...

batigolix’s picture

Thanks for the review. Here is a new attempt that kind of mishmashes your suggestions while still trying to save some of the original concepts.

Regarding your comments:

- In the user interface, I do not think "textarea" fields are called "textarea". Aren't they "Long text" and "Long text and summary" or something like that? Help should be consistent with what the user sees... although since there are several types of "text area" fields, maybe we should say "plain text area" and leave it at that.

New text attempts to fix this

- Do we need to expand out the WYSIWYG acronym?

I do not think we need to explain the acronym, but I included it. Just a thought, could we use the html acronym tag in hook_help?

- I don't think the "for more information" line needs to be its own paragraph.

Done

- I think rather than saying "replaces" plain text area fields, we might say "enhances"... or something... It doesn't really replace the field... hmmm...

New text attempts to fix this

- I also think rather than saying this editor "controls" the HTML ... I think the point you are trying to get across is that depending on the input format, you can configure what HTML tags the editor will insert... Hm....

New text attempts to fix this

- In the Uses section, are those actually the names of the default Drupal 8 text formats (and with that capitalization)? In Drupal 7 the default formats were "filtered HTML" and "full HTML" I think? 

They are, I fixed capitalisation

Also I'm sure this "default" is only if you install with the standard profile, right? So if people are using different install profiles, they might not see this default.
`

New text attempts to fix this

ifrik’s picture

It would probably be good to first make sure that the help for the text editor is up-to-date, and then build the ckeditor help out from that - if only because the ckeditor requires and extends what the text editor does.
I would also include more detail about how to configure the ckeditor, and how to add ckeditor to a new or existing input format.
Batigolix, if it's fine with you, then I spend some time on both modules in the train on the way back from DevDays tomorrow.

I also noticed that the help page fails to show up on the modules and on the help page.

ifrik’s picture

Sorry - it does show up. I forgot to run update.php.

batigolix’s picture

@ifrik: yes please, go ahead. assign the issue to yourself

jhodgdon’s picture

RE #4 - can you upload the patch again? The file is apparently empty.

batigolix’s picture

New attempt

jhodgdon’s picture

Priority: Normal » Critical

As per catch, missing hook_help for a new D8 module is a critical issue. The module should never have been added to core without help, according to the Documentation Gate.

So... Regarding this issue:
- Can we please not use the programming-side term "textarea" to refer to these fields? It should be "text area".
- Can we please not use "e.g.". Use "for example" and you'll also find that "for example" needs to be set apart by commas, as in "It can, for example, be used...".
- The first t() in the Uses section paragraph should not have '!url' in it (that t() does not refer to the on-line documentation URL).
- I've made this comment on other issues, but "See the (link)online documentation(end link)" violates accessibility standards, because the link text "online documentation" does not tell you where the link is going (which is to a specific documentation page, not the generic online documentation landing page). The link text should say something like "online documentation for the CKEditor module", and in that case, there is no point trying to separate out this sentence into a separate t(), because it will be different for each module. I think we should revert the help text standard to what it used to be.
- There is a stray ) in the Uses section after "Full HTML".
- I do not think that "CSS-styles" should be hyphenated.

jhodgdon’s picture

http://www.w3.org/TR/WCAG10-HTML-TECHS/#link-text for reference on link text accessibility.

ifrik’s picture

I'm a bit confused now about the use of !variable instead of @variable - I noticed it got changed on the Help text standard page, but I'm not sure why. https://drupal.org/node/632280

Should the links go through 'check plain' or 'insert as is'? https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/format...

batigolix’s picture

Regarding the link: I followed an example elsewhere (without further thinking).

I reversed the change I made in the Help text standard page at https://drupal.org/node/632280
I changed it, not because of the !variable but for the wording.

jhodgdon’s picture

Should we tweak the standard so that the link text for the online documentation would be "the online documentation for the Foo module" rather than just the "Foo module" part? That might make the link even more accessible?

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

#10:

- Can we please not use the programming-side term "textarea" to refer to these fields? It should be "text area".

done

- Can we please not use "e.g.". Use "for example" and you'll also find that "for example" needs to be set apart by commas, as in "It can, for example, be used...".

done

- The first t() in the Uses section paragraph should not have '!url' in it (that t() does not refer to the on-line documentation URL).

done. and also changed !url to @url

- I've made this comment on other issues, but "See the (link)online documentation(end link)" violates accessibility standards, because the link text "online documentation" does not tell you where the link is going (which is to a specific documentation page, not the generic online documentation landing page). The link text should say something like "online documentation for the CKEditor module", and in that case, there is no point trying to separate out this sentence into a separate t(), because it will be different for each module. I think we should revert the help text standard to what it used to be.

okay. also for your suggestion in #14

- There is a stray ) in the Uses section after "Full HTML".

gone

- I do not think that "CSS-styles" should be hyphenated.

yeh, in Dutch that hyphen is required ;) . it is gone now

ifrik’s picture

About the link to the online documentation at drupal.org:
I think we need to make clear somehow that this link is not the same as a link to a module help page within the site so if we just link to Foo module it's probably not clear enough.
And thinking of it, it would be good if users would get the idea that behind that link that there is more dynamic content, something that can extend over time, and might them offer more now then last time they clicked on the link.
Having said that... the only shorter phrase I can think of would be: For more information, see the <em>Foo online documentation</em>.

Wim Leers’s picture

I'm one of the folks who got this module committed. Bizarre that nobody said this before it got committed!

Also: why is the component for this "Documentation"? If it were "ckeditor.module", I would have found it.


+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -6,6 +6,24 @@
+      $output .= '<p>' . t('The CKEditor module enhances text area fields such as the Long text field with <a href="@cke_url">CKEditor</a>, a visual HTML editor, sometimes referred to as a WYSIWYG (What You See Is What You Get) editor. CKEditor is normally configured to have different behavior depending on which text format is being used for the text area, so that it produces appropriate HTML for each text format. CKEditor allows you to control what kind of HTML the content editors can use. It can, for example, be used to disallow embedding external images in the content or fix broken HTML. For more information, see <a href="@doc_url">the online documentation for the CKEditor module</a>.', array('@cke_url'=>'http://ckeditor.com', '@doc_url' => 'https://drupal.org/documentation/modules/ckeditor')) . '</p>'; ¶
The CKEditor module enhances text area fields such as the Long text field

This is utterly wrong and confusing. The CKEditor module does not do such things, the CKEditor module only implements the API that the Text Editor module (editor.module) provides. It is that module that is responsible for hooking in to fields.

And there, this would also be incorrect, because it doesn't matter which field type you use, it only matters whether it uses the "Text format" selector.

I'm afraid pretty much everything else in there is also inaccurate… :(

Also: trailing space.

What about this instead — and I'm sure that the wording is going to be too programmer-y, so just use it as a reference of correctness:

$output .= '<p>' . t('<a href="@cke_url">CKEditor</a> is a visual HTML editor, sometimes referred to as a WYSIWYG (What You See Is What You Get) editor') . '</p>';
$output .= '<p>' . t('The CKEditor module uses the framework provided by the Text Editor module, so that CKEditor may be enabled for any text format, and automatically show up wherever that text format may be used. CKEditor will automatically adjust to the text format's restrictions, so that content editors can only create allowed HTML. For more information, see <a href="@doc_url">the online documentation for the CKEditor module</a>.', array('@cke_url'=>'http://ckeditor.com', '@doc_url' => 'https://drupal.org/documentation/modules/ckeditor')) . '</p>';

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -6,6 +6,24 @@
+      $output .= '<dd>' . t('CKEditor can be enabled for text formats, such as Basic HTML and Full HTML. You can add more text formats that use CKEditor or configure the behaviour of the editor in the <a href="@url">Text formats and editors configuration page</a>. By configuring the text format you can decide which buttons and CSS styles are available in CKEditor.', array('@url' => url('admin/config/content/formats'))) . '</dd>';

Is it customary to refer to Standard install profile-specific configuration? Because that's what the "Basic HTML" and "Full HTML" text formats are: they're the default text formats for the Standard install profile.

Suggested:

+      $output .= '<dd>' . t('CKEditor can be enabled for text formats, such as Basic HTML and Full HTML. You can configure the buttons CKEditor provides for each text format by simply dragging them into the CKEditor toolbar preview on the <a href="@url">Text formats and editors configuration page</a>.', array('@url' => url('admin/config/content/formats'))) . '</dd>';
Pancho’s picture

How about this?

CKEditor is a visual HTML editor, often referred to as a WYSIWYG (%wysiwyg) text editor. It allows creating formatted content simply by using toolbar buttons instead of HTML tags, just like common word processors do.

CKEditor module leverages the CKEditor for Drupal, using the framework provided by the Text Editor module. CKEditor may be enabled for any text format, and will then automatically show up in text areas using the corresponding text format.

Being tightly integrated with the text format's constrictions, CKeditor will only create allowed, valid, and clean HTML. Drupal's implementation encourages semantically meaningful markup over purely decorative formatting in order to make sure the data is both usable and properly presented in different contexts.

On the Text formats and editors configuration page you can enable CKEditor for your text formats and configure the toolbar buttons by simply dragging them into the CKEditor toolbar preview.

For more information, see the CKEditor module's online documentation

$variables = array(
  '@ckeditor_url'=>'http://ckeditor.com',
  '%wysiwyg' => 'What you see is what you get'
  '@configure_url' => url('admin/config/content/formats'),
  '@doc_url' => 'https://drupal.org/documentation/modules/ckeditor',
)

[edit:] replaced PHP block by a blockquote in order to made the text more readable

Pancho’s picture

Actually there is a standard template for help pages. So in order to comply with the template my 2nd proposal would be:

About

CKEditor is a visual HTML editor, often referred to as a WYSIWYG (%wysiwyg) text editor. It allows creating formatted content simply by using toolbar buttons instead of HTML tags, just like common word processors do.

CKEditor module leverages the CKEditor for Drupal, using the framework provided by the Text Editor module. CKEditor may be enabled for any text format, and will then automatically show up in text areas using the corresponding text format.

Usage

Enable CKEditor for your text formats
You can enable CKEditor for any of your text formats listed on the Text formats and editors page. Standard installs come with CKEditor being already enabled for the 'Full HTML' and 'Basic HTML' text formats.
While configuring a %text_format, you may also configure the displayed toolbar by simply dragging a toolbar button into the CKEditor toolbar preview or out of it.
Use CKEditor when creating or editing content
t.b.d.
Use CKEditor for clean and valid HTML formatting
Being tightly integrated with the %text_format's constrictions, CKeditor will only create allowed, valid, and clean HTML. Drupal's implementation encourages semantically meaningful markup over purely decorative formatting in order to make sure the data is both usable and properly presented in different contexts.
Troubleshooting
The CKEditor toolbar won't work without JavaScript being enabled in your browser. With JavaScript being disabled, CKEditor enhanced text areas will gracefully degrade to regular ones.
If the CKEditor toolbar doesn't show up on a content creation page, you need to choose a %text_format that is configured with CKEditor being enabled. You can select the %text_format right under the text area on the content creation page.
If a specific text format doesn't show up in the selection or if there is no %text_format selection at all, you want to make sure the CKEditor enabled %text_format is also enabled for your user role on the Text formats and editors page. %text_formats may also be completely disabled in the field settings of a specific %long_text field, see the Content types overview page.
For more information, see the CKEditor module's online documentation
$variables = array(
  '@ckeditor_url'=>'http://ckeditor.com',
  '%wysiwyg' => t('What you see is what you get'),
  '@formats_url' => url('admin/config/content/formats'),
  '%text_format' => t('Text format'),
  '%text_formats' => t('Text formats'),
  '%long_text' => t('Long text'),
  '@types_url' => url('admin/structure/types'),
  '@doc_url' => 'https://drupal.org/documentation/modules/ckeditor',
)
batigolix’s picture

I think sentences like Drupal's implementation encourages semantically meaningful markup over purely decorative formatting and CKEditor module leverages the CKEditor still need some work to become human-readable, but it is good to get the perspective of the people who built the module. Would be awesome if we could always get such input.

We chose to group these issues (see #1908570: [meta] Update or create hook_help() texts for D8 core modules) under the component Documentation so that the documentation team members could easily find them.

ifrik’s picture

I've just made a patch with clearer help text for the general text editor module #2031743: Improve hook_help for text editor module, and now I could try to make this one human-readable as well :)

jhodgdon’s picture

RE #19 - also the headings for the Use section should be -ing verbs, like Enabling, Using, etc.

And I agree with the previous review -- we should refer to the Text module's help and only tell what CKEditor does that is different. Probably actually the CKEditor's help should be fairly minimal because all of that configuration stuff is actually provided by the Text module independent of what editor you use, right?

jhodgdon’s picture

Status: Needs review » Needs work

Changing status as per previous comments.

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.88 KB

attached patch:

- corrects info as per #17
- does not include any of the suggestions from #18 & #19
- reduces text as per #23

Pancho’s picture

Re #22:

we should refer to the Text module's help and only tell what CKEditor does that is different.

I'm not sure. I think we shouldn't rely too much on the Text editor module's help page because most users will go by the module that exposes the UI not the one which technically provides the UI. So they will regard Text editor module as some dependency that just works in the background. So if something doesn't work, they will look at CKEditor's help page, and be annoyed if they have to click through several help pages to find an answer to their question, and then only in a difficult to understand abstract way.
Because secondly: The more generic the module, the more abstract (= harder to understand the help text).
Thirdly, it's not like we were duplicating all the same stuff three or four times. We have only one of these text editors in core.

I agree that the paragraph under "Use CKEditor for clean and valid HTML formatting" in #19 should be slightly reworded. But "semantical markup" is not just a techie buzzword. Neither is the tight integration of CKEditor's ACF with our filter rules, the extra work done on ensuring clean markup that doesn't break in different contexts, or the forthcoming integration with CKEditor Widgets. These are unique features stemming from our CKEditor module that counter existing reservations of more demanding users about WYSIWYG editors. It also helps justifying why we're including this WYSIWYG editor, not others.
So while the wording might be improvable - the substance IMHO should definitely remain.

Re #24:

attached patch:
- does not include any of the suggestions from #18 & #19

Funny. Two out of three sentences in your patch draw from my suggestions in #18 & #19, but be it.

jhodgdon’s picture

Pancho: We don't want innacurate information put into help text. So the CKEditor help text should not be claiming that this module does things that another module is actually doing, and furthermore if someone builds a site with Text Editor and a contrib editor (I'm sure some will be available) rather than CKEditor, they should be able to understand what they're giving up (by reading the CKEditor help, which clearly tells them what CKEditor itself is doing vs. what the Text Editor module does).

And we also don't want to duplicate text, and have been following this policy of "module X help should describe only what module X does" all over core. For instance, the Field module explains what fields are and refers to the Field UI module to explain how to use the UI to attach fields to content types etc. It is not so terrible for help to modular and to have to make one extra click to get the full story on what a pair of modules accomplishes together, and also to have the benefit of understanding which module actually does what functionality (for instance, in the Views / Views UI case, separating them out gives you the benefit of understanding when it would be OK to turn off Views UI and save yourself some page load time).

The Text Editor module is not actually all that abstract, and its help does (or will soon) explain how to configure an editor generically and the other functions it is responsible for.

jhodgdon’s picture

Status: Needs review » Needs work

That philosophy aside, I reviewed the help in the patch in #24. I think it is getting better, but there are a few issues:

a) In About, not all the @ references in t() are in the array() -- @doc_url is missing.

b) In About, '@text_editor' => '/admin/help/editor' needs url() around the internal path and it should not start with / -- both links in Uses need this too.

c) The first sentence of the Uses bullet is: "CKEditor can be enabled for text formats on the Text formats and editors, You ..." Um... I think this is missing a . and the word "page"? And then the next sentence has the same link in it... Maybe those two sentences could be combined somehow or the link could be omitted in the second one?

d) The last sentence in Uses: "For more information see the Text Editor module." -- I think this should read ..."the Text Editor module help page" maybe? The link in the About section I think has the right text because there it's saying "in conjunction with the Text Editor module" but here we're saying "for more information read this other help page". Actually, let's say something like "for general instructions on how to configure editors, see the Text Editor help page" maybe?

e) I haven't tried out CKEditor in Core yet... Is dragging buttons all can do to configure the editor, or are there other settings?

f) We should probably say something about it being a good idea to match up the buttons with the filtering being done in the particular text format. So for instance if your text format only allows a few HTML tags, you only need those buttons and not buttons that would generate completely different tags. I don't think this comes automatically -- you have to configure the editor correctly in order to ensure that appropriate HTML is being generated for each text format, correct?

g) I think Pancho's suggestion about saying something about semantically correct HTML is an excellent idea. In "About" we have:
"It allows creating formatted content simply by using toolbar buttons instead of HTML tags, just like common word processors do."
This could be expanded by another sentence explaining that it also creates semantically-correct and valid HTML (which I think is a separate benefit beyond it being a visual editor). Let's try to keep this sentence short and factual though, and not expand on philosophy or saying "Drupal is great because we're doing this".

batigolix’s picture

Status: Needs work » Needs review
FileSize
2.22 KB

As for #25: I tried to summarize what was done in the patch because quite a bit of discussion took place between #15 and #24. I may have made mistakes, but I definitely read all the discussion and tried to include all.

New patch:

- fixes #27 a), b), c), d)
- includes a note about listing CSS styles, which seems to be the extra options available besides dragging buttons. (note #27 e))
- rephrases the About section to include the note about semantically-correct and valid HTML (note #27 g))
- includes a note about allowed HTML (note #27 f)). drupal is pretty smart with this. wysiwyg buttons are actually disabled for disallowed HTML. impressive! I struggled a bit formulating this feature and ended up with the following:

If the options "Display any HTML as plain text" or "Limit allowed HTML tags" are checked, then the buttons that generate disallowed HTML will be unavailable

Not so happy with the negations "disallowed" and "unavailable" in there.

ifrik’s picture

I've just done a new version of the help page for text editors in general #2031743: Improve hook_help for text editor module, so now I can have a go at this one as well - explaining more of the specifics. (and then I can also look at the filters page...)
I've also noticed that the page created by the patch does not gets listed on the Help page, so that needs fixing as well.

ifrik’s picture

I've added a whole bunch of uses, without explaining all buttons. I've mainly mentioned them where additional functionality is not directly obvious or where there could be issues with the way the filters are set up.
There are probably some that will be the same in other text editors, but I wouldn't know how to be sure whether anything could safely move into the general Text Editor help.
As mentioned already in #28: buttons for HTML tags that are not allowed are stripped from the toolbar when a user enters text - but you can still place them into the toolbar when configuring the text format.
I tried to make that clear, because I can see some administrators getting confused by that. The issue about buttons and filters should also be mentioned on the filters page.
I didn't get a list of CSS styles, and on my test site the external image didn't get displayed. Could somebody check whether the description is still correct when it works?
And I've taken moved the point about the valid HTML up because it's probably most relevant for users to know that this ensures that they can create good HTML without much hassle.

jhodgdon’s picture

Status: Needs review » Needs work

OK...

a) About section:
'@text_editor' => '/admin/help/editor' This needs to use the url() function. This is also missing from the first Uses section.

b) Throughout: Formatting is misspelled as "formating"

c) Is that actually true that if I create a text format that doesn't allow, for instance, UL/LI tags, that the buttons on CKEditor for making bullet lists will go away somehow? I haven't tried this in D8 but the WYSIWYG module and CKEditor for D8 definitely didn't do that automatically. Can someone verify the accuracy of this statement?

d) Throughout: "pull-down menu" should be "drop-down menu"

e) No one is very likely to read this help text when they're trying to configure the editor... The hook_help() page is intended to describe what the module can be used for, not tell you specifically how to configure it. The help for individual toolbar items should be displayed on the page where you are configuring the editor (via a Tour or using a different path in hook_help()), not in this help page.

Wim Leers’s picture

#31
c) Yes, that is accurate.
e) I agree that there is *WAY* too much help text in there now. All the button-specific information should be removed IMO.

batigolix’s picture

there is a unreviewed patch in #28 with a help text without any button-specific information

jhodgdon’s picture

I took a look at the patch in #28. I think it's pretty good...

I have a concern in About where it implies that CKEditor does not use HTML tags. Maybe it would be clearer if it said you don't have to type the HTML tags directly? Because they're definitely there behind the scenes.

In Uses, I think it could be a little clearer around where it says

If the options "Display any HTML as plain text" or "Limit allowed HTML tags" are checked, ...

I think it should be restated to say something like "If the text format includes the (those two) filters..." -- the way it is currently worded, it's not clear what/where these "options" are, because we're reading about configuring the CKEditor but this isn't part of CKEditor. And then the rest of the sentence is talking about CKEditor buttons, making it even more confusing.

Also in uses:

@text_editor' => url('/admin/help/editor')

That should not start with / inside url().

Other than that, I think this is a good level of detail and it's pretty clear.

Wim Leers’s picture

Also reviewing #28:

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -6,6 +6,24 @@
+      $output .= '<p>' . t('The CKEditor module enables <a href="@cke_url">CKEditor</a> for Drupal, using the framework provided by the <a href="@text_editor">Text Editor module</a>. CKEditor is a visual HTML editor, often referred to as a <acronym title="What you see is what you get">WYSIWYG</acronym> editor. It allows creating formatted content in semantically-correct and valid HTML, simply by using toolbar buttons (like in common word processors) instead of HTML tags. CKEditor requires JavaScript to be enabled in the browser. For more information, see <a href="@doc_url">the online documentation for the CKEditor module</a>.', array( '@doc_url' => 'https://drupal.org/documentation/modules/ckeditor', '@cke_url'=>'http://ckeditor.com', '@text_editor' => url('admin/help/editor'))) . '</p>';

s/semantically-correct/semantically correct/

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -6,6 +6,24 @@
+      $output .= '<dd>' . t('CKEditor can be enabled for text formats on the <a href="@formats">Text formats and editors page</a>. There you can also define which buttons are available in the toolbar of CKEditor. If the options "Display any HTML as plain text" or "Limit allowed HTML tags" are checked, then the buttons that generate disallowed HTML will be unavailable. If you choose to use the Styles button, you can provide a list of CSS classes that will be availabe to the user. For general instructions on how to configure editors, see the <a href="@text_editor">Text Editor module help page</a>.', array('@formats' => url('admin/config/content/formats'),'@text_editor' => url('/admin/help/editor'))) . '</dd>';
If the options "Display any HTML as plain text" or "Limit allowed HTML tags" are checked, then the buttons that generate disallowed HTML will be unavailable.

1. This is wrong, they're not "options", they're filters.
2. This is also wrong because it not only applies to these two filters, but any filter that restricts HTML.

Consequently, I believe this entire sentence should be removed.


If you choose to use the Styles button, you can provide a list of CSS classes that will be availabe to the user.

This is only relevant for a tiny subset of users, so we shouldn't mention this in the help text, even more so because it is optional.

Thus I believe this sentence should be removed also.


Finally, I do think it is useful to add a sentence like such:

"CKEditor will only allow users to use buttons and paste content that is also allowed by the text format's configuration."

Why? Because many people still believe that WYSIWYG editors generate crappy markup, and explicitly yet briefly mentioning that this is not the case for our CKEditor integration, is very valuable.

Wim Leers’s picture

Title: Create hook_help for ckeditor module » Create hook_help() for ckeditor module
Issue tags: +sprint, +Spark
ifrik’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

Sorry, batigolix, I had build on your patch #28 without properly saying so.
And yes... I've gone overboard with explaining the buttons. I've taken them out again, and will put something properly on the online documentation page later so that it could be used for tour module, or simply as reference.

I only left the one about the button to toggle between formatted text and source because I think it's important to know that you can use CKEditor even if it doesn't have a button for all the formating you want to allow (such as dl, dt, and dd for example).

I've added the proposed sentence about CKEditor formatting and pasting content - that solves what I had been trying to say, and couldn't quite formulate.

Wim Leers’s picture

#37: please provide an interdiff to make it easier to review in comparison to #28, I've already the same stuff many times in a row now.

ifrik’s picture

Sorry, I'm still finding my way around patches and interdiffs.
Here should be the interdiff to patch 27 (comment #28).

Wim Leers’s picture

Status: Needs review » Needs work

#39: No problem at all — we're all learning! :) In the very beginning, I also didn't see the value, but if you're reviewing patches often, then you'll soon start seeing the enormous value of having interdiffs!
Thanks — much appreciated!

And now, the actual review:

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -13,11 +13,17 @@ function ckeditor_help($path, $arg) {
+      $output .= '<p>' . t('The CKEditor module provides a toolbar for text fields that allow users to format content by using buttons instead of typing HTML tags, thereby creating semantically correct and valid HTML. CKEditor uses the framework provided by the <a href="@text_editor">Text Editor module</a>. It requires JavaScript to be enabled in the browser. For more information, see <a href="@doc_url">the online documentation for the CKEditor module</a> and the <a href="@cke_url">CKEditor website</a>.', array( '@doc_url' => 'https://drupal.org/documentation/modules/ckeditor', '@cke_url'=>'http://ckeditor.com', '@text_editor' => url('/admin/help/editor'))) . '</p>';

The "text fields" part is incorrect. CKEditor can only be used on text fields *that use a text format selector*. But, I think for the purposes of this help text, which needs to remain very high-level, I think this is acceptable.

"CKEditor uses the framework…" is wrong and should be "The CKEditor module uses the framework…". After all, CKEditor itself does not use this framework.

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -13,11 +13,17 @@ function ckeditor_help($path, $arg) {
+      $output .= '<dd>' . t('CKEditor has to be enabled and configured separately for individual text formats from the <a href="@formats">Text formats and editors page</a> because the filter settings for eact text format can be different. For more information, see the <a href="@text_editor">Text Editor</a> and <a href="@filter">Filter</a> help pages.', array('@formats' => url('/admin/config/content/formats'), '@text_editor' => url('/admin/help/editor'), '@filter' => url('/admin/help/filter'))) . '</dd>';

s/eact/each/ :)

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -13,11 +13,17 @@ function ckeditor_help($path, $arg) {
+      $output .= '<dd>' . t('When the CKEditor is chosen from the <em>Text editor</em> drop-down menu, the toolbar configuration is displayed. You can add and remove buttons from the <em>Active toolbar</em> by dragging and dropping them, and additional rows can be added to organize the buttons.') . '</dd>';

"When the CKEditor is chosen…" -> "When CKEditor is chosen…"

"the toolbar configuration is displayed" -> "its toolbar configuration is displayed"

+++ b/core/modules/ckeditor/ckeditor.moduleundefined
@@ -13,11 +13,17 @@ function ckeditor_help($path, $arg) {
+      $output .= '<dt>' . t('Toggling between formatted text and HTML source') . '</dt>';
+      $output .= '<dd>' . t('By clicking on the <em>Source</em> button, you can switch  between formatted text and HTML source code. This enables users to type allowed HTML tags even if there are no buttons in the toolbar. After toggling back to the formatted text view the tagged text will be formatted accordingly.') . '</dd>';

It is misleading to include this line of help text, because the "Source" button is not going to be available at all times. However, it is enabled by default in all text formats that Drupal core ships with, so I guess it's fine.

This enables users to type allowed HTML tags even if there are no buttons in the toolbar.

This is sending the wrong message. It implies that it is possible that you won't see any buttons at all. That should never happen. After all, then what is the point of using CKEditor at all? This should only be used by advanced users, and as such, I think this section of the help text should be scrapped entirely.

ifrik’s picture

Thanks for the quick reply. I'll make those changes.

But I disagree with the last point. I think for advanced users, site builders etc. it is relevant to know that there is a way to use CKEditor even when it does not provide a button for a specific HTML tag - and that they can switch between the formated text and the source without breaking the existing formating.

Sure, there won't be so many tags for which that is relevant for the average user, but even now the Basic and Restricted HTML Text Formats in D8 core are set up to allow dl, dt and dd - for which there are no buttons provided.

How about:

If the Source button is available in the toolbar, then users can switch between formatted text and HTML source code. In the source code view, users can format content by typing HTML tags even if no appropriate button is available in the toolbar. Any text tagged with allowed HTML tags will be formatted accordingly after toggling back to the formatted text view.

Wim Leers’s picture

it is relevant to know that there is a way to use CKEditor even when it does not provide a button for a specific HTML tag

Oh, definitely! But that's a very different message than this:

This enables users to type allowed HTML tags even if there are no buttons in the toolbar.

Do you see the distinction? The latter says "maybe zero buttons", the former says "maybe not a button for every tag".

So, your new suggestion is fine :) Excep that "Any text tagged with allowed HTML tags" sounds very weird. Maybe replace "tagged" with "formatted"? :)

jhodgdon’s picture

How about

Any text tagged with allowed HTML tags will be formatted accordingly after toggling back to the formatted text view.

==>

Allowed HTML formatting will be displayed appropriately after toggling back to the formatted text view.

ifrik’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
3.22 KB

Included comments #40-#43

The point raised it #40 that CKEditor does only provide a toolbar for text fields with a format selector is true for text editors in general, so I suppose it could be mentioned there.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good!

I have a few comments:

a)
The opening phrase:

The CKEditor module provides a toolbar for text fields that allow users to format content by using buttons instead of typing HTML tags, ...

is pretty hard to parse. I had to read it a couple of times to figure out what it meant, and I still think it's not grammatically correct for what it is trying to say.

Maybe
The CKEditor module provides an editor for formatted text fields that allows users to format content by using buttons instead of HTML tags, ...

b)

@text_editor' => url('/admin/help/editor')

As noted in a previous review, this needs to NOT have the initial / in url('/...'). This error occurs in numerous places in the patch.

c)

For more information, see the <a href="@text_editor">Text Editor</a> and <a href="@filter">Filter</a> help pages.'

In both of these cases, the link text does not really tell where the link is going, which is to the corresponding help page for these modules. For accessibility, link text needs to tell where the link is going, and if not, you need a title attribute on the link that does tell where the link is going.

d)

Formatting buttons for HTML tags that are not allowed, are not displayed to users when they edit a text field.

That comma isn't right... I understand that removing it would make the sentence a bit hard to parse but ... let's see. How about:

If a text format excludes certain HTML tags, the corresponding toolbar buttons are not displayed to users when they edit a text field that uses this format.

e)

For more information see the <a href="@filter">Filter</a> help page.

As in (c) ... the link text here should be "Filter module help page".

f)

... then users can switch between formatted text and HTML source code.

This doesn't really explain well (IMO) what happens. When you are in what is called "formatted text" mode here, you have toolbar buttons and the content is displayed formatted with HTML. I don't think that is conveyed well by this phrase. Also the word "then" is not necessary at all. Maybe:

... users can click this button to disable the visual editor and edit the HTML source directly.

Then you can get rid of the next sentence entirely -- I don't really think it's necessary. And then you can say "when they return to the visual editor mode" in the last sentence... what does that link/button say anyway? And is it really a button? (In WYSIWYG module in D7 it was a link below the editor area not a toolbar button)

ifrik’s picture

b), c) and e) fixed (and etched into my brain.... :-) )
d) good idea - I only slightly changed it because I think it's the user who uses the text format.
a) and f) I used the description "visual editor" in the about section as well as in the last point. I still want to make clear that the formatting depends on whether the HTML tag is allowed, and not on whether the button is visible or even exists, so I hope it works that way round.

And no - there is no more link under the text field, so users can't get confused between disabling the editor and choosing a different format.
The text that appears when you hover over the button simply says "source" which is why I emphasized that word.

ifrik’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

Status: Needs review » Needs work
diff --git a/core/modules/ckeditor/ckeditor.module b/core/modules/ckeditor/ckeditor.module
old mode 100644
new mode 100755

lets not do that please

ifrik’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

Thanks ParisLakis - looks like that permission change got carried along for a while.
I fixed it.
No changes in the text since #46.

Status: Needs review » Needs work
Issue tags: -sprint, -Spark

The last submitted patch, create-help-text-ckeditor-2029737-49.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Spark
jhodgdon’s picture

Status: Needs review » Needs work

I just reviewed the patch in #49, and I think it is very close!

I have a couple of suggestions:

a) In "Formatting and pasting content", it describes that buttons are not visible if a text format excludes tags, but it doesn't say anything about pasting content. Is there some specific CKEditor functionality that happens on paste that we need to know about? If not, let's take "pasting content" out of the header and first sentence, and if there is, let's describe it.

b) In "Toggling between formatted text and HTML source", there is a typo in the last sentence: "Allowed HTML formatting will be displayed accordingly after toggling back to the visual editor, independent on whether buttons for these tags are available in the toolbar.". It should probably be "independent of".

c) Actually though, this last sentence was not clear to me, and I'm not sure it's accurate. What really happens? For instance, if a text format excludes the A tag, how does the visual editor display links -- would it convert them to <a> or filter them out completely, or what? Using WYSIWYG/CKEditor in D7, there is no filtering in the display (they would display as links even if the filter would exclude them later), but maybe D8 is different? So... Let's change "displayed accordingly" to something that describes what actually happens in this module.

Wim Leers’s picture

Issue tags: -sprint

.

ifrik’s picture

Status: Needs work » Needs review
FileSize
3.03 KB
3.24 KB

Reworded according to the comments in #52.

c) Actually though, this last sentence was not clear to me, and I'm not sure it's accurate. What really happens? For instance, if a text format excludes the A tag, how does the visual editor display links -- would it convert them to or filter them out completely, or what?

Excluded HTML tags are not displayed when toggling back from HTML source to the visual editor. The filtering already is applied in the text editor and not only after the the content is saved.

allowed HTML tags are displayed as formatted

I can't really think of any better way than the very general "formatted". "Formatted text" would sound better, but that doesn't cover tags like hr.

jhodgdon’s picture

Status: Needs review » Needs work

I think we're getting close!

Those last two sentences though... Currently they read:

After toggling back to the visual editor, allowed HTML tags are displayed as formatted, independent of whether buttons for these tags are available in the toolbar. Excluded HTML tags are not be displayed after toggling back to the visual editor.

In the first sentence, I still think it's confusing to say that allowed HTML tags are "displayed as formatted", when really they are being used to format the text, right?

And I still do not know what happens for excluded HTML tags. Are they displayed as plain text? Are they completely stripped out? I guess it would depend on what the text format does, wouldn't it? So maybe those two sentences should be changed to say that the display in the visual editor is composed by first applying the text format (which might strip out excluded HTML tags or convert them to plain text), and then using the remaining/allowed tags to format the text... assuming that this is accurate, hopefully you can figure out a good way to word this. Thoughts?

In any case, that second sentence typo, where it says "are not be displayed" has to be fixed.

Thanks!

ifrik’s picture

allowed HTML tags are displayed as formatted...

I'm struggling with the wording because a tag like <hr> will be turned into a line and not into formated text.
As far as I understand it, HTML tags that are not allowed are really stripped out once you toggle back to the text editor not just 'not displayed'. When you toggle back to the source code, they don't show up again. They also don't come back if you allow the tag is question later.

There is the option to show the HTML tags as plain text, but that then applies to all tags - allowed and not allowed alike.

How about:

After toggling back, the visual editor uses the allowed HTML tags to format the text - independent of whether buttons for these tags are available in the toolbar. If the text format is set to limit the use of HTML tags, then all excluded tags will be stripped out of the HTML source when the user toggles back to the text editor.

Wim Leers’s picture

Status: Needs work » Needs review

#56 sounds good to me :)

Let's ask jhodgdon whether she agrees.

jhodgdon’s picture

Is that really true that the editor itself strips out the disallowed tags? That is not normally how text formats work -- normally the disallowed tags would be left in the source, and stripped at output time by the text filter.

So if someone can confirm this, I like the wording in #56 just fine.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +quickfix, +sprint

#58: yes, it is correct. Thanks to CKEditor's Advanced Content Filter functionality. See #1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings for more details.

RTBC as per #58 :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Wait, we don't have a patch with this new documetnation in it.

batigolix’s picture

Component: documentation » ckeditor.module
Status: Needs work » Needs review
FileSize
1.05 KB
3.35 KB

attached patch adds ifrik suggestion from #56

Wim Leers’s picture

#60: haha, oops!

#61: minor corrections: replace a comma with an em-dash, and wrap "limit the use of HTML tags" in <em> tags.

ifrik’s picture

Thanks everybody,
the patch in #62 works for me.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then. Let's get this baby off our plate. Nitpicked to death :)

jhodgdon’s picture

Agreed, looks great! Thanks for all the iterations!!!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work, folks!! *Really* sorry about this getting missed at commit-time. :(

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

.

Wim Leers’s picture

Issue tags: +CKEditor in core

.

Wim Leers’s picture

d.o tag monster

ParisLiakos’s picture

Issue tags: +Needs followup
diff --git a/core/modules/ckeditor/ckeditor.module b/core/modules/ckeditor/ckeditor.module
old mode 100644
new mode 100755

seems #61 re-introduced the mode change:(

Wim Leers’s picture

#70: ugh :/ I'd wondered where it had come from. A bunch of my other patches in this area were complaining about that.

@batigolix: could you do that? :)

jhodgdon’s picture

Issue tags: -Needs followup

I just took care of this.

Wim Leers’s picture

#72: where? :)

ParisLiakos’s picture

Wim Leers’s picture

Oh, hah :)

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