291459 - Make textareas zoom large when in use

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Marc Schumann [:Wurblzap]]

Using JavaScript onFocus and onBlur, textareas can be enlarged when in use and
shrunk back when not in use.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

I happen to like it :)

Comment 2

[Vlad Dascalu]

Comment on attachment 181528 [details] [diff] [review]
Patch

In the [%# INTERFACE section, I'd specify (required) in the same lines as the
(optional) tag, in order to be complete. As far as I can see, it needs to be
added for minrows and cols.

According to http://www.w3.org/TR/html4/interact/forms.html#h-17.7 , the name
tag is implied, so it's correctly marked as optional.

Asking myk for review as he's the UI owner. This might go in a param since not
everybody might enjoy this "flickering" :), but I don't know how myk feels
about it and that's what it matters after all.

Comment 3

[Vlad Dascalu]

Another issue with this patch is that we're changing textareas from 5 rows to 3
/ 10 rows, depending on focus/blur. However, for users that have Javascript
disabled, this patch will only "shrink" their textareas from 5 rows to 3 rows,
permanently. So it's a UI "reduction" for them (regression would have sounded
too strong)

Comment 4

[Marc Schumann [:Wurblzap]]

In order to help deciding whether it's ok or not -- the patch reduces the
minrows in two places:

o the disabletext textarea in editusers.cgi from 10 to 2 rows
o the whine message descriptive text textarea in editwhines.cgi from 5 to 3 rows

Comment 5

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Making it a user setting.

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 185762 [details] [diff] [review]
Patch 1.2

Argh, wrong file.

Comment 7

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Comment 8

[Vlad Dascalu]

Comment on attachment 185764 [details] [diff] [review]
Patch 1.2

Reason for review-:

-	 <textarea wrap="soft" id="commment" name="comment" rows="6"
cols="80"></textarea>
+	 [% PROCESS global/textarea.html.tmpl
+	   name      = 'comment'
+	   id	     = 'comment'
+	   minrows   = 6
+	   maxrows   = 15
+	   cols      = 80
+	 %]

This removes the wrap="soft" attribute in the comments textarea. However,
you're leaving this intact in template/en/default/attachment/edit.html.tmpl.

I assume we should either remove "soft" from all the places, or leave it in all
the places.


The rest is all purely optional nits/questions.

Nit 1:

Some people might argue that the "wrap=soft" removal should be a separate bug.
The patch might consider having "wrap" as an additional parameter (I know, it's
not an official HTML parameter, but either we remove it first in another bug,
or keep it up with us).

Nit 2:

The "style" thing is used in attachment/edit.html.tmpl, so we might want to add
"style" param as well to the textarea.

Question 3:

Why did you leave that textarea unchanged? (in attachment/edit.html.tmpl)

Nit 4:

Shouldn't we have defaultrows, identical to the old values, when the option is
off? For example in:

-      <textarea name="event_[% event.key %]_body"
-		 rows="5" cols="80">
-	   [% event.value.body FILTER html %]</textarea>
+      [% PROCESS global/textarea.html.tmpl
+	  name		 = "event_${event.key}_body"
+	  minrows	 = 3
+	  maxrows	 = 10
+	  cols		 = 80
+	  defaultcontent = event.value.body
+	%]

defaultrows could be 5, leaving the behaviour unmodified for those with the
option off. I reckon that this might be overkill, so just mentioning it.

Comment 9

[Vlad Dascalu]

Overkill brainstorming idea: assume defaultrows is minrows when it's missing?
That way it needs to be specified explicitely only when it's different than
minrows (those 2 cases in comment #4)?

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

Addressed comments:

- Introduced style and wrap parameters
- Modified all <textarea> occurrences to [% INCLUDE textarea %]
- Implemented the defaultrows idea of comment 9

Comment 11

[Vlad Dascalu]

Comment on attachment 190386 [details] [diff] [review]
Patch 1.3

Sorry for the delay.

This looks to good to be true, darn, it was so close to commit and I want this
feature too :).

