Oh well, do you ever thought that having the input format as a separated fieldset is so boring and clutters the interface? Did you ever had the need to have 2, perhaps 3 or even 4 textareas to input some text?
Did you ever thought why there is no textarea to insert a diferent teaser with a diferent input format?

Attached is my proposal for the evolution of textarea element. Instead of generating a collapsed fieldset, this generates a textarea status bar, allowing to be the placeholder of some interesting functions. (if no Javascript present it simply maintains the input format as a static fieldset)

The basic idea: when the user clicks the "input format" button it shows a popup with the options. After selecting it puts the label "input format: (the selected input format)"

Do you like it? Any ideas?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

magico’s picture

FileSize
49.8 KB

The open version...

Please give me some feedback, so I can have sure that it is nice to implement this.

magico’s picture

For those wondering... yes I would also implement the AJAX version for limit input within the textarea!

stBorchert’s picture

Looks very nice.
Did you check how it downgrades if no js is enabled?

mfer’s picture

Can't you essentially achieve this look with CSS already?

magico’s picture

#3: Yes. It will downgrade exactly as the current collapsed fieldset does.

#4: No. Some javascript needs to create a "div" like the current resize div for textarea; then CSS should be used.
The problem, is that the "textarea status bar" should go along with the grip, and it is necessary at drupal core level to identify each "input format" fieldset with the correspondent textarea.

drumm’s picture

The design doesn't fit into Drupal very well. Lets stick to white text on black with icons used sparingly for a clean interface. Themes can deal with flashier designs if they want.

What does the "Letters 50 / 250" text mean or do? There is nothing to explain it.

I think the biggest win we could have here is actually showing the current input formatting help when collapsed. Something along the lines of:

Filtered HTML
* Allowed HTML tags ....
* ....
more help   change input format
magico’s picture

The design doesn't fit into Drupal very well. Lets stick to white text on black with icons used sparingly for a clean interface. Themes can deal with flashier designs if they want.

Agree. This is a preview of what I have in mind. Off course, that with CSS it is possible to achieve any layout.

What does the "Letters 50 / 250" text mean or do? There is nothing to explain it.

As I said before, it serves as an example for the implementation of javascript character limiter. This way we can configure textareas to limit and display the current limit of allowed characters.

I think the biggest win we could have here is actually showing the current input formatting help when collapsed.

The other big win, is to attach the "input format" to the textarea, giving a much better view of what belongs to what!

If it's ok, I can now do some patches to work on this idea...

Chris Johnson’s picture

Yes, i think this idea is significant improvement to the usability of the text input interface.

drumm’s picture

How will this look in code moving forward? Maybe a #formatable attribute on textareas, which the filter module expands into the form element?

I would recommend sticking to doing one thing at a time. There is putting each input format next to its textarea in code and any UI changes. Maybe a round of standardizing input format usage patterns we have now and then make a clean and simple change like the one I suggest above? If I were doing this I would complete any changes to form arrays before tackling the UI.

magico’s picture

#9: Right.

Currently at the function node_content_form() we have the following

  if ($type->has_body) {
    $form['body_filter']['body'] = array(
      '#type' => 'textarea',
      '#title' => check_plain($type->body_label),
      '#default_value' => $node->body,
      '#rows' => 20,
      '#required' => ($type->min_word_count > 0));
    $form['body_filter']['format'] = filter_form($node->format);
  }

Are you suggesting that we should move to:

  if ($type->has_body) {
    $form['body_filter']['body'] = array(
      '#type' => 'textarea',
      '#title' => check_plain($type->body_label),
      '#default_value' => $node->body,
      '#rows' => 20,
      '#required' => ($type->min_word_count > 0),
      '#formatable' => array('format', $node->format));
  }

We must identify the textarea to be "formatable", by which element ID and what is the current set input format. Do you agree?

drumm’s picture

I'm thinking even simpler-

'#format' => $node->format,

I switched the term since I realized the actual existing format has to be put somewhere. Filter module could expand this to a full form element in filter_alter. This might be related to the upcoming 'widgets' idea on top of formapi.

I could be wrong on this syntax, the main point is the input format item needs to be in a consistent location in form arrays and the syntax could ideally be simplified.

magico’s picture

Assigned: Unassigned » magico

ok, I'm gonna do some work on this idea. now!

chx’s picture

FileSize
3.56 KB

You think I have not tried this? I find your lack of faith disturbing :) This patch won't apply any more to HEAD, it's a 2006 summer code, but it contains most of the necessary tidbits (it's part of a much bigger totally bogus effort. I hope I picked the right parts.

eaton’s picture

FileSize
1.9 KB

I haven't had a chance to look closely at this yet, but it now applies and it doesn't appear to break things. ;-)

webchick’s picture

Awesome idea, Neil. Won't have time to work on this, but subscribing so I can maybe help test.

