Closed Bug 178183 Opened 22 years ago Closed 21 years ago

Cancel button inoperative on "Download Font" dialog

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Platform:
x86
Windows 98
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.4final

People

(Reporter: mozilla, Assigned: sspitzer)

References

Details

(4 keywords, Whiteboard: [adt2][fixed on trunk and branch])

Attachments

(7 files, 10 obsolete files)

Screenshot of Download Font box
8.32 KB, image/png
Details
Addressed jag's comments and changed license
16.84 KB, patch
sspitzer
: review-
jag+mozilla
: superreview+
Details | Diff | Splinter Review
The dialog on W2K, Classic theme
13.10 KB, image/jpeg
Details
The dialog on W2K, Modern theme
13.56 KB, image/jpeg
Details
The dialog on W9x, Modern theme
11.89 KB, image/jpeg
Details
The dialog on W9x, Classic theme
11.09 KB, image/jpeg
Details
patch, per jag / simon's comments
13.82 KB, patch
Details | Diff | Splinter Review 
2002110108 trunk

Open a page that uses a foreign charset. Mozilla pops up the dialog telling you
that you need to install a character set to view the page. The cancel button
doesn't work.

This is a recent regression (in the last week or so).
Keywords: regression
Looks like font dialog problem, change QA contact to Carine.
Keywords: intl
QA Contact: ylong → carineh00
Verified in Trunk build of 11/04/2002: Cancel button does not work. 

The Cancel button for font download does work in the Branch build of 11/03/2002.
*** Bug 178954 has been marked as a duplicate of this bug. ***
Summary: Cancel Inoperative on Language Dialog → Cancel Inoperative on Font Download dialog
Biesi, this seems to have regressed before you checked in bug 177875, but cc-ing
you just in case.
*** Bug 182003 has been marked as a duplicate of this bug. ***
Nomonate as nsbeta1.
Keywords: nsbeta1
*** Bug 182881 has been marked as a duplicate of this bug. ***
*** Bug 183116 has been marked as a duplicate of this bug. ***
*** Bug 185232 has been marked as a duplicate of this bug. ***
*** Bug 186083 has been marked as a duplicate of this bug. ***
*** Bug 186280 has been marked as a duplicate of this bug. ***
*** Bug 186492 has been marked as a duplicate of this bug. ***
Verified on:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212

No action from Cancel button on "Download Font" dialog.
This could be due to bug 32571
(it was originally checked in on 2002-10-25 13:36, see comment 82 there)

That is because the font download dialog is actually a remote xul file:
http://www.mozilla.org/projects/intl/fonts/win/en/package_*.html

It looks like this file does not have chrome privileges.
And clicking "Cancel" in that dialog invokes "window.close();".

to verify if that is the case:
put user_pref("dom.allow_scripts_to_close_windows", true); in your user.js file,
restart mozilla and try again
oh yeah, I actually think that instead of using that pref, you could also just
look in the JS Console after clicking cancel.
Bug 186083, which was closed as a dupe of this one, also staes that the dialog
doesn't get localized correctly, and I couldn't find the strings in the en-*.jar
files, are those strings hardcoded???
Kairo: This dialog is not part of Mozilla, it is stored as an .xul file on
mozilla.org. To properly localize it, the server would have to check the
Accept-Language header, and in any case, that would be a separate bug.
biesi:
thanks for that info... in this case I doubt we have a good way to localize it
(consistent to the selected locale) anyway...
Is there any reason why we can't take the dialog off the server and make it part
of Mozilla so we can solve the problems with it? As far as I remember when I was
working on another issue with this dialog a few months ago, ftang said that it
would now be possible to do that.
If the dialog will become part of mozilla, remember to fix bug 183045 and bug
183046 which are dependent on this one, IMHO... ;) 
*** Bug 190671 has been marked as a duplicate of this bug. ***
Can reproduce on trunk 2003020708 WinME.  Change OS to All?

This bug is annoying.  The workaround is that you can click "X" and the box will
go away.
Not All, this is only on windows; setting to the earliest windows on which it's
been reported.
Keywords: polish
OS: Windows XP → Windows 98
*** Bug 192785 has been marked as a duplicate of this bug. ***
*** Bug 193363 has been marked as a duplicate of this bug. ***
*** Bug 194371 has been marked as a duplicate of this bug. ***
*** Bug 194471 has been marked as a duplicate of this bug. ***
While a search of bugzilla for outstanding "cancel button" bugs returns 33
results, this isn't one of them. I bet there would be a lot less duplicates here
if it were.
Summary: Cancel Inoperative on Font Download dialog → Cancel button inoperative on Font Download dialog
[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.3a) Gecko/20021212]

