Ability to create <textarea> fields

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

2.23.3
Bugzilla 3.2
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [roadmap: 3.2])

Attachments

(1 attachment, 2 obsolete attachments)

v3
5.92 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 242798 [details] [diff] [review]
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.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #242798 - Flags: review?(bugzilla-mozilla)

Comment 2

12 years ago
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

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

Comment 4

12 years ago
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

12 years ago
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.
Attachment #242798 - Flags: review?(bugzilla-mozilla) → review-

Comment 6

12 years ago
The trunk is now frozen -> TM: 3.2
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
(Assignee)

Updated

11 years ago
Whiteboard: [roadmap: 3.2]
(Assignee)

Comment 7

11 years ago
Created attachment 280828 [details] [diff] [review]
v2

Whoever gets to this first is fine with me.

This patch is sponsored by NASA, so reviews will also be sponsored by NASA.
Attachment #242798 - Attachment is obsolete: true
Attachment #280828 - Flags: review?(bugzilla-mozilla)
Attachment #280828 - Flags: review?(LpSolit)

Comment 8

11 years ago
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.
Attachment #280828 - Flags: review?(bugzilla-mozilla)
Attachment #280828 - Flags: review?(LpSolit)
Attachment #280828 - Flags: review-

Comment 9

11 years ago
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

11 years ago
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

11 years ago
(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.
(Assignee)

Comment 12

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

Comment 13

11 years ago
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.
(Assignee)

Comment 14

11 years ago
Created attachment 280938 [details] [diff] [review]
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.
Attachment #280828 - Attachment is obsolete: true
Attachment #280938 - Flags: review?(LpSolit)
(Assignee)

Comment 15

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

Comment 16

11 years ago
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
Attachment #280938 - Flags: review?(LpSolit) → review+

Updated

11 years ago
Flags: approval+
(Assignee)

Comment 17

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 99240

Updated

10 years ago
Blocks: 434008

Updated

10 years ago
No longer blocks: 434008
You need to log in before you can comment on or make changes to this bug.