86697 - Error message not displayed in Color Picker dialog.

Status: VERIFIED FIXED

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

Description

[Henrik Gemal]

Error: editorShell has no properties
Source File: chrome://editor/content/EdDialogCommon.js
Line: 246

to reproduce:
- open prefs
- go to Composer -> New Page Settting
- select Use custom colors:
- click on normal text
- blank the input field and press ok
- press Cancel


build 20010618

Comment 1

[rubydoo123]

cmanske

Comment 2

[Ryan Cassin]

Looking into it...

Comment 3

[rubydoo123]

one line fix, removal of js error

reviewed and approved

Comment 4

[rubydoo123]

removing nsBRanch and pushing out to 1.0

Comment 5

[Charles Manske]

changing milestone to 0.9.4

Comment 6

[Charles Manske]

This is more than just a JS error. The problem is that we don't know about
editorShell when in the pref dialog, so calling "ShowInputError()" method fails
because it tries to use editorShell's messagebox utility. So the real problem
is that we don't show the error message at all.
Changing Summary to reflect the real problem.

Comment 7

[Charles Manske]

So once I fixed the basic problem in ShowInputErrorMessage() method, testing
of the Color Picker dialog revealed other problems:
1. The error message would appear twice: once because of call to onOK()
 from "onOKClick()", but then oncommand calling of onOK() would validated and
 show the error again.
2. Default key shifting from the "Last-picked Color" button to OK button wasn't
happening correctly, or when you did use "Enter" key, you didn't get the 
last-picked color when you should, you would get whatever color was under your
mouse cursor.
So the patch also fixes those problems as well.

Comment 8

[Charles Manske]

Since the arrow keys don't update the colorpicker, the new method
SelectColorByKeypress(aEvent) in the patch should only set the color when 
spacebar is used:
function SelectColorByKeypress(aEvent)
{
  if (aEvent.keyCode == aEvent.DOM_VK_SPACE)
  {
    SelectColor();
    SetDefaultToOk();
  }
}

Comment 9

[Kathleen :Brade]

r=brade

Comment 10

[kinmoz]

The one thing that bugs me about the 08/13/01 13:00 patch is the gOnOKCalled 
workaround ... are we working around a toolkit bug? Why don't other dialogs 
exhibit this same called twice behavior, or do they?

Comment 11

[Charles Manske]

They don't 'cause no one else is silly enough to put an "onclick" and "oncommand"
handler on the same button, probably!

Comment 12

[Charles Manske]

Attachment: Updated patch to simplify onclick/oncommand handling

Comment 13

[Charles Manske]

Code was simplified:
Realizing that "oncommand" handler for the OK is called after "onclick",
I could simply make "onclick" clear the LastPicked color info by calling
SetDefaultToOk(). This removed need for hacky "gOnOKCalled" and onOkClick() 
method.
Note that the code in EdDialogCommon.js is not longer needed, since it is
superceded by the fix for bug 96649, which fixed the error message reporting.

Comment 14

[Kathleen :Brade]

Remove keycode == 0 stuff; use charcode instead of keycode for space character.  
If the comparison isn't removed, we'll also trigger the code when typing letters, 
numbers, etc.  Remove both comment lines in that function.

r=brade only with those changes

Comment 15

[kinmoz]

The 08/28/01 15:15 looks much cleaner! Thanks for losing the gOnOKCalled hack.

sr=[e-mail] with brade's provisions above.

Comment 16

[Charles Manske]

Everything done as requested, here's the new method:
function SelectColorByKeypress(aEvent)
{
  if (aEvent.charCode == aEvent.DOM_VK_SPACE)
  {
    SelectColor();
    SetDefaultToOk();
  }
}

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz on behalf of drivers

Comment 18

[Charles Manske]

checked in

Comment 19

[James Baxter]

Verified on 8-30 Build