Confirmed (on Win95); (I had it when viewing an email source (Ctrl-U)).

Severity should be changed from 'Normal' to 'Minor';
as the workaround is explained in comment 22.

I suggest to update the Summary to 'Cancel button inoperative on "Download Font"
dialog/window'

Answer to comment 15 is "Scripts may not close windows that were not opened by
script." !

*** Bug 194668 has been marked as a duplicate of this bug. ***
Verified on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030210

IBM ThinkPad R32 - Windows XP Pro SP1 / 1GB RAM / 15GB Hard Drive

This is a screenshot of the dialog box that pops up on my screen whenever I go
to http://www.megatokyo.com.  I do not have the Japanese language pack
installed on my machine.  The cancel button does not work.  Even when I cancel
and close the box, it pops up the next time Mozilla is restarted and I go to
the site.
Reply to comment 32:

What you describe is the expected behaviour, let alone the Cancel button issue
(which is the current bug).

It _is_ however a bug that the stupid thing keeps pupping up again and again if
you don't want it. (I too experience the annoyance of it three times a week at
megatokyo.) See bug 86525
man, we should just move this dialog into chrome. There are a few reasons to do
that, but I can see no reason not to do it.
Some arguments for moving it to chrome:
1) This bug would be fixed
2) The dialog can be localized correctly
3) The dialog doesn't break if XUL changes some time
4) It's easier to make patches against the normal tree
5) If someone changes the dialog, he doesn't have to check with multiple
versions if it really works
Reply to comment 34:

You say it yourself: it' bug 86525, not the current bug :->

>Kairo: This dialog is not part of Mozilla, it is stored as an .xul file on
>mozilla.org. To properly localize it, the server would have to check the
>Accept-Language header, and in any case, that would be a separate bug.

Incorrect. To localize it corretly, a url in the language pack have to point to
the localized xul. 


ftang:
I still think it would be better just to move the dialog into chrome, it would
fix this bug, and make it much easier to fix also L10n, as well as probably some
other issues.
i18n triage team: nsbeta1+/adt2
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
*** Bug 198352 has been marked as a duplicate of this bug. ***
*** Bug 201873 has been marked as a duplicate of this bug. ***
*** Bug 201874 has been marked as a duplicate of this bug. ***
*** Bug 203743 has been marked as a duplicate of this bug. ***
*** Bug 204900 has been marked as a duplicate of this bug. ***
*** Bug 204909 has been marked as a duplicate of this bug. ***
I would nominate this as topembed, unfortunately, it has nothing to do with
embedding...Simon, if possible we should get this fixed.
polish was exactly the term I was going to use to describe this. 

There are several issues here:

1. is the fundamental flaw, which I just realized. windows should be able to 
*close* themselves. we can make an exception.. can't we? as I recall from my 
limited understanding of javascript window.self() exists, correct or the "self" 
property exists anyway..I made the function up. Internet Explorer *used to* ask,

"This window is trying to close? Do you want to allow it?"

and then as more scripts started using it.. you get the idea.

We could add this dialog + a checkbox (don't show this again.)

I think this is a much bigger issue of whether to "trust" Mozilla.org (In 
Netscape, netscape hosts things like fonts and plugins to make it easier for 
users, and to drive traffic to their portal.) The concept of security zones, 
though developer's abuse it, is generally sound. We could also have a pref, and 
we should, to turn off such font dialogs entirely. (Mozilla is a developer 
build, it's OK.) Maybe one day, we can just have a message in the java console.

I also have a non-altruistic question:
does this security setting affect the website compatibility issue that windows 
close when a password manager is up:
http://bugzilla.mozilla.org/show_bug.cgi?id=189149 

What?? I don't think this is a javascript issue.

And, just to let you know, windows that javascript opens don't need to ask to
close themselves.

-M
Ah, nevermind my previous comment. I just read the previous comments. I was a
bit hasty -- I've been on the computer a bit too long today.

And -- wait... perhaps this question doesn't belong here (?) but shouldn't
windows that the browser opens be able to close themselves without permission?

-M
A possible quick-and-dirty temporary solution would be to simply remove the
"Cancel" button. In that way, the user would be forced to close the window by
using the OS.  Granted it's not pretty, but it's prettier than having a
non-working button. The non-working button gives really a bad impression to the
user.
I don't think we can just remove the Cancel button... It would then disappear
also in mozilla versions in which that button worked, because that dialog is
loaded from a webpage.

As for the dialog about closing the window, it would look pretty stupid if
Mozilla asked for confirmation if the user clicked cancel here, because it looks
like a browser dialog, not like a window from a webpage.


My opinion is that the best fix would be to not load that dialog from a webpage
but ship it with Mozilla. 
can somebody post the location of this external XUL file, and the code location
where it is called? I would like to have a look, though i´m doubting I could do
anything about it.
It´s very annoying to see this box, and I´m seeing it regularly on far eastern
IT-sites, even from big companies.
It is nice for Netscape to get a statistic about font needs,
but that is also an issue relating to my privacy.


Hermann: that's more bug 86525; this one is restricted solely to the cancel
button not working.
The box would be half as annoying, if the Cancel button would be working,
and if Kairo´s suggestion in comment #35 will be honoured, there will be ways
open to fix this bug, and bug 86525.
Comment #14 gives a lucid explanation why this button isn´t working,
but why is this bug then restricted to Windows only, is the closing of windows
on Unix more secure?
Hermann: Non-Windows platforms do not show this dialog.
1. How does the font dialog pop up?

I don't think us lay users understand until we see the external XUL file. (what 
method calls it up?)

How does it get around the popup blocking? 

and why can't we put an option under scripting and windows allowing this 
windows to close themselves no matter what?
(and have it checked by default)..

What is the security risk.. I suppose a mallicious script could cause data 
loss.. but its unlikely, since such a simple creation would likely be VB and 
run client-side..
two other points:
1. why can't we test to see whether user is running a mozilla version that 
supports this.. aka. good model of browser detection.
2. do we file a seperate bug "software installation dialogs should be part of 
Mozilla?)

