227013 - Creating a new filter does not show up in filter list dialog until you close it

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Scott MacGregor]

Create a new filter. Notice that the filter does not actually show up in the
filter dialog list until you close the list dialog and bring it up again.

Comment 1

[Scott MacGregor]

I found the source of this problem. It is in seamonkey too:

Hi Johnny, this checkin caused a regression in mailnews. See Bug #227013. 

In short after this change the following no longer works:

  window.openDialog("chrome://messenger/content/FilterEditor.xul",
"FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);

  dump('post refresh call\n');
  if ("refresh" in args && args.refresh)
    refresh();

Without this patch, I see the dump statement get executed. With this patch, the
script never resumes after the return from window.openDialog.

I have not quite figured out the relationship as to why this is so. 

Comment 2

[Scott MacGregor]

This was caused (or exposed) by Bug #224549

Comment 3

[Brendan Eich [:brendan]]

Any sign of an exception?  Likely one is being thrown, which was dropped on the
floor wrongly, prior to the fix for bug 224549 going in.

/be

Comment 4

[Brendan Eich [:brendan]]

mscott, does this affect SeaMonkey mailnews too?

/be

Comment 5

[Oleg Sidletskiy]

perhaps unrealed, just clicking "New..." in Message Filters window, produces an
error in JavaScript console:

Error: menulist.selectedItem has no properties
Source File: chrome://messenger/content/mailWidgets.xml
Line: 1520

Comment 6

[Scott MacGregor]

Yes this effects seamonkey and thunderbird.

I don't see any JS exceptions in the console when control returns to the caller.
I'm guessing the window.open call is not wrapped in a try/catch. Nor is an
exception thrown by the filter dialog's onAccept method which is called before
control returns from the modal dialog. I'll try to follow more closely what
happens after the modal dialog's onAccept method.

Comment 7

[Scott MacGregor]

That's a long standing exception that I think has been around for a long long
time and I need to figure out why it is showing up.


That being said, you are right, that JS error is getting dropped on the floor
and that's what prevents script from resuming after the modal window.open call
in the filter list dialog. 

Comment 8

[Scott MacGregor]

Attachment: the fix (remove offending line that was triggering the exception)

I don't know why menuList.selectedItem is always empty when initializing a
search attribute in the filter dialog. However, according to lxr, the line in
question was added to ensure that the attribute values for Priority and Status
attribute fields are left justified in the menu list instead of coming up
centered.  See Bug #129510 for details about that work around. 

By removing this line, I am undoing the work around in 129410 but so far to no
ill effects. The attribute values for priority and status load just fine with
left justification. 

Hence this patch does two things:
1) Removes a work around which is no longer necessary
2) By no longer calling menuList.selectedItem, the xbl binding for a search
attribute no long generates a JS error. This in turn ensures that the new
filter dialog no longer returns an error to its parent filter list dialog.

Comment 9

[Brendan Eich [:brendan]]

From the same file, via lxr:

117       <property name="selectedItem">
118         <getter><![CDATA[
119           return this.selectedItems.length > 0 ? this.selectedItems[0] : null;
120         ]]></getter>
121         <setter><![CDATA[
122           this.selectItem(val);
123         ]]></setter>
124       </property>

So the selectedItem getter clearly returns null if the selectedItems array is
empty.  Which it certainly could be upon initialize(), no?

jst, is the exception properly propagating across the openDialog?

/be

Comment 10

[Scott MacGregor]

Brendan, I think menuList.selectedItem is referring to an actual xul:menulist
binding and not a binding to a widget in mailWidgets.xml. That being said, I'm
sure  the selectedItem method on a menu list looks similar to the widget you cited. 

Comment 11

[Brendan Eich [:brendan]]

mscott: duh, yeah -- but it seems like any selectedItem member could be null, so
maybe there's just been a long-standing lack of null-defensiveness.  Even better
if we can do without the workaround altogether, now.  Is it clear why we can?  I
hate a mystery.

/be

Comment 12

[[e-mail]]

Comment on attachment 136485 [details] [diff] [review]
the fix (remove offending line that was triggering the exception)

I'm all for simplifying code ;-)

Comment 13

[Scott MacGregor]

Update: 
Still trying to figure out why it is clear that we can remove the work around to
answer brendan's concern.

Comment 14

[Scott MacGregor]

The work around was put in place to get around this XBL bug:

http://bugzilla.mozilla.org/show_bug.cgi?id=63370

If we can find proof that 63370 was fixed by another bug and just not marked as
fix then we should have an explanation as to why we can remove the work around. 

Comment 15

[Scott MacGregor]

*** Bug 90739 has been marked as a duplicate of this bug. ***

Comment 16

[Brendan Eich [:brendan]]

mscott: I'd settle for an explanation of how bug 63370, which seems to be valid
still, requires the particular workaround.  Especially since you see no symptoms
when the workaround is removed! ;-)

/be

Comment 17

[Scott MacGregor]

jst's fix for Bug #226617 also "fixes" this problem. Sounds like that patch is
going into the 1.6b build for sure.

I'm going to keep this open separately though because we should still address
the JS error we are generating by hopefully removing this obsolete work around.

Comment 18

[Johnny Stenback (:jst)]

FYI, the patch in bug 226617 is now checked in on the trunk.

Comment 19

[Scott MacGregor]

now that the trunk is open for 1.7a, I checked this in.