chx’s picture

Eaton's reroll missed out quite some bits.

magico’s picture

chx: could you explain a bit your code. It seems to me that what this does it's find all textareas and "process" them to reposition the filters (and that means a bit more CPU time to construct forms).

What are the advantages of this over a simply attribute that associates a specific textarea with a input format?

Resuming, (I think) you propose a situation where the "filter" inserts itself within textareas. OTOH, I think it would be much better have an explicit "attribute" to allow a textarea have or not an input filter defined (following drumm's idea).

What do you think?

sun’s picture

Subscribing. http://drupal.org/node/81297 is similar to this issue. Anyone who wants to follow up this issue should completely read that issue before.

eaton’s picture

FileSize
4.2 KB

That reroll wasn't 'weak,' it was just plain broken. ;-) A set of changes got saved into the wrong drupal install, and the patch was rolled against an incomplete version. Doh. ;-) In any case, here's the correct one.

chx’s picture

yes you could add a format element to textarea and get it processed to a filter_form and a textarea. It's another way of doing.

This code let's you find your sibling textarea and stick to it.

Feel free to code the first one but be extremely careful with parenting.

drumm’s picture

Component: theme system » filter.module
Assigned: magico » drumm
Category: feature » task
Status: Active » Needs review
FileSize
5.96 KB

Here is an alternative patch. To add a input format selector to a textarea, use the '#format' attribute.

ontwerpwerk’s picture

interested - subscribing...

one could also plug this into WYSIWYG modules as a next step?

webchick’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

No longer applies. Dang, this would've been a nice improvement, but is an API change. :( I guess 7.x it is!

Gábor Hojtsy’s picture

Assigned: drumm » Gábor Hojtsy

I am taking on solving this issue as part of my RTE battle plan: http://hojtsy.hu/blog/2008-feb-04/supporting-wysiwyg-editing-better-drup... Thanks webchick for digging this issue up for me, it helped me go over the bumps I head while trying to implement something which so closely resembles this suggestion. Patch coming up early next week.

Gábor Hojtsy’s picture

FileSize
6.93 KB

Patch updated for Drupal 7. Still needs work.

- Used #input_format instead of #format for a more specific name. Depending on CCK coming into core for Drupal 7, there might be things like date format, number format, etc. specification needs on fields, so I thought being more specific is a strength this time.
- Simplified some code.

The remaining issue IMHO:

As we discussed this with chx on IRC, although this works nicely with one format selection only forms, if there are more fields with a format selector, you can only select one format for all of them (because the format selector is always named "format"). This would be solvable by naming the selector something else, and finding that in the input. If leveraging the default children item naming (there is code uncommented for this in the patch), we would get "body" and "body[format]" fields for nodes for example, which is not workable, as the later overwrites the "body" field and we never get the text data. So we need to think about something else.

I thought about naming the format selection radios $parentname ."-filter" (and keeping it on the same level), or putting both the textarea and the selector one level down into a wrapper field which would only be used for wrapping and parenting. How this works is also affected by whether the form is #tree or not.

Dries’s picture

This seems to make sense.

When reviewing this patch, I wondered if it is possible to "render" the form field in the FAPI validate and/or submit hook? That is, can we get the "rendered output" (post input format) to validate the form (as well as the pre input format format)? Being able to obtain the rendered output would be a big plus as we could XML-ify the form and better integrate with web services or programmable forms. I've had situations where it is difficult to validate a form field because you can't assume to know all input formats -- sometimes you really want to validate the output regardless of the input format. We should support this IMO.

I don't understand the code comment in filter_form(). Care to clarify it a bit more in the code?

recidive’s picture

I thought about naming the format selection radios $parentname ."-filter" (and keeping it on the same level), or putting both the textarea and the selector one level down into a wrapper field which would only be used for wrapping and parenting. How this works is also affected by whether the form is #tree or not.

CCK works around this by adding a wrapper, i.e. in our case it would be $form['body']['value'] for the textarea and $form['body']['format'] for the filter radios. This is similar to how date fields work. The problem is that it brings us some inconsistencies, i.e. a textarea with '#input_format' set, we would read the content on the submit function as $edit['field_name']['value'] and if it's not set, the content would be read as $edit['field_name'].

Gábor Hojtsy’s picture

recidive: yes, that's why I'd try to avoid it if possible.

Dries: I wonder what you mean by rendering the field in the FAPI validate and submit hooks pre- and post input format. You mean running the field through the input format selected? How would this help in XML-ifying the form? (So far, we only render on display and just get the data originally input when editing).

magico’s picture

Some of the work being done at #146624: Support wymeditor 0.5-a1 can be usefull to detect some needs about the input format filter.

Gábor Hojtsy’s picture

magico: it would be great if you could summarize thoughts from there. I looked at that issue quickly, and could not identify what you might have referred to. Thanks.

magico’s picture

On the issue I referred, there is some work around the implementation of a specific RTE case.

The strong points adopted on that implementation are:

  1. The RTE is based completely on an input format. This follows the idea that the "input format" controls the RTE and not the way around.
  2. On the input format we select the roles with access to the RTE, and for each role we can specify special configuration (all that happens on the input format environment)
  3. Some ideas about "runtime switching" when a different "input format" is selected. And an option to block the textarea making the RTE popup over the page instead of being bounding to the textarea limits.

Problems detected that makes difficult to implement some things:

  1. The input format should be "machine created" (using an hook) by a module, instead of being "human created". This would allow to bound a specific input format with a specific implementation. Also, a "module input format" should not be deleted and will always be installed/uninstalled by the module.
  2. It is not possible to configure the roles for default input formats. That is simple not right, because any input format should be configurable
  3. Using weights for input formats, would allow to have a list of input formats with different roles, that the system would loop until find the first available input format for the current user role.
mlsamuelson’s picture

Subscribing.

fall_0ut’s picture

Subscribing.

Gábor Hojtsy’s picture

FileSize
5.88 KB

Rerolled for HEAD as a start.

Gábor Hojtsy’s picture

FileSize
7.49 KB

I worked a bit with chx and he figured out some of the finer details of the tree building of formAPI, so this version now should work with multiple textareas on the page, but is still buggy (does not work) on incoming data. Since I need to run now, I am recording the patch here for future work. It would be great if interested parties could help out with the incoming data issue. Currently, we get "ArrayArray" in the textarea :)

