357315 - Ability to create <textarea> fields

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

We already have plain-text <input> fields--it should be very easy to create <textarea> fields. In fact, I wrote the patch just now--it took only a few minutes.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. I haven't tested this extensively, but I did do some basic tests and it seems to work just fine.

Comment 2

[Frédéric Buclin]

Comment on attachment 242798 [details] [diff] [review]
v1

>Index: template/en/default/bug/field.html.tmpl

>+    [% CASE constants.FIELD_TYPE_TEXTAREA %]
>+      <textarea name="[% field.name FILTER html%]" 
>+                cols="60" rows="4">[% value FILTER html %]</textarea>
>   [% END %]


We really have to group fields by type. If someone has 3 text boxes, 2 dropdown menus, and 3 textareas, the UI will be very very ugly. But that's another bug.

Comment 3

[Olav Vitters]

It is not really a problem due to the sortkey on custom fields.

Comment 4

[Frédéric Buclin]

Yeah, right. But what I meant is that we may want to display them in different places in the page based on their type.

Comment 5

[Olav Vitters]

Comment on attachment 242798 [details] [diff] [review]
v1

>Index: template/en/default/bug/field.html.tmpl
>@@ -56,6 +56,9 @@
>                 [%- legal_value FILTER html %]</option>
>           [% END %]
>         </select>
>+    [% CASE constants.FIELD_TYPE_TEXTAREA %]
>+      <textarea name="[% field.name FILTER html%]" 
>+                cols="60" rows="4">[% value FILTER html %]</textarea>

Nit: you could use global/textarea.html.tmpl

review-: When editable is set to '0', you should use FILTER html_linebreak, not FILTER html.

Nit: When changing bugs, it doesn't show spaces at the beginning of lines or empty lines. However, bugmail might to this intentionally.

Comment 6

[Frédéric Buclin]

The trunk is now frozen -> TM: 3.2

Comment 7

[Max Kanat-Alexander]

Attachment: v2

Whoever gets to this first is fine with me.

This patch is sponsored by NASA, so reviews will also be sponsored by NASA.

Comment 8

[Frédéric Buclin]

Comment on attachment 280828 [details] [diff] [review]
v2

>Index: Bugzilla/Field.pm

>+    FIELD_TYPE_TEXTAREA,      { TYPE => 'MEDIUMTEXT' },

Nit: when you edit this field (which may contain e.g. a whole big comment or a stack trace), this huge field appears in the bugs activity table and in bugmail. I let you imagine how unreadable it becomes. Do you plan to do something to prevent this?



>Index: template/en/default/bug/field.html.tmpl

>+       [% INCLUDE global/textarea.html.tmpl
>+           id = field.name name = field.name minrows = 4 maxrows = 8
>+           cols = 65 defaultcontent = value %]

Set cols = 60 for consistency with free text fields and other free text fields in show_bug.


>-  [% value.join(', ') FILTER html %]
>+  [% value.join(', ') FILTER html_linebreak %]

html_linebreak doesn't filter anything. You can inject XSS now (and 008filter.t correctly complains about it). You must write FILTER html FILTER html_linebreak. Anyway, from what I understand, html_linebreak must only be used in element attributes, e.g. <foo bar="[% baz FILTER html_linebreak FILTER html %]", which is not the case here. So you should leave this filter alone.


>                    ${constants.FIELD_TYPE_FREETEXT}      => "Free Text",
>                    ${constants.FIELD_TYPE_SINGLE_SELECT} => "Drop Down",
>                    ${constants.FIELD_TYPE_MULTI_SELECT}  => "Multiple-Selection Box",
>+                   ${constants.FIELD_TYPE_TEXTAREA}      => "Large Text Box",

I note that the UI displays these fields in some random order. We should really fix that. Bug that's another bug.


Else your patch looks good.

Comment 9

[Frédéric Buclin]

Also, maybe should we enclose the field in <pre></pre> when you cannot edit the field so that newlines and indentation are respected.

Comment 10

[Alex Eiser]

I would vote against using PRE, since it essentially overrides the standard CSS on the page.. rendering your text in a fixed width font.

Why would html_linebreak only be used in attribute definitions? Also [% baz FILTER html_line_break FILTER html %] since the <br/> that would be injected would be escaped so that they would be printed to the screen incorrectlyas &lt;br /&gt; &lt;br /&gt;.

I see no reason why you wouldn't want to FILTER html then insert BR's instead of \n when you are not in a text area.

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> I would vote against using PRE, since it essentially overrides the standard CSS
> on the page.. rendering your text in a fixed width font.

That's how bug comments are displayed already. And you can still override PRE in CSS files.


> Why would html_linebreak only be used in attribute definitions? Also [% baz
> FILTER html_line_break FILTER html %] since the <br/> that would be injected
> would be escaped so that they would be printed to the screen incorrectlyas
> &lt;br /&gt; &lt;br /&gt;.

Huh? Did you read the definition of html_linebreak at http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/Bugzilla/Template.pm#488? No <br> is injected and no newline appears in the HTML page.

Comment 12

[Max Kanat-Alexander]

We can use CSS with font-style: monospace and white-space: pre.

Comment 13

[Alex Eiser]

Ahh, that would explain why you used html_linebreak instead of html_line_break
which is provided by the template toolkit library its self.

Based on the definition html_linebreak is a terriable name, and as you
indicated likely not useful in this situation.

Comment 14

[Max Kanat-Alexander]

Attachment: v3

This version uses a div with white-space: pre and font-family: monospace.

I also made wrap_comment accept an argument, and fixed a tiny bug in wrap_comment.

Comment 15

[Max Kanat-Alexander]

Oh, and I forgot to change "65" to "60", but I can do that on checkin.

Comment 16

[Frédéric Buclin]

Comment on attachment 280938 [details] [diff] [review]
v3

>Index: skins/standard/global.css

>+/* For bug fields */
>+.uneditable_textarea {

Nit: this class should have a more generic name as it could be reused in some other places too.



>Index: template/en/default/bug/field.html.tmpl

>+       [% INCLUDE global/textarea.html.tmpl
>+           id = field.name name = field.name minrows = 4 maxrows = 8
>+           cols = 65 defaultcontent = value %]
>   [% END %]
>+[% ELSIF field.type == constants.FIELD_TYPE_TEXTAREA %]
>+  <div class="uneditable_textarea">[% value FILTER wrap_comment(65) 
>+                                            FILTER html %]</div>

Do not forget to s/65/60/ in both places on checkin.

r=LpSolit

Comment 17

[Max Kanat-Alexander]

I didn't change the class name. If anybody wants to re-use the same style on something else unrelated, they're free to just add another class name to the same format description. (Like .other_class, .uneditable_textarea { }).

Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.78; previous revision: 1.77
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.79; previous revision: 1.78
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.60; previous revision: 1.59
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.40; previous revision: 1.39
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.24; previous revision: 1.23
done