Aside: the  plugin dialog is (if you set the preference not to use the plugin 
finder..)
Sam, you can find the link to the external XUL-File here:

Bug 139248 Issues with the font download dialog in mailnews
http://bugzilla.mozilla.org/show_bug.cgi?id=139248#c5
http://www.mozilla.org/projects/intl/fonts/win/en/package_zh-CN.xul

In mailnews, this dialog now doesn´t pop-up, because non-korean speaking people
aren´t interested in reading korean spam.
>1. How does the font dialog pop up?

http://lxr.mozilla.org/seamonkey/source/xpfe/components/intl/nsFontPackageHandler.cpp#133
the font code (?) makes a call to the window watcher.

>I don't think us lay users understand until we see the external XUL file. (what 
>method calls it up?)

Someone mentioned the url already...

>How does it get around the popup blocking? 

?
Windows opened by Mozilla itself are not really subject to popup blocking...

>and why can't we put an option under scripting and windows allowing this 
>windows to close themselves no matter what?
>(and have it checked by default)..

That should just work and not be a preference.

>What is the security risk.. I suppose a mallicious script could cause data 
>loss.. but its unlikely, since such a simple creation would likely be VB and 
>run client-side..

VB? Visual Basic? Why that? A script doing window.close() makes you loose
session history...

>1. why can't we test to see whether user is running a mozilla version that 
>supports this.. aka. good model of browser detection.

What for? What would that fix? What should be done once the browser is detected?

>2. do we file a seperate bug "software installation dialogs should be part of 
>Mozilla?)

They are.

>Aside: the  plugin dialog is (if you set the preference not to use the plugin 
>finder..)

Indeed.
cc'ing smkatz@yahoo.com so he sees my reply to his questions...
Keywords: topembed
*** Bug 205543 has been marked as a duplicate of this bug. ***
Confirmed on 1.4b [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b)
Gecko/20030507]. Windows 98 SE.
I was trying to make the point that no-one would cause malicious damage by 
trying to close a window.. and if they did, they would be using visual basic 
script kiddies which Mozilla does not support.

Sorry. I'll stop.

I will file a bug for "trusted zone" support and we can make mozilla.org 
trusted. Ot: we can make an exception such as:
if new window is opened and 
navigator.history(-1)
returns the appropriate value of "no session history"
or window is chromed,
then window can close.
BTW, I am on the CC list. No reason to CC me. My main address I can't use 
because of Bugzilla's spam-friendly displays of e-mail addresses.

