73049 - ValidateNumberString should show error message if user input string is empty

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Charles Manske]

We use the JS method ValidateNumberString(value, minValue, maxValue)
 to detect when a user enters a number outside of allowable range, but if the
input string ("value") is empty, we don't show the error dialog.
We should simply show the error dialog with a similar message as we do now.

Comment 1

[Charles Manske]

Attachment: Extend ValidateNumberString to allow forcing a value string

Comment 2

[Charles Manske]

A bit more involved than I hoped.
I turns out that the only case where we really want to force the input to
have a value is for "row" and "columns" in table dialogs. This fix is general
in case there's some other users in the future.

Comment 3

[Charles Manske]

Attachment: Updated patch - property strings were stale in last one

Comment 4

[Kathleen :Brade]

Why does EdTableProps.js have a function "ValidateNumber" but no other property 
dialogs do?  Other dialogs (as well as EdTableProps.js) seem to call 
ValidateNumberString in EdDialogCommon.js.

Why haven't you propagated the additional parameter in ValidateNumberString to 
all callers?  Have you checked every usage of ValidateNumberString to be sure 
that we aren't requiring any of those values? 

Comment 5

[Charles Manske]

>Why does EdTableProps.js have a function "ValidateNumber"...?
Simple factoring. No other dialog needed the complicated code that table
attributes require because of "%" vs. pixels issues, etc.
> Why haven't you propagated the additional parameter in ValidateNumberString?
Default (missing = "null) parameters are OK in JS.
>Have you checked every usage...
Yes.
It turned out only the "row" and "column" that needed the new param to be
"true".

Comment 6

[Kathleen :Brade]

>>Why does EdTableProps.js have a function "ValidateNumber"...?
>Simple factoring. No other dialog needed the complicated code that table
>attributes require because of "%" vs. pixels issues, etc.

Other dialogs do have % vs pixels; why is this different?  Why is ValidateNumber 
function in table js rather than the common/shared dialog JS file?

I'm aware of the defaults for JS, just making sure that you are intentionally 
doing that.  I'm glad that no other dialogs rely on this new parameter.

Comment 7

[Charles Manske]

Attachment: move "ValidateNumber()" to common utilities JS file

Comment 8

[Charles Manske]

Latest patch includes change to ValidateNumber input param: input widget passed
in instead of its ID for consistency with 2nd parameter ("listWidget").
ValidateNumber() may be used by Horizontal Rule and Image Dialogs -- file
separate bugs if we want to do that.

Comment 9

[rubydoo123]

P3 priority

Comment 10

[Charles Manske]

Attachment: Updated fix: Change "ValidateNumberString" to clearer "ValidateNumberRange" for all callers and added comments for ValidateNumber() method

Comment 11

[Charles Manske]

Adding review keyword -- ready for final reviews

Comment 12

[Charles Manske]

Attachment: Ooops! forgot to add comments for "minValue" and "maxValue"

Comment 13

[Charles Manske]

Attachment: update patch - some problems apply previous?

Comment 14

[Charles Manske]

Attachment: Forgot change to editor.properties in previous patches.

Comment 15

[Charles Manske]

Attachment: Updated patch: Duplicated logic in property dialogs replaced by using new ValidateNumber() method.

Comment 16

[Charles Manske]

Brade: latest patch should clear up problems you found in your previous testing.

Comment 17

[Charles Manske]

Attachment: Same as last patch, but includes change needed to EdImageProps.xul

Comment 18

[Kathleen :Brade]

r=brade

Comment 19

[Charles Manske]

Need just sr now.

Comment 20

[Simon Fraser [no longer active]]

Please rename 'error' to gError if it really is global, or rewrite the code to 
not require a global at all. 'error' is way too prone to name conflicts for a 
global variable. An alternative way to pass back this error condition would be to 
throw an exception, which might make the code cleaner.

Also, will this kind of validation still work when we change to using key 
listeners which can grab keypresses before they get to the input field?

Fix 'error' and sr=sfraser

Comment 21

[Charles Manske]

Ok, I've changed "error" to "gValidationError" for clarity.
This validation is called separately from the "filter"
validation that happens on typing inside an edit field, so it will have
no effect on future changes to that.

Comment 22

[Charles Manske]

Removing keywords since reviews are finished

Comment 23

[Charles Manske]

checked in.

Comment 24

[sujay]

currently if you have no value for row and column, the OK 
button is dimmed out...so the user won't see any error panel
pop up...is this what this bug is all about?

Comment 25

[James Baxter]

In the 9-06 Build:
1. create a table with 2 rows, 2 columns
2. right click on the table and select properties
3. change the row and columns number to 0
RESULT: Receive error: "The number you entered (0) is outside the allowed range.
 Please enter a number between 1-1000."

1. create a table with 2 rows, 2 columns
2. right click on the table and select properties
3. change the row and columns number to null
RESULT: I get a message informing me that: "Reducing the number of rows or
columns will delete table cells and their contents. Do you really want to do
this?" If I click "Delete Cells" on this the table, and it's contents are deleted.  

Is this the correct behavior, should I see the first error message in both
instances, or as Sujay pointed out - should the OK button be greyed out (like it
is when you atempt to create a table with no value for rows or columns)?

Comment 26

[Kathleen :Brade]

reopen bug based on previous comments
First example is correct behavior.
Second example (no data in edit fields) is wrong.

Comment 27

[Charles Manske]

I think we should have opened a new bug on the specific issue noted.

Comment 28

[Syd Logan]

Closing this bug, opening http://bugzilla.mozilla.org/show_bug.cgi?id=99306

Comment 29

[sujay]

verified. new bug opened