432132 - Use <textbox type="number"> in Composer where applicable

Status: NEW

(SeaMonkey :: Composer, defect)

Description

[Stefan [:stefanh]]

Spin-off from bug 431061. It appears that there are lots of textboxes in composer that lacks the type attribute.

Comment 1

[Stefan [:stefanh]]

Attachment: Fix textboxes in editor/ui

Interestingly, if you type "99999999999999999999" in a number textbox the value gets rounded to "100000000000000000000"...

This patch:
-Should fix all textboxes in editor/ui
-Removes some redundant js
-Removes some css because I decided to use size="3" on all textboxes that had class="narrow"

There seem to be a lot of calls to ValidateNumber in the js, but it doesn't seem to be used/working. At one stage, I think I noticed the dialog popping up, even after I removed the call to ValidateNumber in the main prefs. But now it doesn't seem to show up anymore anywere.

I'm not sure I like the spinbuttons when you have a menulist beside them, but I guess that's the way it should be done.

Comment 2

[Stefan [:stefanh]]

(In reply to comment #1)
> Created an attachment (id=320953) [details]
> Fix textboxes in editor/ui
> 
> Interestingly, if you type "99999999999999999999" in a number textbox the value
> gets rounded to "100000000000000000000"...
> 
> This patch:
> -Should fix all textboxes in editor/ui

and editor/ui/composer/content/pref-composer.xul, of course

Comment 3

[[e-mail]]

Comment on attachment 320953 [details] [diff] [review]
Fix textboxes in editor/ui

Well, type="number" seems to have a few issues in some of the dialogs. Some I noticed are:
1. Can't have an empty value (this is permitted in some fields).
2. New table width defaults to illegal 0% value.
3. Constraining scaled image dimensions doesn't work.

Comment 4

[Stefan [:stefanh]]

Hmm, and I guess it would be a bit odd if we mixed both types 

Comment 5

[Ian Neal]

I would tackle this in several patches - a quick win would be doing editor/ui/composer/content/pref-composer.xul and also removing the then unused LimitStringLength function (including any comments about it).

Comment 6

[Stefan [:stefanh]]

Attachment: Just fix editor/ui/composer/content/pref-composer.xul (checked in)

As IanN suggests in comment #5...

Comment 7

[Ian Neal]

Comment on attachment 321090 [details] [diff] [review]
Just fix editor/ui/composer/content/pref-composer.xul (checked in)

On code inspection, will test later and comment.

>Index: editor/ui/dialogs/content/EdAEHTMLAttributes.js
>===================================================================
>@@ -298,18 +298,18 @@ function onInputHTMLAttributeValue()
>     return;
> 
>   // Trim spaces only from left since we must allow spaces within the string
>   //  (we always reset the input field's value below)
>   var value = TrimStringLeft(gDialog.AddHTMLAttributeValueInput.value);
>   if (value)
>   {
>     // Do value filtering based on type of attribute
>-    // (Do not use "LimitStringLength()" and "forceInteger()"
>-    //  to avoid multiple reseting of input's value and flickering)
>+    // (Do not use "forceInteger()" to avoid multiple
Nit: You've lost a word here "reseting" or even better spelt correctly "resetting"
>+    //  input's value and flickering)

Comment 8

[Stefan [:stefanh]]

(In reply to comment #7)
> (From update of attachment 321090 [details] [diff] [review])
> On code inspection, will test later and comment.
> 
> >Index: editor/ui/dialogs/content/EdAEHTMLAttributes.js
> >===================================================================
> >@@ -298,18 +298,18 @@ function onInputHTMLAttributeValue()
> >     return;
> > 
> >   // Trim spaces only from left since we must allow spaces within the string
> >   //  (we always reset the input field's value below)
> >   var value = TrimStringLeft(gDialog.AddHTMLAttributeValueInput.value);
> >   if (value)
> >   {
> >     // Do value filtering based on type of attribute
> >-    // (Do not use "LimitStringLength()" and "forceInteger()"
> >-    //  to avoid multiple reseting of input's value and flickering)
> >+    // (Do not use "forceInteger()" to avoid multiple
> Nit: You've lost a word here "reseting" or even better spelt correctly
> "resetting"
> >+    //  input's value and flickering)
> 

Ugh. Actually, double that (missing "of" as well). I'll add "resetting of" to the last line when I (eventually) land this.

Comment 9

[[e-mail]]

Comment on attachment 321090 [details] [diff] [review]
Just fix editor/ui/composer/content/pref-composer.xul (checked in)

>-                 oninput=" ValidateNumber(this, null, 0, 99); LimitStringLength('recentFiles',2);"/>
Add maxlength="2" perhaps?

Comment 10

[Stefan [:stefanh]]

(In reply to comment #9)
> (From update of attachment 321090 [details] [diff] [review])
> >-                 oninput=" ValidateNumber(this, null, 0, 99); LimitStringLength('recentFiles',2);"/>
> Add maxlength="2" perhaps?
> 

+                 type="number"
+                 max="99"
+                 min="0"

'max="99"' should be enough.

Comment 11

[[e-mail]]

maxlength avoids the flickering you get trying to type in too many digits.

Comment 12

[Stefan [:stefanh]]

Oh, ok - I add that, then.

Comment 13

[Stefan [:stefanh]]

Comment on attachment 321090 [details] [diff] [review]
Just fix editor/ui/composer/content/pref-composer.xul (checked in)

I haven't seen IanN for ages, so bumping the r? to Neil (I'll fix comment #8 and comment #9 on check-in if the patch gets approved).

Comment 14

[Stefan [:stefanh]]

(by ages I mean more than 1 week)

Comment 15

[Stefan [:stefanh]]

Comment on attachment 321090 [details] [diff] [review]
Just fix editor/ui/composer/content/pref-composer.xul (checked in)

Landed this:

Checking in editor/ui/composer/content/pref-composer.xul;
/cvsroot/mozilla/editor/ui/composer/content/pref-composer.xul,v  <--  pref-composer.xul
new revision: 1.37; previous revision: 1.36
done
Checking in editor/ui/dialogs/content/EdAEHTMLAttributes.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdAEHTMLAttributes.js,v  <--  EdAEHTMLAttributes.js
new revision: 1.30; previous revision: 1.29
done
Checking in editor/ui/dialogs/content/EdDialogCommon.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdDialogCommon.js,v  <--  EdDialogCommon.js
new revision: 1.152; previous revision: 1.151
done

Leaving bug open until we settled the rest.

Comment 16

[Stefan [:stefanh]]

Sigh, I forgot to address the comments

Comment 17

[Stefan [:stefanh]]

(In reply to comment #16)
> Sigh, I forgot to address the comments
> 

Fixed comments:
Checking in composer/content/pref-composer.xul;
/cvsroot/mozilla/editor/ui/composer/content/pref-composer.xul,v  <--  pref-composer.xul
new revision: 1.38; previous revision: 1.37
done
Checking in dialogs/content/EdAEHTMLAttributes.js;
/cvsroot/mozilla/editor/ui/dialogs/content/EdAEHTMLAttributes.js,v  <--  EdAEHTMLAttributes.js
new revision: 1.31; previous revision: 1.30
done

Comment 18

[Robert Kaiser]

This bug is open but targeted for seamonkey2.0a1, which has been released a long time ago. Please set the target milestone to an appropriate value, "---" if it has no specific target.

Comment 19

[Phoenix]

Is it still actual?