www.paperlessconscience.com
hint: try a catch-all. (use catch-all + that domain, and you will contact me 
quickly.
topembed-
Keywords: topembed → topembed-
Bug still occurs with Mozilla Firebird 0.6 - cancel button is inoperative.
Can we please stop spamming the bug with confirmations that it's still a bug? 
We know that -- that's why there's a bug report open.

"Bugzilla Etiquette": http://bugzilla.mozilla.org/page.cgi?id=etiquette.html
We need to get this fixed for 1.4
Flags: blocking1.4+
Summary: Cancel button inoperative on Font Download dialog → Cancel button inoperative on "Download Font" dialog/window
This is an regression caused by a security fix which disallow JavaScript file
from external source to call the window.close()


Was there a particular reason this wasn't built into Chrome?  I'm new to all
this, so I may be way off-base, but it seems like it would be much easier and
faster to add this to Chrome than to solve it by adding trusted zone support. 
One could always move this into Chrome to fix it for now and then add trusted
zone support later...
Another option is just to switch the darn thing off, and let people see ??? for
Chinese characters. It's not going to kill them, and it's what happens in every
other browser. This option has the advantage of being really simple - just
return NS_ERROR_ABORT (like in the Mail/News case) in that nsFontPackageHandler
function referenced in comment 59.

Whatever happens, 1.4 cannot ship with things in this state. The current user
experience is an embarrassment.

smontagu: this bug is assigned to you, but you haven't commented on it in six
months. Could you confirm your plans for it for 1.4 final?

Gerv
Also: removing it would fix bug 183044, bug 183045 and bug 183046, and work
around bug 86525. IE manages OK without one of these, so it can't be vital to
the survival of civilisation.

Gerv
gerv: er, that dialog is an exact copy of MSIE's dialog. so it is not quite true
that no other browser has this dialog.
My question in comment 19 (less than five months ago but who's counting?) has
not had an answer. I could produce a patch to move the dialog from a remote
server into chrome, but I'm not going to spend time on it if there is a reason
not to do that.
Is there any good reason to NOT put this in chrome except for the L10N hit?

Does the content need to come from the web?

I don't think so.
biesi: really? I've truly never seen it, and my Windows machine definitely
doesn't have all the fonts it could have. <shrug> Anyway, I stand corrected, but
if it comes to the wire, and the choice is the dialog in the state it's in or no
dialog, I'd go for no dialog. But if we can fix it, that would do - although
there's got to be some way to turn the darn thing off! :-|

Gerv
The purpose of doing browser detection just to clarify is to show a page with a
cancel button if the browser version supports it.

I believe this is a bug in the security patch.. and that someone should rewrite
it so that browser windows that the browser opens (plugin finder, "font
download") what have you, can close themselves.
> Another option is just to switch the darn thing off, and let people see ???
> for Chinese characters. It's not going to kill them, and it's what happens
> in every other browser.

I assume that this still works in IE.
>------- Additional Comment #70 From Gervase Markham  2003-05-22 10:23 -------
>
>Another option is just to switch the darn thing off, and let people see ??? for
>Chinese characters. It's not going to kill them, and it's what happens in every
>other browser. 

Will it 'kill you' if the mozilla see ??? for all "English" characters? It won't
"kill you" neither, right?

Not sure what is your definitation of "every other browser"
Windows based IE and AOL will download the font and install for you when they
hit Chinese page (I think after IE version 5)

>Was there a particular reason this wasn't built into Chrome? 
Because there were people gave me hard time for any mozilla i18n UI changes 2-3
years ago when I add the code.
Also we should be able to control the URL of the font package from the server in
case MS change it. But that could be done by a redirect. 

Please move the code into chorome and point the font package to a redirect url
in www.mozilla.org Thanks
l10n hit is not big. and we already have the localized string for DE/FR/JA/ES
Attached patch First cut at a patch (obsolete) — — Splinter Review 
This still needs work and doesn't include changing the download URL to a
redirect URL. I'll finish it up over the weekend.
Attached patch Patch (obsolete) — — Splinter Review 
This seems to me ready for review. I suggest we focus this bug on moving the
existing dialog into chrome without major changes, and then land fixes for some
of the other open bugs on the dialog (e.g. bug 183045 and bug 183046). Then we
can port the whole lot across to the branch.
Attachment #124106 - Attachment is obsolete: true
Attachment #124246 - Flags: review?(timeless)
Comment on attachment 124246 [details] [diff] [review]
Patch

good enough for a first round

license should be MPL-tri (c) 2003 not NPL-tri, not (c) 2002 nor (c) 1998

something to consider:

const dm =
Components.classes["@mozilla.org/download-manager;1"].
	getService(Components.interfaces.nsIDownloadManager);
const tempDir =
Components.classes["@mozilla.org/file/directory_service;1"].
getService(Components.interfaces.nsIDirectoryServiceProvider).
getFile("TmpD",{});

dm.addDownload(
"http://www.mozilla.org/projects/intl/fonts/win/redirect/package_" + gLangCode
+ ".html",
tempDir,
titleString,
{}	/* aMIMEInfo, not set because it isn't doc'd	*/,
0	/* startTime, not set because it isn't doc'd	*/,
{}	/* webBrowser persist, not set because i can't
	   understand the doc	*/
);
Attachment #124246 - Flags: review?(timeless) → review+
Attachment #124246 - Flags: superreview?(jaggernaut)
Comment on attachment 124246 [details] [diff] [review]
Patch

Since these files are based on NPL sources, I'm not sure if MPL-trilicense or
even NPL-trilicense is right. I've mailed staff@mozilla.org to ask.
*** Bug 207493 has been marked as a duplicate of this bug. ***
*** Bug 207725 has been marked as a duplicate of this bug. ***
*** Bug 207755 has been marked as a duplicate of this bug. ***
*** Bug 207945 has been marked as a duplicate of this bug. ***
do we have a patch to check in??? we could miss 1.4 final train...!
QA Contact: carineh00 → ylong
Simon,
Please add an ETA for when you will be able to land this on the 1.4 branch.  Thanks.
Whiteboard: [adt2] → [adt2][ETA needed]
Comment on attachment 124246 [details] [diff] [review]
Patch

<spring space="spacer">
<spring style="spacer">

These should probably be <spacer flex="1">

<text class="label"> has become <description>

<box orient="vertical|horizontal"> has become <vbox>|<hbox>

+<!-- extracted from package_ja.xul -->

That should probably be fontpackage.xul, or just omitted.

Indentation in the xul and js files could do with some work (alignment of
attributes on second line for xul, consistent spacing around '=' for js). The
hardcoded widths and heights seem kinda ugly. Are they really needed?

Instead of having to keep the language package list in synch in three places
you could make it a comma separated list in the .properties file which you load
and parse from these two places.

I don't like the hardcoded MB values, but the only "fix" for that would be to
place the .properties file on the server and grab it from there, with some kind
of network timeout support to fall back on a back-up .properties file.

This would however allow people to update the MB sizes as the font packages
themselves are updated (iff that ever happens) and to add more packages.

Perhaps in a future revision of this file.

Please clean up the XUL.
Attachment #124246 - Flags: superreview?(jaggernaut) → superreview-
Simon, please use the MPL tri-license. Thanks.
>Instead of having to keep the language package list in synch in three places
>you could make it a comma separated list in the .properties file which you load
>and parse from these two places.

I think that would make the code unnecessarily complicated and the net effect
would be that it would be harder to maintain. Right now in
nsFontPackageService.cpp there are separate status flags for each language
(mJAState, mKOState, etc.). I suppose they could be turned into a bitfield or
something, but one would still have to add redirect URLs on the server and
labels in the properties file for new languages, so what would be the gain.

New patch coming up with the other changes.
Whiteboard: [adt2][ETA needed] → [adt2][ETA 2003/06/04]

        Attachment #124246 -
        Attachment is obsolete: true

        Attachment #124861 -
        Flags: superreview?(jaggernaut)

    
    

    
  


  
Attached the wrong file last time, sorry for spamming.

        Attachment #124861 -
        Attachment is obsolete: true

        Attachment #124862 -
        Flags: superreview?(jaggernaut)

        Attachment #124861 -
        Flags: superreview?(jaggernaut)

    
    

    
  
Comment on attachment 124862 [details] [diff] [review]
Addressed jag's comments and changed license

Could you attach a screen shot of what the dialog looks like?

rs=jag, but get someone to r= these changes (e.g. make sure aren't extraneous
boxes).

        Attachment #124862 -
        Flags: superreview?(jaggernaut) → superreview+

    
    

    
  

      
      
      
      

        Attached image
          
        The dialog on W2K, Modern theme (obsolete)
        — 
      

    


  

    
    

    
  
Just so you know, on W2K at the least, the control panel is actually called
"Regional Options"

-M

    
    

    
  


  

        Attachment #124951 -
        Attachment is obsolete: true

    
    

    
  
>Just so you know, on W2K at the least, the control panel is actually called
>"Regional Options"

Good point, so it is. I'll make that change before checkin. On XP it's called
"Regional and Language Options", but I hope nobody will be too confused by that.

        Attachment #124862 -
        Flags: review?(dmose)
Whiteboard: [adt2][ETA 2003/06/04] → [adt2][ETA 2003/06/09]

    
    

    
  
Comment on attachment 124862 [details] [diff] [review]
Addressed jag's comments and changed license

My XUL / DOM skills are not strong enough that it would be a good idea for me
to be the only reviewer (since jag gave an rs).  Sorry.  :-(

        Attachment #124862 -
        Flags: review?(dmose)

    
    

    
  
Comment on attachment 124862 [details] [diff] [review]
Addressed jag's comments and changed license

timeless, can you re-review?

        Attachment #124862 -
        Flags: review?(timeless)

    
    

    
  
Comment on attachment 124862 [details] [diff] [review]
Addressed jag's comments and changed license

rejecting this patch.

1)

just to confirm, this window only is only seen on win32, right?

2)

+var is_win2k = ((agt.indexOf("windows nt 5.0") !=-1));
+var is_winxp = ((agt.indexOf("windows nt 5.1") !=-1));

this seems a little fragile, what if there is a 5.2?

given how you use it, how about just:

+ if (agt.indexOf("windows nt 5.") !=-1) {

it always makes me nervous to check user agents, as that can be changed with
the general.useragent.override pref, right?

3)

downloadtxt is an atypical id for a button.  consider downloadbutton, to match
your .dtd entity.

+      <hbox align="center">
+	 <button class="dialog"
+		 label="&downloadbutton.label;"
+		 id="downloadtxt"
+		 align="left"
+		 oncommand="download();" />

4)


+	 <button class="dialog"
+		 label="&cancelbutton.label;"
+		 id="cancel"
+		 align="right"
+		 oncommand="cancel();" />

5)

