106536 - Blank charset list in SaveAs charset of HTML composer

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[nhottanscp]

The current trunk build, charset list is empty for SaveAs charset.
I see that also in 0.9.5.
Juraj, do you know anything about this? When is the last time you saw it's 
working?

Comment 1

[nhottanscp]

Attachment: Screen shot, empty charset list.

Comment 2

[[e-mail] (away - not reading bugmail)]

accepting as regression - will investigate what caused it. Targeting M096 for 
now, since this is quite urgent.

Comment 3

[R.K.Aa.]

Today mailnews crashed when i clicked on mails.
I then discovered that the charset for the folder was empty.
Setting it to western, iso8859-1 cured the crash.

1: shouldn't crash on missing charset
2: i specifically set the charset ti iso8859-1 less than a week ago when i
discovered it for some unexplainable reason was set to arabic (which seems to be
some default for all mailnews folders also??)

Linux 2001102408

Comment 4

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 5

[nhottanscp]

It's broken using 9/21 trunk build. I can't find earlier daily builds but 
mozilla 0.9.4 does not have the problem.
Here is a list of check in between 9/4 (0.9.4 freeze date) and 9/21.

Checkins to directory mozilla/editor between 09/04/2001 00:00 and 09/21/2001 
11:00 : 

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&bra
nchtype=match&dir=mozilla%2Feditor&file=&filetype=match&who=&whotype=match&sortb
y=Date&hours=2&date=explicit&mindate=09%2F04%2F2001+00%3A00%3A00&maxdate=09%2F21
%2F2001+11%3A00%3A00&cvsroot=%2Fcvsroot

Comment 6

[nhottanscp]

In EditorSaveAsCharset.js, I dumped charsetNode.Value and it has a right value.
I also see the string passed to AppendStringToTreelist() looks fine, and
AppendStringToTreelist() seems to be working fine because it is also used by
link property dialog and I see it's listing the links witout problem.
So no idea where it's broken. Juraj, any idea?
What about line 139, item.firstChild.firstChild.setAttribut?
What is this line supposed to do?


http://lxr.mozilla.org/mozilla/source/editor/ui/dialogs/content/EditorSaveAsCharset.js#135

135     for (var i = 0; i < rdfContainer.GetCount(); i++) 
136         {
137       charsetNode =
availableCharsets.getNext().QueryInterface(Components.interfaces.nsIRDFResource);
138       item = AppendStringToTreelist(gDialog.charsetTree,
readRDFString(rdfDataSource, charsetNode, kNC_name));
139       item.firstChild.firstChild.setAttribute("value", charsetNode.Value);
140       if(charset == charsetNode.Value) 
141         selectedItem = item;
142     }

Comment 7

[[e-mail] (away - not reading bugmail)]

Naoki,

item.firstChild.firstChild.setAttribute sets the charset "ID" which as oppesed 
to the pretty name which is inserted using AppendStringToTreelist().

I tried a few things and it looks to me that the XUL syntax has changed and 
some little detail about the tree declaration in the XUL file and/or data 
population in JS is killing us. I'm still looking into it - what a pain! It's 
about time they freeze XUL once and for all ;-)

Comment 8

[[e-mail] (away - not reading bugmail)]

looks like an XPToolkit problem. Removing ensureElementIsVisible() will at 
least render the tree, although the scrollbar will be missing and there will 
also be a repaint problem. I'll try to figure out, what to change (or add) in 
the XUL tree definition to avoid this problem.

http://lxr.mozilla.org/mozilla/source/editor/ui/dialogs/content/EditorSaveAsChar
set.js#147

Comment 9

[sujay]

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

Comment 10

[[e-mail] (away - not reading bugmail)]

Attachment: proposed patch v1

Comment 11

[nhottanscp]

Juraj, does the patch fix the problem? If so, please get reveiews and check in
for 0.9.6.

Comment 12

[[e-mail] (away - not reading bugmail)]

Naoki could you r= the patch? It fixes the problem, but the item will not be 
scrolled to. Seems like an XPToolkit issue to me. The patch should be good 
enough for M096...

Comment 13

[[e-mail] (away - not reading bugmail)]

cmanske: 

could you please r= this change?

Comment 14

[Charles Manske]

Comment on attachment 56219 [details] [diff] [review]
proposed patch v1

So we're not supporting the "rows" parameter for trees any more? 
I just want to be sure this fix isn't hiding a different problem in XUL.

Comment 15

[Charles Manske]

Ok, I see from other comments that this is a "temporary" fix, right? Let's be sure
the appropriate toolkit people are aware of such problems.
r=cmanske
if this type of fix is ok with XPFE folks.

Comment 16

[[e-mail] (away - not reading bugmail)]

thanks cmanske, I agree that this is most likely a regression in XPFE Toolkit 
and will file a follow-up bug against them.

Comment 17

[John Morrison]

Okay, I tracked this down to the window from 09/18/2001 08:00:00 to
09/21/2001 08:00:00. I didn't have builds for a narrower window. 
(Warning, if you do that bonsai query, be aware that you will pick 
up the great whack-the-license-then-back-it-out checkin, so be prepared
to wait a while :-]).

I could see (using the world's greatest tool, the DOM Inspector) that 
the treecells all had zero width/height.

Anyways, after reviewing the list, I backed out 
 cvs update -j1.78 -j1.77 content/shared/public/nsXULAtomList.h
and then 
 cvs update -j1.35 -j1.34 layout/xul/base/src/nsBox.cpp
both of which had to do with checking for values of (min)height/width.

After the second backout, this dialog was fixed without the band-aid fix
already attached above (and the first wasn't likely required, but I didn't
want to rebuild content again).

I propose that you check in the band-aid for the 0.9.6 _branch_, but that
we don't put it on the trunk, since this seems like a basic bug in box 
layout and probably should be fixed. Sound like a plan?

Comment 18

[[e-mail] (away - not reading bugmail)]

jrgm - thanks for the great work! Judging by the nature of this problem it 
seems quite reasonable to assume that this is related to hyatt's checkin (Make 
XUL boxes check maxheight/maxwidth attrs as well as minwidth. r/sr=hewitt,jst). 
Should we file a new bug for the trunk fix then?

I'll try to get a sr= and a= for the M096 branch since this is an i18n blocker. 
Chris could you help us out one more time with a sr=?

Comment 19

[[e-mail] (away - not reading bugmail)]

Comment on attachment 56219 [details] [diff] [review]
proposed patch v1

M096 branch only

Comment 20

[David Hyatt]

Wait, let's fix this the right way.  I accidentally removed the 

if (val > etc.)
  blah blah

checks from nsBox.cpp's checking of minheight and minwidth.  Restoring those
checks should fix the problem.  I didn't actually mean to remove them.

Comment 21

[[e-mail] (away - not reading bugmail)]

Attachment: patch v2 - based on hyatt's comments it adds two lines which have been accidentaly removed

Comment 22

[[e-mail] (away - not reading bugmail)]

thanks Dave - that really helped.

BTW what are the chances to get a r=/sr= before the milestone deadline wheezes 
by? I realize this is an 11th hour effort, yet I have to give it try :-)

Comment 23

[David Hyatt]

Fix the indentation (the lines under the if statements should be indented 2
spaces).  sr=hyatt

Comment 24

[[e-mail] (away - not reading bugmail)]

ugh, the -w -b diff options are nothing but trouble ;-) Well, it's fixed at 
last, so thanks everyone!

Comment 25

[Yuying Long]

Fixed verified on 11-07 trunk build.