Text format tips do not explain that HTML is allowed when HTML filter is disabled

I went about trying to make this change and ran into a problem. Since the Text Input tool tips are tied to specific filters, there's no way to add a tool tip without a filter. There are a few options for how this could be fixed, but they all require a bit of hacking.

1. You could have a dummy filter. For example, in the default installation, a filter is automatically selected that shows the message, "All HTML Tags Can Be Used" that doesn't have any associated actions.
2. We could have a tool tip assigned to the absence of the "Limit allowed HTML tags." When that filter is not selected, the message would appear. When selected, the default message would show.
3. You could enable by default "Limit allowed HTML tags" and have all possible tags listed and allowed. One possible repercussion of this is that we would need a much bigger text input box for the list of allowed HTML tags. You could also switch how the filter functions and only show restricted tags versus allowed ones.

Is there a better approach?

sorry

I'm working on this right now. Patch to follow.

Please consider, not throwing up all html tags that are available - keep it short or just say that all tags will be available - minus those who aren't?

I would prefer Bojhan's solution as well. It makes a lot more sense to just add tags that should be omitted. For those not super-familiar with coding, I think that this would be easier as well. It's less of a burden to think of tags I want excluded in Full HTML than it is to sort through 91 elements.

This is my first patch so please let me know if there are conventions I didn't follow or if this is too much of a hack to put in core.

The patch just checks if either of the filters "Limit allowed tags" or "Escape all html" are enabled for a given format, and if not, adds a filter tip that says "All HTML tags are allowed." I didn't want to create an entire dummy filter, because then we'd have to create an exception for not displaying that filter elsewhere, which would be even more hackish.

Here's a screenshot of the filter tips after applying the patch.

minor nitpick from a 2 second review...

$allowed_html=array() should add spacing to conform to coding standards.

The screenshot looks great. I'll try to give a more thorough review later. Leaving as CNR for now so more eyes get on this.

Congrats, jumpfightgo, on your first patch! Yours passes the simpletest, which is more than I can say for my first patch. :P

This patch needs a bit of work. First, I'd suggest you run a patch locally before post it. You'd find that the patch as is doesn't work as intended, though I can see where you're going with it. Below are some notes:

1. The _filter_filter_tips_tags return $output; at the end, and as a result the html tips tables will not display.
2. The variable $base_url is referenced without being globalized, which throws a notice error for each input using the html filter.
3. beeradb pointed out a coding standard issue, and I ran across several others, including inconsistent else statements and improper concatination spacing. Check out the coding standards page. Someone could clean this up for you, but I find it's good to practice so you can develop a habit. That way, reviewers can focus on the concepts instead.

I have a couple thoughts / questions about the patch:

1. Would it be useful to factor out a function where you pass it a list of allowable tags, and it returns the tag table, without the additional markup? I can see this being more handy for outside code than something that includes the "This site allows HTML text.." text.
2. Would it be better to put the "This site allows HTML content..." text and the HTML entities table above the filter tips, so it doesn't need to be repeated for each filter using it? A check could be made to see if the HTML filter is enabled in any of the input filters and add that text to the page if so.
3. Is it misleading to output the html table for the Full HTML filter, since (correct me if I'm wrong) it doesn't include all the HTML tags you can use?

Thanks stompeers et al.

I'm having trouble getting to this right away but I'll tackle it soon.

Thanks stompeers for the careful response.

I fixed those errors and ran it through the coder module to try to clean up all those coding standards errors.

In regards to your comments/questions, I agree that the way we handle long tips for HTML tags is a little strange.

I wonder if our concerns with the compose tips page (what does that even mean, anyway?) is a separate issue from this patch, however. The markdown filter is another example where if you have a filter enabled for multiple formats the same long tip is printed multiple times on the compose tips page. Creating an exception to that rule for just HTML tags would not resolve the underlying problem with the way we handle long tips.

One alternative would be to leverage the help module to put a question mark next to every filter which you could click to see information about that specific filter. That would give users immediate information about how the current text format works, instead of sending them off the current page and forcing them to navigate a long list of descriptions that are completely out of context.

Could someone post a screenshot (I'm not yet able to apply patches). Thanks!

This is a very tricky issue to solve.

I think there is a flaw in the following code:

+    if (array_key_exists('filter/0', $filters) == FALSE && array_key_exists('filter/4', $filters) == FALSE) {
+      if ($tip = module_invoke('filter', 'filter_tips', -1, $format->format, $long)) {
+        $tips[$format->name][] = array('tip' => $tip, 'id' => 'filter-all-html');
+      }
+    }

This assumes that the only two filters that restrict HTML are the ones provided by Drupal core. However, there are contrib modules which provide filters that are intended to be drop-in replacements for the HTML filter as well (for example, the WYSIWYG filter module). We need to somehow accommodate those filters as well, or otherwise the "all HTML tags are allowed" tip will get added when it isn't true.

The attached patch implements a VERY crazy idea for doing that. It's such a ridiculous idea that I'm almost embarrassed to suggest it, except for the fact that it's really simple and it works :) All we do is define a completely invalid HTML tag (<abcdefghij>), run it through the text format, and see if it comes out unscathed at the other end. If it does, we can reasonably assume that the text format does not impose any restrictions on HTML tags, so we print the "all HTML tags are allowed" filter tip in that case.

Reviews? (A screenshot is also attached.)

By the way, I did not include any special handling for the "long" filter tips in this patch (thereby making it a much smaller patch). As discussed in #18 above, the way we handle long filter tips in Drupal is fundamentally broken anyway - and that comment really only scratches the surface of how broken it is :)

There's no way we can fix the whole system in this issue, so let's just do the simplest thing of using the same (very short) tip for the "short" and "long" tip formats in the case where all HTML tags are allowed. In the 99% use case, the person who has access to this kind of text format also has access to Filtered HTML, so when they are looking at the long filter tips page, they are already going to see the giant table of HTML tags for that anyway. Seeing the exact same giant table repeated twice isn't going to help them :)

Applied the patch, works as described, does what it says on the tin. :)

As for whether this is the right approach, someone more experienced will have to weigh in on my behalf.

That seems like an interesting approach David.
It's true that we shouldn't base the logic solely on the core input formats to check whether the show the "All HTML tags are allowed" tip, so IMHO it's better than the earlier patches.
The only use case that would give problems is when you would somehow enable a filter that strips out certain HTML tags, but lets all others pass (blacklisting).
However I can't really think of a valid scenario where you would do something like that...

This looks good.

I've asked sun to take a look at this, as the filter system maintainer.

+++ modules/filter/filter.module	30 Oct 2009 04:06:51 -0000
@@ -729,6 +729,18 @@ function _filter_tips($format_id, $long 
+    $fake_tag = '<abcdefghij />';
+    if (strpos(check_markup($fake_tag, $format->format, '', TRUE), $fake_tag) !== FALSE) {
+      $tips[$format->name][] = array('tip' => t('All HTML tags are allowed.'), 'id' => 'filter-all-html');
+    }

That's a really neat idea, David! :)

Unfortunately, it doesn't work out though.

Because, who says the text format is intended for HTML in the first place?

But anyway. :)

I tinkered about this issue in the past hour and yes, D7 and only D7 allows this, because of the changes that were already committed in #582378: Filter System clean-up.

I'll do a patch.

This review is powered by Dreditor.

So, erm, first of all: I agree that this missing piece of information is very confusing.

However.

Even my approach is flawed.

1) Unless a text format is updated, only enabled filters are stored in the database. That's a separate issue though, which needs to be fixed regardless of this issue. Anyway, to test this patch, you have to update the "Full HTML" format at least once - you don't even need to change anything, just update it.

2) But, well, when installing PHP module and also updating that text format, the same weirdness happens:

That's a problem, because HTML filter would still output "All HTML tags are allowed." for text formats that don't necessarily have anything to do with HTML.

Frankly, I don't think we can resolve this issue until we introduce the "target markup language" property to text formats, which I'm advocating for a long time already. That, however, we will probably see in D8 at earliest.

@sun, thanks for looking into this, but it is interesting..... When I look at the above screenshot, I think it looks exactly right :) The filter tips are correctly describing what is and is not possible to do with that text format. I think it's up to the person configuring the text format to make sure that what they intend it to do matches what it actually does.

In the particular case of the PHP code format, I also would say that it makes total sense to have HTML tags be allowed, because PHP is often used inline. For example, I might want to type this:

<h2>Some headline</h2>

<p>Hello, <span style="color:red;"><?php print check_plain($GLOBALS['user']->name) ?></span></p>

So saying "all HTML tags are allowed" is not only technically correct in that case, but also potentially very useful.

For other markup languages (like Markdown or whatever), that is where it gets really interesting. It's true that a Markdown format probably doesn't want to deal with HTML at all, but in D7, it's stuck doing that anyway. A correctly-configured Markdown format should usually (for security reasons) also include some kind of HTML filter in it - and in that case Drupal will already print something like "no HTML tags allowed" next to it anyway. So, same issue - with or without my patch.

With the target markup language thing you want to do for D8, I assume that would come down to classifying both text formats and filter tips by intended language, and only printing the tips that match? ... If so, my patch doesn't affect the feasibility of that one way or another. We'd simply classify the "all HTML tags are allowed" tip as only applying to HTML, and then it would work the same way as any of the others.

Does the above makes sense? If so, I'll upload my original patch again.

