233191 - to/cc/bcc menulist in addressing widget in compose window is too wide

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Scott MacGregor]

to/cc/bcc menulist in addressing widget in compose window is too wide

let me screen shot.

this is with this mornings build, classic skin.

from the console:

WARNING: empty damage rect: update caller to avoid fcn call overhead, file d:/tr
ees/vcard/mozilla/layout/html/base/src/nsFrame.cpp, line 2506
###!!! ASSERTION: Row index out of range!: 'aRowIndex >=0 && aRowIndex < GetRowC
ount(aIsHorizontal)', file d:/trees/vcard/mozilla/layout/xul/base/src/grid/nsGri
d.cpp, line 584
###!!! ASSERTION: Row index out of range!: 'aRowIndex >=0 && aRowIndex < GetRowC
ount(aIsHorizontal)', file d:/trees/vcard/mozilla/layout/xul/base/src/grid/nsGri
d.cpp, line 598
###!!! ASSERTION: Row index out of range!: 'aRowIndex >=0 && aRowIndex < GetRowC
ount(aIsHorizontal)', file d:/trees/vcard/mozilla/layout/xul/base/src/grid/nsGri
d.cpp, line 613
###!!! ASSERTION: The recipient list is not supposed to be null -Fix the caller!
: 'recipients', file d:/trees/vcard/mozilla/mailnews/compose/src/nsMsgCompFields
.cpp, line 589

Comment 1

[Scott MacGregor]

Attachment: screen shot

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does reverting the last change to nsXULElement.cpp help?

Comment 3

[Scott MacGregor]

modern has it too.  

(but you have to use View | Apply theme from the browser to switch, due to bug 
#231769)

Comment 4

[Greg Onufer]

This problem exists on Solaris/SPARC as well, it's not restricted to
PC/Windows2k    

Comment 5

[Scott MacGregor]

yes, backing out the change to nsXULElement.cpp (from bug #197427) fixes this 
problem.

thanks bz.  I'll re-assign to jst, since it was his change.

Comment 6

[Alfonso Martinez]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Actually, jst's change just fixed a number of DOM-compliance bugs in the XUL
element (by making it use the same code as HTML elements do, which is far better
tested and more correct).  So chances are, anything that broke was relying on a
bug...

Comment 8

[Johnny Stenback (:jst)]

Attachment: Make us fire correct insert/append notifications when inserting/appending elements to the DOM

This fixes the problem, thanks to bz for figuring out most of this. The actual
problem appears to be in the XBL binding manage's notification handling, and
bz's looking into that problem.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Based on info from bc, this shoul fix bug 233137 too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140701 [details] [diff] [review]
Make us fire correct insert/append notifications when inserting/appending elements to the DOM

>Index: content/base/src/nsGenericElement.cpp
>+  PRBool isAppend = aIndex == GetChildCount();

Do we care that this runs even if aNotify is false?

In any case, put parens about the equality test to make this more clear.

>@@ -2776,19 +2783,23 @@ nsGenericElement::doInsertBefore(nsICont
>-    res = aElement->InsertChildAt(newContent, refPos, PR_TRUE, PR_TRUE);
>+    if (!aRefChild) {
>+      res = aElement->AppendChildTo(newContent, PR_TRUE, PR_TRUE);
>+    } else {
>+      res = aElement->InsertChildAt(newContent, refPos, PR_TRUE, PR_TRUE);
>+    }

Why do we need this change now that we fixed up InsertChildAt to fire the other
notification?

>Index: content/xul/content/src/nsXULElement.cpp

>+    PRBool isAppend = aIndex == GetChildCount();

As above.

>+    if (mDocument) {
>+        aKid->SetDocument(mDocument, aDeepSetDocument, PR_TRUE);

Why only do this if mDocument?	Are we guaranteed that the kids have a null
mDocument coming in here?  If so, we should probably assert that....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to nsBindingManager (don't check in -- that assert fires)

Comment 12

[Johnny Stenback (:jst)]

Fix checked in. Leaving bug open to figure out the nsBindingManager issue...

Comment 13

[Boris Zbarsky [:bzbarsky]]

I should just take this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140701 [details] [diff] [review]
Make us fire correct insert/append notifications when inserting/appending elements to the DOM

sr=bzbarsky with the comments I made and the irc discussion.

Comment 15

[Aleksey Nogin]

This have introduced a new "may be used uninitialized" warning on brad TBox:

+content/base/src/nsGenericElement.cpp:2412
+ `PRBool isAppend' might be used uninitialized in this function

Comment 16

[Johnny Stenback (:jst)]

That's a bogus warning, it won't be used w/o being initialized. Silly compiler :-)

Comment 17

[lohphat]

Appears fixed in 2004021109 win32.  Can anyone else confirm?

Comment 18

[Scott MacGregor]

lophat, see comment #12

http://bugzilla.mozilla.org/show_bug.cgi?id=233191#c12

Comment 19

[lohphat]

Perhaps a change to the summary description since it's no longer relevant to the
open issue?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Please stop spamming this bug, ok?  The summary is relevant, because that's how
I can find this bug...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to binding manager that sorta works

Well, works as well as any of the rest of the code at least...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142299 [details] [diff] [review]
Patch to binding manager that sorta works

The algorithm was suggested by hyatt, by the way.  So r=me on the general
approach if that helps with the review.  ;)

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 142299 [details] [diff] [review]
Patch to binding manager that sorta works

Looks good. r+sr=jst

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 25

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

This checkin (tracked by Neil) has broken the behavior of the hierarchical
status bar in Composer and Nvu... 

Comment 26

[Boris Zbarsky [:bzbarsky]]

Daniel, if you can file a bug on that, preferably with as small a testcase as
possible that shows the problem, I'll look into it....