329415 - trying to remove exceptions in recurrencedialog with no exceptions selected throws JS-errors

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Robin Edrenius]

If no exception in the recurrenc-edialog is selected and you press 'remove exceptions', you get a JS error:

Error: item has no properties
Source File: chrome://calendar/content/calendar-recurrence-dialog.js
Line: 357

Comment 1

[Robin Edrenius]

Attachment: Puts removeExceptions() in an if-block, v1

This should remove the JS-errors.

We should probably disable the button when no exceptions is selected as well.

Comment 2

[Robin Edrenius]

Attachment: This patch deals with disabling the button

This patch disables the 'remove exceptions-button' if no exception is selected.

Should the if-block from the previous patch be removed now? Since the button can't be pressed when no exception is selected with this patch.

Comment 3

[Robin Edrenius]

Attachment: This patch deals with disabling the button, v2

Fixes a whitespace change in the previous patch, sorry for the bugspam.

Comment 4

[Joey Minta]

Comment on attachment 214085 [details] [diff] [review]
This patch deals with disabling the button, v2

     self.focus();
+    checkSelectedException();
Focusing should almost always be the last call.  Move this up a bit.

+    if (item) {
+
+        var addedRecently = false;
+        for (var ii in window.addedExceptions) {
The whole point of this patch is to make it so this function is never reached if 'item' would be null, so your suspicion that this is not needed is correct.

+    var item = exceptionList.selectedItem;
+        if(!item) {
+        document.getElementById("remove-exceptions-button").setAttribute("disabled", "true");
+        }
+        if(item) {
+        document.getElementById("remove-exceptions-button").removeAttribute("disabled");
+        }
The indenting here looks weird.  Also, this looks like the classic strucutre for if/else, not if/if.

+}
\ No newline at end of file
Oops, can you fix that warning?

onclick="checkSelectedException()"
What's the reason for doing onclick as well?  In theory, onselect ought to be the only case we care about, since we're basing our enable/disable on the selection.

Comment 5

[Robin Edrenius]

Attachment: Patch v3

(In reply to comment #4)
>      self.focus();
> +    checkSelectedException();
> Focusing should almost always be the last call.  Move this up a bit.
>

Fixed.
 
> 
> +    var item = exceptionList.selectedItem;
> +        if(!item) {
> +       
> document.getElementById("remove-exceptions-button").setAttribute("disabled",
> "true");
> +        }
> +        if(item) {
> +       
> document.getElementById("remove-exceptions-button").removeAttribute("disabled");
> +        }
> The indenting here looks weird.  Also, this looks like the classic strucutre
> for if/else, not if/if.
> 

Fixed the indenting and changed to if/else.

> +}
> \ No newline at end of file
> Oops, can you fix that warning?
> 

Doh! Fixed =)

> onclick="checkSelectedException()"
> What's the reason for doing onclick as well?  In theory, onselect ought to be
> the only case we care about, since we're basing our enable/disable on the
> selection.
> 

I was under the impression that it was possible to deselect the items in the listbox by clicking in an empty area of the listbox, however, that was not the case. onclick removed.

Comment 6

[Joey Minta]

(In reply to comment #5)
> 
> > 
> > +    var item = exceptionList.selectedItem;
> > +        if(!item) {
> > +       
> > document.getElementById("remove-exceptions-button").setAttribute("disabled",
> > "true");
> > +        }
> > +        if(item) {
> > +       
> > document.getElementById("remove-exceptions-button").removeAttribute("disabled");
> > +        }
> > The indenting here looks weird.  Also, this looks like the classic strucutre
> > for if/else, not if/if.
> > 
> 
> Fixed the indenting and changed to if/else.
> 
Looks to me like the if/else change didn't make it.

Comment 7

[Robin Edrenius]

Attachment: Patch v4

(In reply to comment #6)
> Looks to me like the if/else change didn't make it.
> 

Hmm...I must have updated the wrong tree or something.

However, here is an updated version!

Comment 8

[Joey Minta]

Comment on attachment 214555 [details] [diff] [review]
Patch v4

+    var item = exceptionList.selectedItem;
+    if(!item) {
+        document.getElementById("remove-exceptions-button").setAttribute("disabled", "true");
+    }
+    else {
+        document.getElementById("remove-exceptions-button").removeAttribute("disabled");
+    }
Nit: if (foo) is proper style, with a space between the f and the (
Nit: else goes on the same line as }

-      <listbox id="recurrence-exceptions-listbox" rows="8" flex="1"/>
+      <listbox id="recurrence-exceptions-listbox" rows="8" flex="1" onselect="checkSelectedException()"/>
       <vbox align="top">
         <datepicker id="exdate-picker"/>
         <button label="&newevent.addexceptions.label;" oncommand="addException()"/>
-        <button label="&newevent.recurrence.remove.label;" oncommand="removeSelectedException();"/>
+        <button label="&newevent.recurrence.remove.label;" id="remove-exceptions-button" oncommand="removeSelectedException();"/>
Nit: wrap lines longer than 80chars.

Everything else looks great! r=jminta with the nits picked.

Comment 9

[Joey Minta]

Thanks for the work on this! patch checked in.