Error message not displayed in Color Picker dialog.

VERIFIED FIXED in mozilla0.9.4

Status

()

P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: bugzilla, Assigned: cmanske)

Tracking

Trunk
mozilla0.9.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
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

18 years ago
cmanske
Assignee: beppe → cmanske
Priority: -- → P3
Target Milestone: --- → mozilla0.9.3

Comment 2

18 years ago
Looking into it...

Comment 3

18 years ago
one line fix, removal of js error

reviewed and approved
Status: NEW → ASSIGNED
Keywords: nsBranch

Comment 4

18 years ago
removing nsBRanch and pushing out to 1.0
Keywords: nsBranch
Target Milestone: mozilla0.9.3 → mozilla1.0
(Assignee)

Comment 5

18 years ago
changing milestone to 0.9.4
Target Milestone: mozilla1.0 → mozilla0.9.4
(Assignee)

Comment 6

18 years ago
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.
Summary: javascript error in EdDialogCommon.js → Error message not displayed in Color Picker dialog.
(Assignee)

Comment 7

18 years ago
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.

(Assignee)

Updated

18 years ago
Keywords: patch, review
Whiteboard: FIX IN HAND need r=, sr=
(Assignee)

Comment 8

18 years ago
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

18 years ago
r=brade
Whiteboard: FIX IN HAND need r=, sr= → FIX IN HAND need sr=

Comment 10

18 years ago
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?
(Assignee)

Comment 11

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

Comment 12

18 years ago
Created attachment 47386 [details] [diff] [review]
Updated patch to simplify onclick/oncommand handling
(Assignee)

Comment 13

18 years ago
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.
Whiteboard: FIX IN HAND need sr= → FIX IN HAND need r=, sr=

Comment 14

18 years ago
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

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

sr=kin@netscape.com with brade's provisions above.
(Assignee)

Comment 16

18 years ago
Everything done as requested, here's the new method:
function SelectColorByKeypress(aEvent)
{
  if (aEvent.charCode == aEvent.DOM_VK_SPACE)
  {
    SelectColor();
    SetDefaultToOk();
  }
}
Whiteboard: FIX IN HAND need r=, sr= → FIX IN HAND reviewed
(Assignee)

Comment 18

18 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Keywords: patch, review
Resolution: --- → FIXED
Whiteboard: FIX IN HAND reviewed

Comment 19

18 years ago
Verified on 8-30 Build
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.