516217 - Anything can be entered in the "font-size" field, even non numeric

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, defect)

Description

[worms]

Attachment: Confused checked items in the Font-size menu

Careless users could decide to enter anything (alphanumeric, URL...) in the font-size (other...) field, and this confuses ChatZilla - but doesn't prevent it to work.

But the "checked" items in the Menu become a bit inconsistent, see screenshot attached

Comment 1

[worms]

I meant "doesn't prevent it from working", of course.

Sugnim++

Comment 2

[Sean Martinez]

Taking this bug on!

Comment 3

[Sean Martinez]

Attachment: Patch for review of logic change plus prompt change.

Group that worked on this:  Sean Martinez, Justin Ross, Claire Buehler, JJ Garcia.

File change - chatzilla.properties
A change to the dialog box to explain that non-numeric inputs will result in a switch to default font-size.

File change - commands.js
Logic change so that non-numeric inputs will result in switch to default font-size.

Comment 4

[:Gijs (he/him)]

Hi Sean,

Thanks for working on this! How did you generate the patch? Normally speaking, assuming you used the copy of chatzilla you got through "hg", you should be able to use either mercurial queues or use "hg diff" or "hg export" to get a readable version of the diff. The attachment ( https://bugzilla.mozilla.org/attachment.cgi?id=8561896 ) isn't readable, unfortunately... not sure what it is?

Comment 5

[Sean Martinez]

(In reply to :Gijs Kruitbosch from comment #4)
> Hi Sean,
> 
> Thanks for working on this! How did you generate the patch? Normally
> speaking, assuming you used the copy of chatzilla you got through "hg", you
> should be able to use either mercurial queues or use "hg diff" or "hg
> export" to get a readable version of the diff. The attachment (
> https://bugzilla.mozilla.org/attachment.cgi?id=8561896 ) isn't readable,
> unfortunately... not sure what it is?

Hhmm, I might have misunderstood what I was supposed to attach.  I zipped only the two files that we adjusted and sent that as the attachment along with the explanation.  What is proper protocol?  The attachment section only allowed one file to add.

Comment 6

[:Gijs (he/him)]

(In reply to Sean Martinez from comment #5)
> What is proper protocol?  The attachment section only
> allowed one file to add.

Right. If you are at the root of the source tree in a terminal, and assuming you have modifications but did not commit them, you can do:

$ hg commit -m "Bug 516217 - fix numeric fields in the preferences to actually be numeric-only, r?gijs"
$ hg export -r-1

which will dump a "diff" ( http://en.wikipedia.org/wiki/Diff_utility ) to your terminal. You can pipe this to a file, using something like:

$ hg export -r-1 > 516217.patch

and attach the file here.

"hg commit" will create a changeset in your local copy of the source tree; if you want to change that commit to include more changes or change the commit message, you can use "hg commit --amend". -m indicates that the commit message for the changeset should be the string passed in quotes (you can also use -e instead of -m "..." to get your default editor).

"hg export" will export a changeset as a diff, and the -r switch indicates which changeset ("revision", hence -r) to export to a diff. "-1" is a hacky indication of "the topmost changeset", which saves you looking for the ID of that changeset. :-)

I hope that helps, if not, please feel free to needinfo me! :-)

Comment 7

[Sean Martinez]

Attachment: fix numeric fields in the preferences to actually be numeric-only

Second attempt at sending patch, as response to Gijs' comment.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8563003 [details] [diff] [review]
fix numeric fields in the preferences to actually be numeric-only

Review of attachment 8563003 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! The patch file is definitely of the form I'd expect now - I'll look at the contents tomorrow. :-)

Comment 9

[Svetlana Tkachenko]

There are some valid non-numeric font sizes in CSS. Should this text box support them?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8563003 [details] [diff] [review]
fix numeric fields in the preferences to actually be numeric-only

Review of attachment 8563003 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start! I'd forgotten about this dialog thing in the email I sent to you about this bug... so the bits in here are important.

However... I still think we should update the preference panes as well (see chatzilla > Preferences > global settings > appearance tab):

http://hg.mozilla.org/chatzilla/file/2f6817b26141/xul/content/config.js#l569

and specifically, add a line like:

this.edit.setAttribute("type", "number");

which I *think* should mean that those pref inputs will only accept numbers as input (it'll still let you set non-integer things, like 14.5 or whatever, but I *think* most of the rest of the code will also cope with that, so that should be OK for now).

Does that work, and/or if not, can you let me know what breaks? Note in particular that this will update a lot of other number-pref inputs as well (e.g. the maximum number of messages per view, and so on). It's possible I've forgotten about some pref or other that somehow pretends to be a number and then stops working, but in principle I think it ought to work...

::: locales/en-US/chrome/chatzilla.properties
@@ +1302,4 @@
>  msg.fonts.family.pick  = Enter the font family you wish to use:
>  msg.fonts.size.fmt     = Font size is %Spt
>  msg.fonts.size.default = Font size is default
> +msg.fonts.size.pick    = Enter the font size you wish to use.  Note: Non-numeric inputs will result in the default size.

Can you also update pref.font.size.help ? This is the tooltip that appears for the input in the preference panes.

::: xul/content/commands.js
@@ +3980,5 @@
>  
>                  default:
> +					if(isNaN(val)) {
> +						val = 0;
> +					}

Right, I'd not thought about this, so it's a good job you found it and fixed it. Minor style nits here:

- please don't use tabs. Use spaces instead.
- single-line if conditions don't need braces in the chatzilla style (and if code does need braces, the braces go on their own line).
- space after 'if' before the opening '(' of the condition, please.
- indent 4 spaces for bits inside if/else conditions

So, apart from the extra leading spacing to indent this the same way as the other case: blocks in the switch statement, this can be:

if (isNaN(val))
    val = 0;
else
    val = Number(val);

Comment 11

[:Gijs (he/him)]

Comment on attachment 8563003 [details] [diff] [review]
fix numeric fields in the preferences to actually be numeric-only

Whoops, I meant to set feedback+ here (which basically means: the approach is correct, but I'd like to look at the patch again before we land it)

Comment 12

[:Gijs (he/him)]

(In reply to Gryllida from comment #9)
> There are some valid non-numeric font sizes in CSS. Should this text box
> support them?

No, the pref only deals with pt-based font sizes, and overhauling it completely isn't something we should be trying to do here, IMO.

Comment 13

[James Ross]

In case it's not been linked recently, http://www.hacksrus.com/~ginda/pedant.html is the ChatZilla Coding Guidelines we're still trying to follow. :)

Comment 14

[Sean Martinez]

Attachment: bug516217patchvers2.patch

Revised coding standards, adjusted mouse-over text to specify integer values for preference input and added number requirement to preference number dialog boxes as mentioned.

Comment 15

[:Gijs (he/him)]

Attachment: fix numeric fields in the preferences to actually be numeric-only, and update font-size dialog to restrict input,

So I've pulled together your two patches and corrected some minor indenting and formatting things. However, a small test reveals that just using input type='number' breaks putting in '-1' in the fields (which works for some things). I didn't realize that using a numeric input actually uses a different XBL binding and so it does some weird stuff... Hmm. I think an adequate solution for now is to just add another line after the setAttribute one you added in config.js, and set the min attribute to -1. If you base it on this patch (and use spaces instead of tabs) then we should be good to go after that\!

Comment 16

[Sean Martinez]

Attachment: bug516217patchvers3.patch

Patch adjusted spacing again and added minimum requirement of -1 to "number" text boxes in the preferences tabs.

Comment 17

[:Gijs (he/him)]

Sean or Claire, can you list the names of your group members who worked on this bug so that you can all get credit in the commit log? :-)

Comment 18

[:Gijs (he/him)]

Attachment: Patch for checkin (modulo changes in authors)

Comment 19

[:Gijs (he/him)]

Ugh, bzexport, stealing bugs that I didn't mean to.

Comment 20

[Sean Martinez]

(In reply to :Gijs Kruitbosch from comment #17)
> Sean or Claire, can you list the names of your group members who worked on
> this bug so that you can all get credit in the commit log? :-)

Absolutely!  Our group is:

Sean Martinez
Claire Buehler
JJ Garcia
Justin Ross

Comment 21

[:Gijs (he/him)]

Thanks!

http://hg.mozilla.org/chatzilla/rev/c1c2c1cd169e