298841 - JS error: "less0 has no properties" and first lessButton in edit virtual folder not fully functional

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Opening the dialog for editing virtual folders shows following js error:

Error: document.getElementById("less0") has no properties
Source File: chrome://messenger/content/virtualFolderProperties.js
Line: 112

Also the disabled state of the lessButton is not updated correctly due to the
missing property disabled. Patch is on the way.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v1

This patch adds the missing ids to the lessButtons and sets their disabled
state over setAttribute due to the missing property.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v2

Same like patch v1 but with updated comment for function updateRemoveRowButton

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v3 more simpler fix

Ok, I simplified the patch a lot. We shouldn't add an id for each button. If
the user removes e.g. the first button we would run into trouble because
id="less0" doesn't exist anymore. This fix also fixes this part.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: the correct patch v3

Comment 5

[Karsten Düsterloh]

Comment on attachment 187427 [details] [diff] [review]
the correct patch v3

>Index: mailnews/base/search/resources/content/searchTermOverlay.js
>===================================================================
>-    firstListItem.lastChild.lastChild.lastChild.disabled = gTotalSearchTerms == 1;
>+    firstListItem.lastChild.lastChild.lastChild.setAttribute("disabled", gTotalSearchTerms == 1);

Strangely enough, the disabled property really doesn't work, so the
setAttribute is needed! I wonder if something's broken there...

> function getSearchRowIndexForElement(aElement)
> {
>   var listitem = aElement;
>   while (listitem)
>-{
>+  {
>     if (listitem.localName == "listitem") 
>       break;
>     listitem = listitem.parentNode;
>   }
> 
>   return gSearchTermList.getIndexOfItem(listitem);

You could write that as

var listitem;
for (listitem = aElement; 
     listitem && listitem.localName == "listitem"; 
     listitem = listitem.parentNode);
return gSearchTermList.getIndexOfItem(listitem);

(if you like).

> function onLess(event)
> {
>   if (gTotalSearchTerms > 1) 
>   {
>-    var rowIndex;
>-    if (event)
>-{
>+    if (event) 
>+    {
>       removeSearchRow(getSearchRowIndexForElement(event.target));    
>       --gTotalSearchTerms;
>     }
>   }

It'd be shorter to say

if (event && gTotalSearchTerms > 1) 



r=me with the onLess nit fixed. And please fix the 

JavaScript strict warning: chrome://messenger/content/searchTermOverlay.js,
line 198: assignment to undeclared variable i

and the broken indentation there for bonus points, while your at it. ;-)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v4

(In reply to comment #5)
> You could write that as
> 
> var listitem;
> for (listitem = aElement; 
>      listitem && listitem.localName == "listitem"; 
>      listitem = listitem.parentNode);
> return gSearchTermList.getIndexOfItem(listitem);

That's not true. If you use that, no listitem will be returned. You have to
execute the for loop while "listitem.localName != "listitem"). But I won't use
that, it looks a little bit complex. So I modified the while loop to fetch your
mentioned way.

> r=me with the onLess nit fixed. And please fix the 

Carrying over r with that fixes.

> JavaScript strict warning: chrome://messenger/content/searchTermOverlay.js,
> line 198: assignment to undeclared variable i

Finally done.

Comment 7

[Karsten Düsterloh]

Eww, a typo :((

Comment 8

[David :Bienvenu]

Comment on attachment 187755 [details] [diff] [review]
patch v4

Neil can probably say what's going on with the enabled/disabled attribute.

Comment 9

[[e-mail]]

Comment on attachment 187755 [details] [diff] [review]
patch v4

>-    for (i = 0; i < searchTerms.Count(); i++) {
>+    for (var i = 0; i < searchTerms.Count(); i++) {
Odd, I wonder why my build isn't whining about this.

>-    firstListItem.lastChild.lastChild.lastChild.disabled = gTotalSearchTerms == 1;
>+    firstListItem.lastChild.lastChild.lastChild.setAttribute("disabled", gTotalSearchTerms == 1);
Ah, of course, I forgot that .disabled doesn't work for a newly created
element. Sigh :-[

>-  if (gTotalSearchTerms > 1) 
>+  if (event && gTotalSearchTerms > 1) 
>   {
>-    var rowIndex;
>-    if (event)
>-{
>-      removeSearchRow(getSearchRowIndexForElement(event.target));    
>-      --gTotalSearchTerms;
>-    }
>+    removeSearchRow(getSearchRowIndexForElement(event.target));    
>+    --gTotalSearchTerms;
If you thought this was bad, mscott's original patch was worse.

>     initializeSearchRows(nsMsgSearchScope.offlineMail, aSearchTerms);
>-    if (aSearchTerms.Count() == 1)
>-      document.getElementById("less0").setAttribute("disabled", "true");
>+    updateRemoveRowButton();
Doesn't initializeSearchRows already call updateRemoveRowButton?

>   }
>   else
>     onMore(null, 0);
This should just be onMore(null) now.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: patch v5 with all mentioned changes

(In reply to comment #9)
> >	initializeSearchRows(nsMsgSearchScope.offlineMail, aSearchTerms);
> >-	if (aSearchTerms.Count() == 1)
> >-	  document.getElementById("less0").setAttribute("disabled", "true");
> >+	updateRemoveRowButton();
> Doesn't initializeSearchRows already call updateRemoveRowButton?

Uuh, you are right. I removed the crap.

> This should just be onMore(null) now.

Fixed.

I also updated the function onMore(event). gSearchTermList.getRowCount() is
called every time also when it's not needed by an event. In that case we save a
function call every time when a search term is added.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I believe this bug should be fixed now? I hope we could checkin the fix ASAP.
Only the sr is missing.

Comment 12

[[e-mail]]

Comment on attachment 187829 [details] [diff] [review]
patch v5 with all mentioned changes

I noticed that the title of the virtual properties dialog for an existing saved
search isn't set correctly, is there already a bug open for this?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> I noticed that the title of the virtual properties dialog for an existing saved
> search isn't set correctly, is there already a bug open for this?

Yes, this is covered by bug 294407.

Comment 14

[Chris Beard]

Comment on attachment 187829 [details] [diff] [review]
patch v5 with all mentioned changes

a=cbeard, let's get this landed ASAP to make 1.8b3

Comment 15

[Karsten Düsterloh]

Checking in searchTermOverlay.js;
/cvsroot/mozilla/mailnews/base/search/resources/content/searchTermOverlay.js,v 
<--  searchTermOverlay.js
new revision: 1.43; previous revision: 1.42
done

Checking in virtualFolderProperties.js;
/cvsroot/mozilla/mailnews/base/resources/content/virtualFolderProperties.js,v 
<--  virtualFolderProperties.js
new revision: 1.11; previous revision: 1.10
done

Comment 16

[Karsten Düsterloh]

cbeard: sorry for killing that blocking flag! :(
But it's irrelevant now anyway. :)