have you gotten any UI feedback on this UI yet?

I think you want:

[download] [cancel]  (with download the default) 

or

[ok]  (*not* cancel) (with ok the default)

having just a cancel button doesn't make any sense to me.

6)

what about accesskeys for the buttons?	we need those.

7)

when you have a cancel button, you should set up the <key> foo, to map escape
to it.	(and enter should map to "download" (or "ok", if just an "ok" button)

8)

do you have a=bobj yet, for the new strings?

9)

I'd like jag to re-review as well, as his xul kungfu is strong.

I'll email him directly

        Attachment #124862 -
        Flags: review?(timeless) → review-

    
    

    
  
sspitzer: yes, that dialog is windows-only.

Don't you get a keybinding for <esc> for free if you use a <dialog>?

    
    

    
  
Re: comment 104
===
5)

have you gotten any UI feedback on this UI yet?

I think you want:

[download] [cancel]  (with download the default) 

or

[ok]  (*not* cancel) (with ok the default)

having just a cancel button doesn't make any sense to me.
===

Just speaking as an observer hoping to see this bug get fixed, I hope this bug
will stay on getting the *existing* dialog/window working, and leave the topic
of whether the UI should be changed for a separate bug report.

    
    

    
  
UI changes.  CC'ing yxia and rcloseirl.

    
    

    
  