I'll upload my original patch again.

If at all, then we'd have to merge both patches. Which means to invoke check_markup() in the filter tip callback of HTML filter, and only, when HTML filter is disabled.

Partially, because of hook_filter_info_alter() - which would at least allow modules to alter away the default filter tip callback in case of a conflict.

However.

For other markup languages (like Markdown or whatever), that is where it gets really interesting. It's true that a Markdown format probably doesn't want to deal with HTML at all, but in D7, it's stuck doing that anyway. A correctly-configured Markdown format should usually (for security reasons) also include some kind of HTML filter in it - and in that case Drupal will already print something like "no HTML tags allowed" next to it anyway. So, same issue - with or without my patch.

The problem is that we cannot safely (as in 1000%) assume what filters will do to "markup that looks like HTML" in case a text format is not intended for HTML in the first place.

And, we cannot safely assume that an alteration to the markup that looks like HTML, which we passed in, means that it's no longer the (in)valid HTML we passed in.

in: <foo>
out: <foo about="foo">

It really gets non-HTML only, if

in: <foo>
out: &lt;foo&gt;

but that would mean HTML filter - OR - a HTML-filter-alike filter is enabled.

#626024: filter_list_format() hits database too often / filter_format_save() doesn't save all filters now contains a fix for the bug I mentioned in #28.

TBH, my experience with Markdown and other text formats/filters is limited.

We can try to do what I suggested in #31, i.e. merge both approaches, so this is done in the filter tips callback of HTML filter, and if this might be a regression for some edge-case scenario, then there will be a bug report against D7, but site builders as well as contributed modules can alter away the default callback.

Initial stab at this attached.

OK, I didn't understand what you meant by merging the two approaches, but now I see how it works (or rather, would work once the other issue is fixed):

1. The HTML filter is always there in the database,
2. Its tips callback will always get called even when it is disabled
3. So we can put the relevant code in there without forcing contrib modules like WYSIWYG Filter to implement this also (the problem identified in #21).

Clever. This could use a code comment explaining the theory behind it, I think :)

The main concern would be that as shown in the patch, we'd now be requiring every single filter in Drupal to put something like this at the beginning of their tips callback:

+  if (!$filter->status) {
+    return;
+  }

(And BTW, the patch is missing that change in PHP module.)

So is it worth it? (Especially given that we're trying to avoid API changes at this point...) Well, I guess it would definitely be worth it if there are other situations besides this one where a filter might want to print a tip even when it is disabled. I can't think of any great examples of that right now, but seems like there maybe could be?

Since the main goal of putting it the tips callback was to make it alterable, it is worth pointing out that it is still alterable (albeit via a less clean method) with the approach in my original patch. I believe it's the case that the filter tips always pass through theme('filter_tips') before being displayed, so you could for example use a theme preprocess function if you wanted to remove the "no HTML tags allowed" tip. Not ideal, but perhaps at this point the lesser of two evils?

+    //   so the proper way to check this would be to use PHP DOM functions.

Sounds like a good idea, although indeed could be a followup, since it's somewhat of an edge case -- plus the worse that happens in those cases is that it fails to output that tip, which is what is failing to happen 100% of the time without this patch :)

I'm not sure whether/when I'll have time to work on this. Regarding altering away the tips callback, I surely meant http://api.drupal.org/api/function/hook_filter_info_alter/7, but of course as you mention, it's themed as well.

I agree that this is a usability problem. However, since we need to do a slight API change to make it work properly, I'm deferring to webchick as to whether this can still be fixed for D7.

This problem was encountered during UMN usability testing in 2011. A participant wanted to add <img> tags to a node, and didn't realize that switching to Full HTML would have helped.

It was also suggested that it might make sense to add some example tags to the description (rather than just "all HTML tags are allowed".

Personally, I think there's still an issue with the text "Lines and Paragraphs break automatically"
If you don't come from a typographical or markup background, and you don't know what that means, the wording sounds negative.

Can I suggest...
"Line breaks and Paragraphs are published as you see them."
or
"Line breaks and Paragraphs are rendered automatically"

Thoughts?

I agree with the above. #39 - If the Filter is called Full HTML, we need to educate or at least make a novice end-user aware of what HTML tags even are. Only 4 or 5 examples would be necessary.

By now, we have a WYSIWYG editor in core, and hence the filter descriptions on the form itself are removed, because they're now irrelevant. They're still available at the "About text formats" page with full detail.

IMHO this is now irrelevant. I'll ask Bojhan to chime in.

Agreed, I don't think this is really relevant anymore.

Alright, closing :)

This isn't related to WYSIWYG editors. The issue title is a bit misleading, so fixing that.

#46: aha! Thanks for the clarification :)

Link http://www.drupalusability.org/node/106 is not working.