magico’s picture

FileSize
42.85 KB
45.98 KB

And here it comes the screens that proves the idea express 1 year ago.

Just a note, this was done for 5.x-dev, and that means that for 7.x it will certainly be much more easier with Gábor's code; anyway, some of the code present at #146624: Support wymeditor 0.5-a1 ( comment #33) will certainly be useful meanwhile.

Screencast comes later :)

Advantages of this work:
1. We can change the input format at runtime and the associated editor
2. We need to change the textarea.js so it will not be executed on the page load, but just on the input format loading
3. No more "editor" attached to all textareas. Just only to those with an input format that allows the editor itself

David_Rothstein’s picture

FileSize
28.22 KB

I've been looking into the details of this issue for a little while, and I believe I may finally have a working solution in the form of the attached patch.

The patch definitely needs review, but I have left it as "code needs work" for the simple reason that it currently breaks every single form in Drupal except for one of them ;) This is because I got the one form working as proof of concept, and the rest are only broken for a simple reason (an API change to $form_state['values'], which is used by every submit and validate function in Drupal). Updating these forms to the new proposed API won't be hard, but it will be time-consuming, so I thought it would be wise to stop here and get reviews and feedback on the basic approach before putting together a final patch.

How to test:

  1. Log in and then apply the patch (you have to do it in that order, since the user login form is one of the ones that doesn't work ;)
  2. Visit admin/build/block/add and try to add blocks using the form there. Everything should hopefully work correctly. You should also be able to use the "configure" option for blocks that already exist.
  3. Go into block.module and remove the line '#input_format' => isset($edit['format']) ? $edit['format'] : FILTER_FORMAT_DEFAULT, that was added by this patch. The form should now appear without an input format selector and still basically work okay (there might be some PHP notices since the block module assumes a format will be there upon submission).

How it works:

I came to the conclusion that the best way to accomplish the goals of this patch was to go back to some of the original ideas that Gabor suggested in #25. Conceptually, what we're really trying to do here is have the input format selector be a "child" of the textarea, so creating a fake container for both of them and then inserting them both as siblings did not seem to me to be the best idea in the long run; if we want them to have a parent-child relationship, we really should insert it as a direct child. Furthermore, we don't want a solution that requires too much copying of code from Form API, because it seems to me that this mechanism should be reusable (right now we are trying to attach radio buttons as children of a textarea, but someday someone else might want to attach, e.g., checkboxes as children of a textfield). For this purpose, the attached patch provides a form_attach_children() function that can be used to attach form elements as children of each other (during the #process stage as used here). It also allows this to happen the normal way though form definitions, e.g.:

$form['text'] = array(
  '#type' => 'textfield',
  ...
);
$form['text']['options'] = array(
  '#type' => 'checkboxes',
);

This type of code does not currently work in Drupal, but the patch allows it to work for both #tree = TRUE and #tree = FALSE cases.

I think there may be a lot of benefits to implementing things this way. A simple example is that it should allow validation functions to do what Dries suggested in #26 (at least if I understood his suggestion correctly); if you have text at $form_state['values']['something'], you will be able to just check $form_state['values']['something']['format'] to find the input format (if there is one) and then run it through check_markup() if you need the rendered output for validation purposes.

Consequences:

The problem that needed to be solved here is the exact problem which Gabor identified in #25. Specifically, if we naively made input formats children of the textarea, we would get HTML like this:

<textarea cols="60" rows="15" name="body" id="edit-body" class="form-textarea resizable">
<input type="radio" name="body[format]" value="1" class="form-radio" />

which looks good in theory, but causes the second to overwrite the first (and a similar problem would exist in $form_state['values']).

Since we want a solution that does not cause the element naming to depend on whether or not there are children, my solution in this patch was to change the HTML produced by Drupal for all form elements to always look like this:

<textarea cols="60" rows="15" name="body[#value]" id="edit-body" class="form-textarea resizable">

regardless of whether or not they have children. Then, when a child is added, you get

<input type="radio" name="body[format][#value]" value="1" class="form-radio" />

which works fine. Similarly, in the submission and validation steps, any form values that were previously accessed at e.g. $form_state['values']['body'] are now at $form_state['values']['body']['#value'], etc.

That accounts for the bulk of the changes made by this patch. As far as I can tell, that's the simplest way for Drupal to be able to support having form elements as "true" children of each other... although perhaps it's a bit of an unfortunate complication in some ways. (If anyone can come up with a better way to do this, I'd definitely love to hear it.)

David_Rothstein’s picture

Note: I think there is a slight error in the logic for the form_attach_children() function that I'll need to fix... however, it shouldn't affect reviewing the rest of the patch (it's logic that does not get used at this particular time).

Dries’s picture

At first glance, this actually looks like a good API clean-up. It's slightly more verbose, but it does make the input format handling a lot cleaner and it could really benefit WISYWYG integration/switching as well. I'm inclined to say we should proceed with this but I'd love to hear other people's feedback.

catch’s picture

Status: Needs work » Needs review

No one will look at this if it's not CNR.

chx’s picture

Assigned: Gábor Hojtsy » chx
FileSize
4.14 KB

As per Dries request I was thinking and then implementing something. This has a few interesting characteristics: does not change the resulting form array. Works. Very very small change.

chx’s picture

FileSize
6.82 KB

I have added a bit of refactoring into form.inc -- moved a loop into a separate function -- making the filter_form_add a lot more robust.

sun’s picture

Wow. Quick notes:

- We should use #input_format instead of #format, because the term "format" is ambigious.
- Although it's intentional to tie the input formats right behind the textarea, we should not tie it that close ($form['#weight'] + 1/10000), so (upcoming) modules like Inline API can work with reasonable numbers, i.e. $form['#weight'] + 1/10 or similar.

chx’s picture

FileSize
7.09 KB

Added doxygen and removed some cruft from older times: $count does not need a separate counter, we already have one at hand.

chx’s picture

FileSize
7.12 KB

Ok, let it be #input_format. The weight stays because I am operating against the form.inc autoweight which uses 1/1000 and i want the input format chooser to stick to the textarea. You can use 1/100000 or just 1/20000 if you really want to stick something between the format chooser and the textarea.

catch’s picture

Tests still pass with this and it looks pretty tidy to me. Can't comment on the fapi internals, and of course +1 to the general idea.

David_Rothstein’s picture

@chx, thanks for looking into this. I only tried your patch out briefly, but doesn't it have some of the same issues that Gabor talked about in #25? Specifically, if you have two textareas (on the same level of the form) and want to attach input formats to each, one will overwrite the other and it won't work.

chx’s picture

Then you should not have one on the same level. Another solution is to add an input_format_key . But I do not think that's necessary. This patch is a lot cleaner and as a side effect lets any #process to add a sibling not just a child as before.

catch’s picture

Given CCK lets you do any kind of form layout you want, I don't think it'll work letting textareas overwrite each other's input formats.

Gábor Hojtsy’s picture

As I said to chx before, I don't think the format really needs to fall back on the form as it does in the patch (and as it makes it impossible to have two textareas with formats on the same level). This "special case" (actually very typical case) brokenness is something we'd want to avoid IMHO.

The patch keeps the format key falling back on the parent, so that basic node forms will still work as they do today. However, CCK keeps the format one level down and would not like a format field fallback to my understanding. Also, the format key on nodes is misleading, as it is only relevant and used for the teaser/body field and nowhere else. So it is an antiquated and old construct we are trying to work towards IMHO.

Granted, we discussed with chx, that waiting for fields in core to be a reality (and thus loosing the body field altogether from nodes) is not an option. So I propose to handle this format field for node forms on submission and push it down one level there. Otherwise leave it as is for the cck textareas et al.

moshe weitzman’s picture

subscribe

David_Rothstein’s picture

So, we seem to have two working patches here. That's approximately two more than most issues on drupal.org have ;) Given that, it would be good to settle on one and then get this in, right? This issue is holding back lots of other good improvements (or so I was told when I was asked to work on this)....

I personally think @chx's approach is kind of neat, but the reason I went with my approach in #37 is because I think that using array structures to represent a parent-child relationship is more natural and is easier for programmers to work with. (For example, if you want to "unset" a textarea, the input format that is attached to it will automatically go away too.) I think this is more extensible in the long run, especially for being able to attach other kinds of selectors to other kinds of form elements. My patch should handle those scenarios very naturally.

@chx's approach in #45 currently does not work with multiple textareas on the page, but Gabor pointed out offline that you could make it work by generating unique IDs via string concatenation; the end result would be that the format for the 'body' textarea is stored in $form_state['values']['body-format'] or something like that. I personally find this a little confusing because what it means is that you have to start thinking about sometimes Drupal Form API expresses parent-child relationships via the array structure (e.g., for fieldsets, when you are defining a form), but other times it expresses parent-child relationships via string concatenation.... which seems confusing, at least to me.

Of course, the advantage of @chx's approach is that it does not require an API change for people writing forms, whereas mine does.

So, feedback, please?! Thanks...

David_Rothstein’s picture

There's also the (somewhat independent) issue of how to expand form elements in the #process step: My patch introduces a simple function called form_attach_children() that gets the job done; @chx's patch breaks out some existing code into a more complicated but also more powerful function called _form_builder_handle_child(). See the patches for details. I'm not 100% convinced that the extra power is worth having (since if something is inheriting its parents properties, maybe you shouldn't be encouraged to add it as a sibling, as @chx's patch allows), but it is definitely there. Either function could be reusable in other scenarios, I think.

Leeteq’s picture

Subscribing.

chx’s picture

Changing the structure of form_state to + * $form_state['values']['mail']['#value'] = 'robouser@example.com'; is not something I would endorse. It buys us little while breaks a lot. Yes we do not keep BC but changing for the sake of change is not encouraged either. My patch really just adds a few lines of code to form.inc , most of the patch is copy-paste and yet it makes it more powerful.

moshe weitzman’s picture

Seems to be like we need Gabor or Dries to break this deadlock between the two patches. Either patch would be better than current state.

Gábor Hojtsy’s picture

Posted background info at http://hojtsy.hu/blog/2008-sep-04/do-not-let-wysiwyg-editors-rule-our-te... and asked for more reviewers. We really need more eyes on this, since it is so crucial to successful WYSIWYG integration.

moshe weitzman’s picture

I am at a loss for which patch to review.

chx’s picture

I am going to post a patch which will expand a textarea named foo to foo[value] and foo[format]. In node_save it will assign body[value] to node.body and body[format] to node.format. This will allow you to have a foo and a bar textarea on the same level -- the latter would become bar[value] and bar[format].

ximo’s picture

I haven't tried the patches yet (will do so later), but here is a more realistic mockup of how this might look in Garland:
http://skitch.com/ximo/idq1/input-filter-remix-1
http://skitch.com/ximo/idqu/input-filter-remix-2

Damien Tournoud’s picture

Status: Needs review » Needs work

@ximo: I like your mockups, that's pretty neat, but a little outside of the scope of this issue. Currently we are only trying to link the textarea and the input format programmatically. Reworking the couple to work as a true widget is for a follow up issue.

CNW as #59.

ximo’s picture

@Damien: I suspected so. The issue did start with a conceptual mockup, but has taken a more technical direction since (which is very good!). I've created another issue (#304330) for theming the Input format fieldset, a new GUI widget if you like. It should be mainly CSS and jQuery, not too difficult to achieve.

chx’s picture

Status: Needs work » Needs review
FileSize
3.88 KB

Here we go.

Status: Needs review » Needs work

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14148. If you need help with creating patches please look at http://drupal.org/patch/create

chx’s picture

Status: Needs work » Needs review
FileSize
5.25 KB

Rerolled against HEAD, changed #type to make it easier to theme separately and added some comments.

Gábor Hojtsy’s picture

Note that some of the original goals of the above approaches with having an additional property, not an additional #type were:

- it is easy to go from filtered to non-filtered (by adding another property)
- it does not change the form structure, so that you would look to ['item']['value'] and ['item']['format'] instead of ['item'] itself
- it allows for single line input fields to have format (eg. short quotes)

This latest approach goes against all three original simplification ideas making the API more complex / different depending on input format in the name of simplifying the underlying code.

chx’s picture

Note that it is not simplifying the underlying code, #64 was simpler. It is still easy to change, you need to change #type as well. As you need to handle the dfferent value structure (strings vs array) it's not a big deal. You do look at item['value'] and item['format'] -- I do not get your second line. And third, if you want that, we can go back to #64.

Gábor Hojtsy’s picture

The idea was that if you "format enable" an input field, you would not need to go in and modify the handler code to inspect childrens instead of the original item, but would only need to add code to handle the *additional* format item. So the original element value location would be kept intact, but an $elementname .'_format' or something would be available for inspection. So basically, painless moving from and to format enabled stuff with only adding one property.

chx’s picture

FileSize
5.14 KB

So here we are... we have $form['body']['value'] and $form['body']['body_format'] and the #parents are set accordingly. It's a bit tricky but should work.

Edit: so instead of trying to finding the parent to the element we now set two children. The textarea has the same #parents as the original textarea. The format element has almost the same but the last element which got a _format suffix.

chx’s picture

FileSize
5.12 KB

Reroll.

sun’s picture

FileSize
6.12 KB

I do not see a reason why we cannot allow any #type, hence, not limiting this to textareas and including #type textfield already.

Subtle difference in the form_process_input_format():

$type = $element['#type'];

Stay tuned for further test results.

sun’s picture

FileSize
6.1 KB

Even better,

- #type must not be reset at all
- original item's #weight now properly passed on

sun’s picture

I now have extensively tested this patch by creating and editing core nodes and blocks, but also - and I think this is more important for the overall goal - by implementing a sample module that contains a input format enabled textarea. -- The results are awesome.

As claimed by chx, a field's value is always found in the regular $form_state['values'] key. If a form field is input format enabled, an additional key containing the selected input format is added to the submitted values. For example:

array
  'storage' => null
  'submitted' => boolean true
  'values' => 
    array
      'title' => string 'TextFIELD, #weight = 200' (length=24)
      'title_format' => string '2' (length=1)
      'entry' => string 'Textarea without #weight.' (length=25)
      'entry_format' => string '1' (length=1)
      'what' => 
        array
          'ever' => 
            array
              'here' => string 'Textarea within 2 fieldsets, #tree = TRUE.' (length=42)
              'here_format' => string '2' (length=1)
...

This means that any textarea can be input format enabled at any point in time. In D6, filter_form() alters the whole form structure, leading to broken forms, if some other module performs a form_alter() on the body.

Hence, testing whether a form value is input format enabled (which may differ due to user role permissions) is as simple as

function mymodule_submit($form, &$form_state) {
  if (isset($form_state['regular-field-key-here_format'])) {
    $format = $form_state['regular-field-key-here_format'];
  }
}

I'd say that this patch is RTBC, but chx pointed out that this would be Gábor's decision... :)

Gábor Hojtsy’s picture

Wow, looks quite clever. I love how it keeps the original format of the data from the form. For this to be RTBC, it needs a phpdoc block explaining what form_process_input_format() does. I am also in the dark on why the ID clearing is required, although I see how it is done to work with the ID generation code.

Finally, this patch modifies the node forms, but what about other core forms using the format selector?

sun’s picture

FileSize
12.58 KB

- by sdboyer: Added PHPdoc block.
- by sun: Added "support" for block and comment. No other occurrences of filter_form() in Drupal core.

The id clearing is required because of form_clean_id(). For me, this clearing was trivial, also because chx already added an inline comment right in front of it?

chx’s picture

FileSize
9.83 KB

A lot cleaner patch thanks to Gabor's comment. Those unsets in there were not required for this kind of implementation and removing them allowed me to remove more, unnecessary changes. Now form.inc changes are absolutely driven to minimal.

chx’s picture

FileSize
9.57 KB

It was not driven enough. Even more stuff removed!

sun’s picture

Status: Needs review » Reviewed & tested by the community

oh my god, you're driving me insane, chx!

To clarify in advance: #value does not need to be reset for the parent element, because D7 requires markup elements to explicitly define #type = 'markup'.

Without the other changes throughout core, this 3 KB patch is a major improvement towards input format and wysiwyg handling in core.

chx’s picture

Erm, no, #type markup is still optional but it prints #markup not #value .

David_Rothstein’s picture

Actually more like 10KB than 3KB, but yes, point taken ;)

I support this patch going in. I haven't reviewed it carefully enough to put my name on the RTBC, but it looks like it should work, and it's definitely clever.

However, I would like to request that after this goes in, the issue be kept open so that my patch in #37 can actually be considered. I posted that patch over 40 comments ago and it really didn't get much of a review despite the fact that it solved the task at hand. It's true that I was proposing what would have been a very large patch, but the suggestion to make these changes across Drupal core was not made on a whim; the point was to improve Drupal's Form API so that it is more flexible and natural to use, so that problems similar to this one won't keep coming up again! The current patch sweeps these issues under the rug.

That said, the fact that (a) the current patch seems to work, (b) either approach would have the same #input_format syntax for input formats, and (c) this issue is important to resolve quickly since others depend on it, I think it makes sense to go with this now.

chx’s picture

@David_Rothstein, I am not supporting adding #value to form_state['values'] -- and that's a separate issue anyways. If you have a general issue with FAPI please file an issue and crosslink it to here.

sun’s picture

Small addition: This patch does not break existing functionality. Before Gábor's comment in #74, Block and Comment modules were not updated to use #input_format, but they still worked just like they did without this patch. This is because filter_form() is not altered by this patch.

Damien Tournoud’s picture

I still don't see the point in wanting to keep backward compatibility here, for two reasons:

(1) The text itself and the format are currently separated in 'body' and 'body_format'... But none of them make sense without the other! The idea to keep them closely linked in a 'body' array with two keys 'text' and 'format' makes perfect sense to me. And we could refactor other API with that convention (check_markup, especially).

(2) Having a 'textarea_formatted' #type, along with a 'textfield_formatted' makes total sense, and it would probably be easier to theme down the road. Today, how to you plan on implementing #304330: Text format widget, if the textarea element and the filter selection are kept separated?

For this two reasons, I strongly prefer the approach of #65 (and we already discussed this with chx on IRC).

Note: I'm not removing the RTBC of that patch. I will not go against the majority if you think my arguments above are not worth considering.

sun’s picture

We should differentiate between submitted form values and the form builder structure. Regarding submitted values, as mentioned in #73, submitting 'name' and optionally 'name_format' on the same tree level is the most clean way I can think of. 'name' will always exist, only 'name_format' is optional, depending on the input field.

However, regarding the form structure, both form elements are shifted below a new parent element. Implementing additional FAPI types for the text* field elements would be wrong, because we do not want to alter the presentation of a textarea or textfield itself. Instead, for #304330: Text format widget, we want to add a surrounding container, i.e. alter the parent element, so the input field and input format selector are, as a unit, able to be identified, styled, and altered via JavaScript and CSS. Additionally, Wysiwyg API is able to re-use that information to alter the client-side input field interface. So we are rather speaking of changing the #type markup of the parent element to a new #type that aids in theming, if necessary at all. But this is outside of the scope of this patch and may be added via #304330.

Gábor Hojtsy’s picture

@Damien Tournoud: The reason for keeping 'body' as 'body' and not for example ['body_field']['body'] is that keeping the simpler structure you can walk in and out of formatted textareas in your modules, without a requirement to modify your code. Eg. in CCK if the user selects plain text input, the text will be in 'body', if the user selects formatted input, you guessed it, the text will be in 'body'.

Damien Tournoud’s picture

Eg. in CCK if the user selects plain text input, the text will be in 'body', if the user selects formatted input, you guessed it, the text will be in 'body'.

I have to disagree. In a formatted input context, the text has *no meaning whatsoever* without the filter. Keeping them closely tied together in a data structure makes a lot of sense.

Gábor Hojtsy’s picture

Ok, the Drupal 7 maintainers will have their final say anyway.

sun’s picture

@Damien Tournoud: Keeping them closely tied together in a data structure makes a lot of sense. That's exactly what this patch does: Both in the form builder structure and in the submitted form values, text and filter are closely tied together, as outlined in #84 already.

Note to core maintainer: As far as I can understand recent follow-ups, chx, Gábor, and me vote +1 on the latest patch in #77.

chx’s picture

Here is what we can do. I give a architecture overview for Dries, he approves it and then we can play the nitpick game with webchick. So, #type textfield and textarea can get a #input_format property which defines the input format to be used. Simplified example:

$form['body'] = array(
  '#type' => 'textarea',
  '#input_format' => $node->format,
);

When the form is processed, there will be a $form['body]['value'] element for the textarea and a $form['body']['format'] element for the filter form. After submit, $form_state['values']['body'] and $form_state['values']['body_format'] holds the code -- _format is a suffix so multiple elements can get a filter form.

sun’s picture

FileSize
11.26 KB

Rewrote PHPdoc and added inline comments as per Dries' request.

webchick’s picture

Those comments are not *quite* there yet, but are very close.

Tomorrow when it is not 4am, I'll make an attempt at cleaning them up a bit and hopefully you or chx can help me refine them. Then I'll get this sucker committed, and roll our first unstable release tomorrow night. Woohoo!

sun’s picture

FileSize
11.39 KB

Now my last attempt still including limited grammar skills.

jjeff’s picture

This is a great patch. It will open the door for WYSIWYG input formats which can enable themselves on a textarea (via JS) when their associated radio button is selected. Without this patch, a WYSIWYG would need to "guess" its associated textarea.

sun’s picture

FileSize
2.24 KB
116 bytes

Attached is the custom module I used to extensively test the new property and behavior.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.59 KB

Patch with refined PHPdoc and some renamed variables, as a result of sun and I pastebinning back and forth.

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
12.97 KB

Now with updated filter tests, and final PHPdoc.

Related tests pass:

1362 passes, 0 fails, 0 exceptions

sun’s picture

FileSize
14.03 KB

5796 passes, 9 fails, 0 exceptions

...due to

- Drupal running on Windows, resulting in:

Expected file permission to be 664, actually were 666.	Other	file.test	677	FileCopyTest->testNormal()	

- missing changes in PHP filter tests.

Updated PHP filter tests accordingly, tested them again:

74 passes, 0 fails, 0 exceptions

RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Big green button: PRESSED!

Thanks SO much for your hard work, folks! :)

webchick’s picture

Status: Fixed » Needs work

Oh wait. Not *quite* fixed yet.

1. We need a blurb in the upgrade guide @ http://drupal.org/node/224333 about $form['body'] changing position.
2. We need to include this new property in the FAPI Reference in HEAD.

Thanks!

Gábor Hojtsy’s picture

I've just written some docs for this, so (1) is taken care of at http://drupal.org/node/224333#input_format with:

New #input_format to assign input format selection to fields. Changes 'body' field location in node, comment, block, etc.

(issue) In Drupal 6, you have been assigning input formats to textareas by wrapping the textarea into a parent element, and adding a filter format selector sibling element, such as:

  $form['comment_filter']['comment'] = array(
    '#type' => 'textarea',
    '#title' => t('Comment'),
    '#rows' => 15,
    '#default_value' => $default,
    '#required' => TRUE,
  );
  if (!isset($edit['format'])) {
    $edit['format'] = FILTER_FORMAT_DEFAULT;
  }
  $form['comment_filter']['format'] = filter_form($edit['format']);

There was no standard for naming, parenting the format selector, so it was impossible to identify textareas with input formats attached (compared to plain text input widgets). It was also common to use this construct with a tree structure, so you'd get ['comment_filter']['comment'] and ['comment_filter']['filter'] values or a non-tree structure so that you get ['comment'] and ['format'].

In Drupal 7, this is all strictly defined and automated with the introduction of the #input_format FAPI property. Just specify the input format used by default for the field and Drupal will automatically expand the element to a 'value' and a 'format' element later, so the above code becomes:

  $form['comment'] = array(
    '#type' => 'textarea',
    '#title' => t('Comment'),
    '#rows' => 15,
    '#default_value' => $default,
    '#input_format' => isset($edit['format']) ? $edit['format'] : FILTER_FORMAT_DEFAULT,
    '#required' => TRUE,
  );

This is way simpler, and Drupal 7 takes care of the rest. #input_format should have the value of the default input format to use with the selector. Through the form processing, Drupal transform this structure to have 'value' and 'format' children in form_process_input_format(). Although the structure results in ['comment']['value'] and ['comment']['format'] respectively, the $form_values array with the submitted form data contains the comment value in ['comment'] and the format in ['comment_format']. This lets you swap between formatted and non-formatted input easily, without rewriting your code to more complex structures. This new format also lets WYSIWYG editors to attach themselves to widgets with input formats they support.

Damien Tournoud’s picture

Although the structure results in ['comment']['value'] and ['comment']['format'] respectively, the $form_values array with the submitted form data contains the comment value in ['comment'] and the format in ['comment_format']. This lets you swap between formatted and non-formatted input easily, without rewriting your code to more complex structures. This new format also lets WYSIWYG editors to attach themselves to widgets with input formats they support.

I still believe this is totally unsubstantiated. The FAPI is now doing some belly dancing for no purpose whatsoever. Your code still has to manage the format, pass the text to check_markup(). In fact, in formatted mode, the text (the one we put a lot of effort to keep in the same place as before) really has *no meaning* without the format.

sun’s picture

Status: Needs work » Fixed
FileSize
8.52 KB

Committed attached patch to Form API reference. Also fixed "Forms API should be Form API" all over the place in the FAPI reference and quickstart guide.

SeanBannister’s picture

Sub

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

See #414424: Introduce Form API #type 'text_format' for a relevant follow-up issue.

schildi’s picture

I just checked your proposal to set the input filter on a textarea field (D6).
The fields value is set using a function to read the content of a (small) file:

  $form['A']['B']['C']['readme'] = array(
    '#type' => 'textarea',
    '#attributes' => array('readonly' => 'readonly'),
    '#rows' => '15',
    '#title' => t('Information taken from file "%DOC" distributed with the module', array('%DOC' => $docfile)),
    '#required' => FALSE,
    '#default_value' => _MODULE_get_doc($docfile),
    '#description' => '',
  );

then the filter is set as shown by your example:

$form['A']['B']['C']['format'] = filter_form(1);

everything is looking fine with the exception of text not beeing filtered.
Any idea?

jm.federico’s picture

@schildi

It won't filter by default, you have to do the filtering by yourself in your submit function

http://api.drupal.org/api/function/check_markup/6