I guess the question is, do we wanna do a simple conversion of what we already
have, or do we want to fix it while we're at it. Given the ETA for RC2, I was
thinking we could go with the simple conversion, but perhaps we should fix it
while we're at it (and hope drivers is willing to take the change).

The right thing to do would be to convert this to a <dialog>, which you can tell
which buttons to show/hide, provide your own buttons (to hook up access+D) and
hook actions up to their oncommand, while getting Esc and Enter hook-ups "for free".

For the DIY (Win 2k+) case the "cancel" button label should be "Close", not
"Cancel" (on Mac we'd just omit the button, since there's already a close button
on your dialog).

I could take a stab at doing this conversion if no one else has time.

    
    

    
  
That is indeed the question. I'm not at all a UI guy or a XUL expert, so the
"simple conversion of what we have" is the most I can efficiently contribute to
this bug, hence my suggestion in comment 80 to focus on that and defer other fixes. 

If reviewers and drivers want to get it right at this stage I respect that, but
I'm sure there are plenty of people who could do it faster and better than I.

    
    

    
  
Re: Comment #98 

For the Win2K dialog --

instead of this text:
"You can install it through Control Panel:Regional Setting:Language"

I suggest:
"Use the Regional Options Control Panel to install these components."

The text in the 9x dialogs looks OK.

    
    

    
  
thanks robin.  I've sent mail to simon / jag, I have the cycles to help out.

    
    

    
  
I'll take this bug, but simon's done all the hard work.

I'll just finish it up, hopefully by tonight, so it can make rc2.
Assignee: smontagu → sspitzer

    
    

    
  
I've converted to <dialog> but I'm finding other (small) things that need fixing.

new patch coming soon.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch, not complete yet (obsolete)
        — 
          — Splinter Review
      

    


  
a patch, this includes some changes to force the dialog (so I could test it).

one issue I have is that we are adding these new xul,js,dtd and properties file
on all platforms, instead of just on win32.

    
    

    
  
I think I might have a much simpler, lower risk, fix for 1.4 branch.

working on that now, and we'll leave the other fix for the trunk.

    
    

    
  
> I think I might have a much simpler, lower risk, fix for 1.4 branch.