In some XXX bug (I can't remember its number), justdave decided that XSS is
acceptable if it's done by admin. This was the price to pay in order to enable
the admins to customize product descriptions, and to allow in them HTML-like
formatting code.

For example, you can enter in a description <big><big>Hello!</big></big>, and
Hello! will appear with increased font size in the product description.

That's why filterexceptions.pl has:

'admin/classifications/edit.html.tmpl' => [
  'description', 
],

Your patch takes that away:

--- head/template/en/default/admin/classifications/edit.html.tmpl      
2004-12-27 19:56:54.000000000 +0100
+++ patched/template/en/default/admin/classifications/edit.html.tmpl   
2005-07-24 23:36:13.547012800 +0200
@@ -31,7 +31,14 @@
     </tr>
     <tr>
       <th align="right">Description:</th>
-      <td><textarea rows=4 cols=64 name="description">[% description
%]</textarea></TD>
+      <td>
+	 [% INCLUDE global/textarea.html.tmpl
+	   name 	  = 'description'
+	   minrows	  = 4
+	   cols 	  = 64
+	   defaultcontent = description
+	 %]
+      </td>

which is wrong for several reasons:

-> you're leaving the filterexceptions.pl untouched despite removing the only
place with unfiltered description in it.
-> You are FILTERing HTML that description, which means <big> will appear
&lt;big&gt; in the edit box, and cancel the HTML formatting, XSS-hole effect.
-> you are inconsistent in replacing textareas with filtered HTML content and
textareas with unfiltered HTML content into only one type.

My best bet on the easiest way to solve this would be another
textarea.html.tmpl parameter indicating the type of filtering to be applied
(but since textarea is HTML, that might be a boolean value, unless we're adding
XHTML as a 3rd option anytime soon).



Optional nit: It bothers me that some textareas have both name and id fields,
and others only name fields. I think one of them is an IE-proprietary tag; darn
those validation issues. Oh well, like mkanat's favorite quote is saying,
that's another bug. :)

Comment 12

[Vlad Dascalu]

Actually, the text contained within a <textarea> should always be FILTER html,
since HTML tags are not allowed inside <textarea> containers.

(practically <textarea>&lt;</textarea> will appear as "<" in the editbox, so we
should always filter)

Not filtering there was probably a mistake in the first place.

Anyway, the review- reason that stands is that you should remove as well from
filterexceptions.pl unfiltered variables where you're removing the last
occurance. Like the example I gave.

And it seems we always need to filter HTML, so I guess we can ditch away that
filter parameter.

Comment 13

[Marc Schumann [:Wurblzap]]

Vlad, the patch does remove surplus filterexceptions.pl entries. (There is only
one.) Unless I'm mistaken or there is something else wrong with the patch, it
seems to me it qualifies for r+ as it stands. Can you please take another look?

Comment 14

[Marc Schumann [:Wurblzap]]

Comment on attachment 190386 [details] [diff] [review]
Patch 1.3

Requesting review again per comment 13.

Comment 15

[Marc Schumann [:Wurblzap]]

Comment on attachment 190386 [details] [diff] [review]
Patch 1.3

Vlad, if you're reviewing this patch at the moment, please leave a comment --
you seem to be busy, so I'm opening the r? to the wind for now.

Comment 16

[Teemu Mannermaa (:wicked)]

Comment on attachment 190386 [details] [diff] [review]
Patch 1.3

Bitrotten, what a pity. :(

Comment 17

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.4

Unrotted.

Comment 18

[Vlad Dascalu]

Comment on attachment 201416 [details] [diff] [review]
Patch 1.4

Well, as long as filterexceptions is taken care of...

It might be bitrotten a little, but fixable upon checkin.

We might want this off by default, although on seems good to me.

Comment 19

[Frédéric Buclin]

Marc, your latest patch doesn't remove an entry from filterexceptions.pl. Is that expected? This removal was in your previous patch though.

Comment 20

[Frédéric Buclin]

Attachment: unbitrotten patch (the one I checked in)

For the record, here is the patch I checked in. attachment/edit/html.tmpl was bitrotten and 'attachid' is now 'attachment.id', among others.

Comment 21

[Frédéric Buclin]

Marc, I didn't remove the entry from filterexceptions.pl as you removed it from your latest patch. If it should be removed anyway, I think you can do it without opening a new bug.


Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.472; previous revision: 1.471
done
Checking in template/en/default/admin/classifications/add.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/add.html.tmpl,v  <--  add.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/classifications/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/classifications/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/admin/components/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/components/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.15; previous revision: 1.14
done
Checking in template/en/default/admin/keywords/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/keywords/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/users/userdata.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/userdata.html.tmpl,v  <--  userdata.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.23; previous revision: 1.22
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.32; previous revision: 1.31
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.71; previous revision: 1.70
done
Checking in template/en/default/bug/create/create-guided.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create-guided.html.tmpl,v  <--  create-guided.html.tmpl
new revision: 1.27; previous revision: 1.26
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.55; previous revision: 1.54
done
Checking in template/en/default/global/setting-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/setting-descs.none.tmpl,v  <--  setting-descs.none.tmpl
new revision: 1.7; previous revision: 1.6
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/textarea.html.tmpl,v
done
Checking in template/en/default/global/textarea.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/textarea.html.tmpl,v  <--  textarea.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v  <--  edit-multiple.html.tmpl
new revision: 1.32; previous revision: 1.31
done
Checking in template/en/default/pages/linkify.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/linkify.html.tmpl,v  <--  linkify.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/whine/schedule.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/whine/schedule.html.tmpl,v  <--  schedule.html.tmpl
new revision: 1.4; previous revision: 1.3
done

Comment 22

[Frédéric Buclin]

The textarea for the comment when editing an attachment is very small and doesn't expand. Is that expected? Or is it a bug?

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
> The textarea for the comment when editing an attachment is very small and
> doesn't expand. Is that expected? Or is it a bug?
> 

Hum... maybe not smaller than usual, but I expected this feature to be used here too.

Comment 24

[Vlad Dascalu]

--- head/template/en/default/admin/classifications/edit.html.tmpl       2005-10-17 23:58:51.000000000 +0200
+++ patched/template/en/default/admin/classifications/edit.html.tmpl    2005-10-20 08:20:03.458881600 +0200

-      <td><textarea rows=4 cols=64 name="description">
-            [% classification.description FILTER none %]</textarea>
+      <td>
+        [% INCLUDE global/textarea.html.tmpl
+          name           = 'description'
+          minrows        = 4
+          cols           = 64
+          defaultcontent = classification.description
+        %]


So now, instead of FILTER none, we apply FILTER html. I don't know enough about the decisions that David (justdave) made regarding admin-filled text and its FILTER-ing, so CC-ing him post-checkin. See comment 11 where I suggested a textarea.html.tmpl param in order to specify the type of filtering.

Also, there still is an unrequired filterexception in filterexception.pl, mentioned by LpSolit.

Comment 25

[Dave Miller [:justdave]]

(In reply to comment #24)
> So now, instead of FILTER none, we apply FILTER html. I don't know enough
> about the decisions that David (justdave) made regarding admin-filled text
> and its FILTER-ing, so CC-ing him post-checkin.

That's okay, this is the text entered in the textarea, not displaying it outside of one.  That was an dataloss problem waiting to happen with the filter set to none on that.  The admin can edit the HTML, but we need to show him the source of the HTML to edit, not have it be interpreted.

However, I didn't realize you guys were doing this EVERYWHERE.  I thought you were just doing it on the show_bug and enter_bug forms. My fault for not reading the bug closely before approving.  I really don't think we need this everywhere.  It's way overkill.  Many of the textareas (especially the ones on most of the admin pages) aren't intended to have lots of text in them anyway.  Think product and component descriptions here.  Those fields should be short and concise as much as possible.  Expanding the textarea artificially makes the admin think they can write more. :)

I'd really appreciate some of these getting backed out, with some thought to which ones really need it and which ones don't.  But that's another bug at this point, not worth wasting time on it on this one.

Comment 26

[Dave Miller [:justdave]]

You know what I'd rather see instead of this, is a little drag widget on the bottom right-hand corner of text areas, and let the user resize them themselves if they need them bigger.  Drupal does it, so I know it's possible. :)

Comment 27

[Marc Schumann [:Wurblzap]]

Vlad, I didn't re-remove filtering per your comment 12.

And Dave, a plus side of this patch is not only that textareas may become bigger, but that they start out smaller, too. My pet peeve was editusers.cgi's disabledtext textarea, which has become less page-dominating now.

We can tweak sizes easily now. And it's a user pref.

Comment 28

[Vlad Dascalu]

Marc: we're passing that to the textarea.html.tmpl template unfiltered.

If something can be removed from filterexceptions.pl, and the testing suite still runs ok (especially the filter test), then it should be removed.

Comment 29

[Vlad Dascalu]

> I really don't think we need this everywhere.

Well, even if we use the template everywhere, this is actually enabled only when we have a specific max_rows param, different from the min_rows one. Which is far fewer places than it might seems.

And having an unique textarea.html.tmpl allows now to implement your Javascript resize thingy.

Comment 30

[Vlad Dascalu]

(In reply to comment #28)
> If something can be removed from filterexceptions.pl, and the testing suite
> still runs ok (especially the filter test), then it should be removed.

To be more explicit, I think we can remove at this point classifications/edit.html.tmpl from filterexceptions. It was not removed by the latest patch (v1.4), althought previous patches might have removed that, based on LpSolit's comment.

Comment 31

[Frédéric Buclin]

This story is between vladd, marc and dave.

Comment 32

[Vlad Dascalu]

Well this bug is resolved so I filled for discussion/patches bug 329082, bug 329083, bug 329084.

Comment 33

[Dave Miller [:justdave]]

*** Bug 352550 has been marked as a duplicate of this bug. ***

Comment 34

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.

Comment 35

[tejguddigerekempaiah]

The zoom functionality is not working in firefox, Version: 64.0
I would like to re-open this defect which is in closed / fixed state.

Comment 36

[Kohei Yoshino [:kohei]]

Please file a new bug. Note that this is a bug in Bugzilla (this website), not Firefox.