46681 - Disable theme "Apply" button if no item is selected in the tree

Status: VERIFIED FIXED

(Core Graveyard :: Skinability, defect, P3)

Description

[Aaron Lehmann]

A build compiled from July 26, 2000's CVS with the following options:

ac_add_options --disable-mailnews
ac_add_options --disable-tests
ac_add_options --enable-optimize
ac_add_options --disable-debug
ac_add_options --disable-mng
ac_add_options --disable-dtd-debug

Fails with this error message when clicking on OK in the dialog resulting from
clicking "Switch theme" in the themes pane of the preferecnes dialog:

JavaScript error:
 line 0: uncaught exception: [Exception... "Illegal value"  code: "-2147024809"
nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location:
"chrome://communicator/content/pref/pref-themes.js Line: 32"]

This makes it practically impossible to change themes. I have tried deleting my
~/.mozilla directory, which has not helped with this problem.

Comment 1

[Jeffrey Baker]

Pulled the tree at 13:00 27 July 2000, built with:

  ac_add_options --disable-mailnews
  ac_add_options --disable-tests
  ac_add_options --enable-optimize
  ac_add_options --disable-debug
  ac_add_options --enable-strip-libs
  ac_add_options --enable-elf-dynstr-gc

Switched themes back and forth with no problems or javascript errors.

Comment 2

[matt]

themes

Comment 3

[Asa Dotzler [:asa]]

OK, I saw the error in my debug Mac build from this afternoon 7/27 
At first the theme simply did not switch.  I saw the error, hit OK and no theme 
switch.  closed and reopend the prefs and then the themes were not visable.
Then I closed the browser and launched it and the themes were visible I switched 
to classic and crashed with an error type 12.  But when I fired the browser back 
up it had the classic theme.

Comment 4

[sairuh (rarely reading bugmail)]

over to Themes.

Comment 5

[Blake Ross]

Skin switching = Skinability

Comment 6

[Aaron Lehmann]

When I first reported this, no themes showed up in the themes pane, and clicking on the Switch Theme button and OK on the dialog it presented caused this error. With CVS as of today, themes are listed in this pane. Theme switching works, but I still get the same error if I don't select a theme before clicking on Switch Theme. IMHO this button should be grayed out if no theme is selected. Hope this new info doesn't make this bug a duplicate.

Comment 7

[Blake Ross]

Please file that 'Switch Theme should be disabled if no theme chosen' bug 
separately, it sounds entirely different.

I'm still a little confused by the original bug here.  Is the problem that when 
you click Switch Theme and then click OK, the OK button doesn't work?  If so, 
that sounds very much like bug 40884, although the console error in each case 
is different.  I'm still confused as to how the error in 40884 is a JS problem 
with pref-fonts.js, if the user hasn't even switched to that category.

I'm pretty sure this is actually a prefs issue.

Comment 8

[Aaron Lehmann]

Actually, it is still the same bug as the original. At the time that I posted the original bug report, I did not know how to change themes. Since no themes appeared in the list, I pressed the "Switch Theme" button thinking that would prompt me for a theme. In my latest build, theme names do appear in the list. If I click on a theme and click on the "switch theme" button, it works. If there is no theme selected, as was the situation before when no theme names appeared, and the button is pressed and the dialog it brings up is answers, that javascript error results. So the problem really is that the Switch Theme button should be disabled if no theme is selected, but I didn't realize that when I reported the bug. I have changed the subject of the bug to reflect this, I hope this is the right course of action to take.

Comment 9

[Blake Ross]

ah, OK.  I see.  When you kept saying 'I hit OK', I thought you meant OK in the
prefs window -- not OK in the alert that appears upon clicking 'Switch Theme'.
In any case, this will be fixed when the request to preselect the current theme
in the tree is fixed (since there will then be no way to not have a selected
item, unless the "buttons don't take focus" is fixed).  Or, if and when the
"abolish switch theme button, OK button should switch theme" bug is ever fixed,
this will also go away.  I'll leave it open for now, marking minor, though...we
should handle this more gracefully, but I don't see any real loss of function.  

Comment 10

[Blake Ross]

I don't see many (any?) cases where an item in the tree wouldn't be selected, 
unless the user had no themes installed, in which case the user wouldn't be 
attempting to switch themes anyways.  I recommend futuring this...

Comment 11

[Paul Chen]

Sounds good to me, marking Future

Comment 12

[Paul Chen]

Pretriage of skinnability bugs, marking nsbeta1- to get it off the radar

Comment 13

[Gervase Markham [:gerv]]

Currently, what happens is this:

You start with the current theme selected (X). The button says "Apply X". If 
you unselect that theme (Ctrl-click) the button stays the same. However, 
clicking it does nothing. No crash, though. 

We should either:
a) Grey out the button (perhaps removing the theme name) when no theme is 
selected, or
b) Install the theme whose name is on the button anyway.

I'll try taking this if someone can tell me which would be better. Blake?

Gerv

Comment 14

[timeless]

a) disabled "Apply Theme"

Comment 15

[Gervase Markham [:gerv]]

Patch coming up :-) Simple one, this. Clearing nsbeta1- for re-evaluation. 

timeless: can I get an r= from you? Ben - an sr=?

Gerv

Comment 16

[Gervase Markham [:gerv]]

Attachment: patch (4-line JS)

Comment 17

[timeless]

yes, r=timeless

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

hate to be sucha  pedant but can you put the opening brace at the start of the 
else block up on the same line as the else, so it matches the 'if' block? 

sr=[e-mail] ;) 

Comment 19

[Gervase Markham [:gerv]]

Attachment: SRed patch

Comment 20

[Gervase Markham [:gerv]]

timeless? Could you check this in? (Or blake, or ben, or anyone...)

Gerv

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch sr=ed by jag as well ;-)

Comment 22

[Peter "jag" Annema]

Checked in, marking fixed.

Comment 23

[Patty Mac]

Marking verified on commercial build (2001-06-01-08-trunk)