nevermind.

I'm continuing on with what I have, updated patch coming.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch, more issues fixed (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #125388 -
        Attachment is obsolete: true

    
    

    
  
hoping to land today.
Status: NEW → ASSIGNED
Whiteboard: [adt2][ETA 2003/06/09] → [adt2][ETA 2003/06/11]
Target Milestone: --- → mozilla1.4final

    
    

    
  
+      <description value="&msg1;"/>
+      <description value="&msg2;"/>

why not use a single <description>?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
fix some C++ per bienvenu, and switch to less precision in the size of the
download.

5.X MB is enough, no need for 5.XYZ

        Attachment #125410 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 125410 [details] [diff] [review]
updated patch, more issues fixed

+<!DOCTYPE window SYSTEM "chrome://global/locale/fontpackage.dtd">
+
+<dialog title="&window.title;"

The "window" in the doctype should be dialog...

+	 ondialogcancel="window.close();"
I don't think you need this...

        Attachment #125410 -
        Attachment is obsolete: false

    
    

    
  


  

        Attachment #125410 -
        Attachment is obsolete: true

        Attachment #125412 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        more changes, per biesi (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #125414 -
        Attachment is obsolete: true

    
    

    
  
biesi has reviewed, but I'll also seek some reviews (from jag and simon)

I also need a=i18n and a=adt.

        Attachment #125416 -
        Flags: superreview?(jaggernaut)

        Attachment #125416 -
        Flags: review?(smontagu)

    
    

    
  
Comment on attachment 125414 [details] [diff] [review]
updated patch, per christian's comments

sr=bienvenu

        Attachment #125414 -
        Flags: superreview+

    
    

    
  
Comment on attachment 125416 [details] [diff] [review]
more changes, per biesi

a=asa (on behalf of drivers) for checkin to the 1.4 branch.

        Attachment #125416 -
        Flags: approval1.4+

    
    

    
  
a=adt.  Please land on branch and add fixed1.4 keyword.

    
    

    
  
just waiting for a=i18n and some additional reviews.
Summary: Cancel button inoperative on "Download Font" dialog/window → Cancel button inoperative on "Download Font" dialog

    
    

    
  
Can you please add some l10n comments for file fontpackage.properties? It does
not look very clear to us how to translate that file. For example, 
   +handled_languages=ja, ko, zh-cn, zh-tw
   +name_ja=Japanese Text Display
   +size_ja=5.3 MB
For each language including European languages, how can we and what can we
translate?

    
    

    
  
Also, currently for Japanese, we are using this URL:
  fontDownloadURL=http://www.mozilla.org/projects/intl/fonts/win/ja/package_%1$s.xul

Since you changed it inside the code, as:
+ 
window.open("http://www.mozilla.org/projects/intl/fonts/win/redirect/package_" +
gLangCode + ".html");

I wonder it might not working for JA. Frank?

    
    

    
  
the old url formatting string in the property file point to ui, which need to be
differrent for all language and package combination
the new url hardcoded in js point to a redirect page without ui, which is shared
between localiztion. 

So that should not be  issue. they are accessing different thing

    
    

    
  
thanks, frank,
i checked the url
   http://www.mozilla.org/projects/intl/fonts/win/redirect/package_ja.html
it does download the jamondo.exe file, without ui.

so the patch is fine for l10n, and my understanding is l10n should not touch the
strings:
   handled_languages=ja, ko, zh-cn, zh-tw
   size_xx=5.3 MB
if there is clear l10n comments, we will be very appreciated.


    
    

    
  
Comment on attachment 125416 [details] [diff] [review]
more changes, per biesi

r=smontagu. Thank you SO much for taking this over, Seth.

with respect to l10n, clearly we should add
; Localization Note (handled_languages): DONT_TRANSLATE
but I'm not sure about the size_xx strings. Is MB used as-is in Greek and
Russian, for example?

        Attachment #125416 -
        Flags: review?(smontagu) → review+

    
    

    
  
I believe that MB needs to be translatable. I know KB does.

    
    

    
  
Comment on attachment 125416 [details] [diff] [review]
more changes, per biesi

>+  // aFontPackID is of the form lang:xx or lang:xx-YY
>+  const char *colonPos = strchr(aFontPackID,':');
>+  if (!colonPos || !*(colonPos + 1))
>+    return NS_ERROR_UNEXPECTED;
>+  
>+  // turn (const char *)xx-YY into (PRUnichar *)xx-yy
>+  nsAutoString langCode;
>+  langCode = NS_ConvertASCIItoUCS2(colonPos + 1);
>+  ToLowerCase(langCode);
>+
>+  // check for xx or xx-yy in handled_languages
>+  // if not handled, return now, don't show the font dialog
>+  nsAutoString handledLangCodes(handledLanguages);
>+  if (handledLangCodes.Find(langCode) == kNotFound)
>+    return NS_OK;  // XXX should be error?
>+

