Open Bug 432132 Opened 14 years ago Updated 10 years ago

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

Categories

(SeaMonkey :: Composer, defect)

defect
Not set
normal

Tracking

(Not tracked)

People

(Reporter: stefanh, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Spin-off from bug 431061. It appears that there are lots of textboxes in composer that lacks the type attribute.
Attached patch Fix textboxes in editor/ui (obsolete) — Splinter Review
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.
Attachment #320953 - Flags: superreview?(neil)
Attachment #320953 - Flags: review?(neil)
Status: NEW → ASSIGNED
Target Milestone: --- → seamonkey2.0alpha
(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 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.
Attachment #320953 - Flags: superreview?(neil)
Attachment #320953 - Flags: superreview-
Attachment #320953 - Flags: review?(neil)
Hmm, and I guess it would be a bit odd if we mixed both types 
Status: ASSIGNED → NEW
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).
As IanN suggests in comment #5...
Attachment #320953 - Attachment is obsolete: true
Attachment #321090 - Flags: superreview?(neil)
Attachment #321090 - Flags: review?(iann_bugzilla)
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)
(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 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?
Attachment #321090 - Flags: superreview?(neil) → superreview+
(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.
maxlength avoids the flickering you get trying to type in too many digits.
Oh, ok - I add that, then.

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).
Attachment #321090 - Flags: review?(iann_bugzilla) → review?(neil)
(by ages I mean more than 1 week)
Attachment #321090 - Flags: review?(neil) → review+
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.
Attachment #321090 - Attachment description: Just fix editor/ui/composer/content/pref-composer.xul → Just fix editor/ui/composer/content/pref-composer.xul (checked in)
Sigh, I forgot to address the comments
(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
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.
Target Milestone: seamonkey2.0a1 → ---
Status: NEW → ASSIGNED
Assignee: stefanh → nobody
Status: ASSIGNED → NEW
Is it still actual?
You need to log in before you can comment on or make changes to this bug.