You might want to read up on
http://mozilla.org/projects/xpcom/string-guide.html#Unicode_Conversion

nsAutoString langCode;
CopyASCIItoUCS2(nsDependentCString(colonPos + 1), langCode);
ToLowerCase(langCode);

if (!FindInReadable(langCode, handledLanguages))
  return NS_OK;

This should save you two string copies (and object constructions).

>@@ -121,65 +146,24 @@

>+  nsCOMPtr<nsISupportsString> langID = do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID, &rv);;

Two semi colons

>Index: xpfe/global/resources/content/fontpackage.js
>===================================================================
>+var gLangCode;
>+
>+function onLoad()
>+{
>+  var size = document.getElementById("size");
>+  var downloadbutton = document.getElementById("downloadbutton");

Nit: interCaps?

>+  var install = document.getElementById("install");
>+  var fontpackageBundle = document.getElementById("fontpackageBundle");

>+  // test if win2k (win nt 5.0) or winxp (win nt 5.1)
>+  if (navigator.userAgent.toLowerCase().indexOf("windows nt 5") != -1) 
>+  {
>+    downloadbutton.setAttribute("hidden","true");
>+    size.setAttribute("hidden","true");

Nit: Space after comma?

>+    // if no download button
>+    // set title to "Install Font"
>+    // and set cancel button to "OK"
>+    var downloadfontdialog = document.getElementById("downloadfontdialog");

Nit: interCaps?

>+  var languageTitle = document.getElementById("languageTitle");
>+  if (languageTitle) {

No need for null check, we know the element is gonna be there.

>+  var sizeSpecification = document.getElementById("sizeSpecification");
>+  if (sizeSpecification) {

Same.

>Index: xpfe/global/resources/locale/en-US/fontpackage.properties
>===================================================================
>+handled_languages=ja, ko, zh-cn, zh-tw
>+name_ja=Japanese Text Display
>+name_ko=Korean Text Display
>+name_zh-tw=Traditional Chinese Text Display
>+name_zh-cn=Simplified Chinese Text Display
>+size_ja=5.3 MB
>+size_ko=2.1 MB
>+size_zh-tw=3.9 MB
>+size_zh-cn=4.4 MB
>+cancelButtonNoDownload=OK
>+windowTitleNoDownload=Install Font

Like requested, add instructions on what should and shouldn't be translated.

sr=jag

(Would like to see you use interCaps more, but if this is your style, ...)

        Attachment #125416 -
        Flags: superreview?(jaggernaut) → superreview+

    
    

    
  
I need to test (and retest) that patch [note, it has some debugging foo that
can't be checked in:

-        if (!have)  {
+        if (!have || PR_TRUE)  {

once I test, I'll take that out, and land on trunk and branch.

    
    

    
  
Re comment 135:

For French (for example), MB -> Mo, as KB -> Ko; (Bytes -> octets).


    
    

    
  
fixed on trunk on branch.

I didn't check in that exact last patch, but something very close.

I removed the debugging foo, and fix the .dtd (I missed some interCaps foo).


Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Keywords: fixed1.4
Resolution: --- → FIXED
Whiteboard: [adt2][ETA 2003/06/11] → [adt2][fixed on trunk and branch]

    
    

    
  
the win2k screenshots are not correct.  I'll go make some new ones.

    
    

    
  
Verified it's fixed in 06-12 branch and trunk build on Win98-EN.

I couldn't find a win2000 machine w/o CJK fonts to verify, Teruko could you
please help to verify it, and mark as verified together with change keyword to
verified1.4 after you done? thanks!

    
    

    
  
I don't know if it matters, but language settings are in the "Languages" tab of
"Control Panel" > "Regional and Language Options" in Windows XP. It's a little
different than Win2k.

    
    

    
  
The font download dialog in Win2k is different from the one in Win98.  The
dialog in Win2k does not have the cancel button.  

Yuying, you can just verify this bug.

Jerry Baker, thank you for your comment.  Please log the differnt bug about the
words in the dialog.

    
    

    
  
OK, mark as verified.
Status: RESOLVED → VERIFIED
Keywords: fixed1.4 → verified1.4

    
    

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

    
    

    
  
>Please log the differnt bug about the words in the dialog.

No, please don't :-) It would be a dupe of bug 183044.

    
    

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

    
    

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

    
    

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

    
    

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

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: