"Save as type" drop-down box in "Save Page As" dialog always saves as html only in Win9x

VERIFIED FIXED in mozilla1.3alpha

Status

--
major
VERIFIED FIXED
16 years ago
3 years ago

People

(Reporter: kazhik, Assigned: tetsuroy)

Tracking

({regression})

Trunk
mozilla1.3alpha
x86
Windows 98
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
File type dialog in "Save Page As" dialog shows an invalid option
or shows "Web page, complete" only.

This bug is reproducable from 2002092404-trunk/Win98.
QA Contact: sairuh → petersen

Comment 1

16 years ago
I can't reproduce this problem in the 2002-10-09-08 trunk build. Tested under
Windows ME. Reporter, can you try with a newer trunk build ?

Comment 2

16 years ago
I can reproduce the problem in Win32 trunk build 2002111204.
Summary: File type combobox in "Save Page As" dialog shows an invalid option → "Save as type" drop-down box in "Save Page As" dialog doesn't have "Web Page, HTML only" option and might show invalid option(s)

Comment 3

16 years ago
Created attachment 106080 [details]
screenshot showing the problem

Comment 4

16 years ago
This bug had gone once, since 2002-09-29.
Now it returned after 2002-11-11-04-trunk/Win98SE.

Comment 5

16 years ago
I suspect patch of bug 104934 would cause this bug.
It was checked in on 09/23/2002 12:16, backed out on 09/28/2002 08:52 
and checked in again on 11/08/2002 14:46-14:47.

This bug can be reproduced between 2002092404-trunk and 20020928??-trunk, 
and 2002111104-trunk or later.
It cannot be seen from 2002092908-trunk to 2002110808-trunk.
(Assignee)

Comment 6

16 years ago
reporter: is this Win98/ME only problem?
          Which language OS is this?  

assign bug to myself
Assignee: law → yokoyama

Comment 7

16 years ago
I have only Japanese Windows 98 Second Edition. Therefore I can confirm 
this bug on it.

Per Bugzilla-JP comments, this can be reproduced on Japanese Windows 98, 
not on Japanese Windows XP. There is no report about Japanese Windows Me.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2696 

Comment 8

16 years ago
Same build (2002112108) exhibits this problem on Win98, but not on Win2K, so it
probably is Win9x only.

Comment 9

16 years ago
*** Bug 181608 has been marked as a duplicate of this bug. ***

Comment 10

16 years ago
I see this on Norwegian Win95 with trunk-build 2002111804. The 1.2 branch-build
2002111817 is not affected. 

Duplicate bug 181608 is reported with en-US win98.

Comment 11

16 years ago
Also noted in duplicate bug 181608: The only valid option in the File Types list
is "Web Page, complete" but selecting this saves the page as Text, not HTML,
does not create the _files directory or download any other files.  It behaves
like Save As Text.

Save Page is totally broken. :(
(Assignee)

Comment 12

16 years ago
Created attachment 107420 [details] [diff] [review]
Need to give a proper filter length to WideCharToMultiByte()

Calling WideCharToMultiByte() with -1 for the Wstring will
causes to convert the string only to the first null terminated string.
Filter string is a pointer to a buffer containing pairs 
of null-terminated filter strings

We need to find the correct filter string length before
calling WideCharToMultiByte() and provide the length.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/WinUI/WindowsUserInterface/UserInput/CommonDialogBoxLibrary/CommonDialogBoxReference/CommonDialogBoxStructures/OPENFILENAME.asp


shanjian: can you review?
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3alpha

Comment 13

16 years ago
Comment on attachment 107420 [details] [diff] [review]
Need to give a proper filter length to WideCharToMultiByte()

Fix the nits and mark r=shanjian.
>+    while ((aFileNameW->lpstrFilter[len] != L'\0') 
>+          || (aFileNameW->lpstrFilter[len+1] != L'\0'))
>+    {
>+      len += 1;
>+    }
    while ((aFileNameW->lpstrFilter[len] != L'\0') ||
	   (aFileNameW->lpstrFilter[len+1] != L'\0'))
    {
      ++len;
    }
>+
>+    len = WideCharToMultiByte(CP_ACP, 0,
>+                                  aFileNameW->lpstrFilter,
>+                                  len,
>+                                  filterA,
>+                                  MAX_FILTER_NAME, NULL, NULL);
    len = WideCharToMultiByte(CP_ACP, 0,
			      aFileNameW->lpstrFilter,
			      len,
			      filterA,
			      MAX_FILTER_NAME, NULL, NULL);
>+    filterA[len] = '\0';
>+    filterA[len+1] = '\0';
>     ofnA.lpstrFilter = filterA; 
>   }
>   if (aFileNameW->lpstrCustomFilter)  {
(Assignee)

Comment 14

16 years ago
Created attachment 107642 [details] [diff] [review]
Fixing the nits
Attachment #107420 - Attachment is obsolete: true
(Assignee)

Comment 15

16 years ago
Comment on attachment 107642 [details] [diff] [review]
Fixing the nits

as per shanjian's comment.
Attachment #107642 - Flags: review+

Comment 16

16 years ago
Comment on attachment 107642 [details] [diff] [review]
Fixing the nits

==== Why do we need to check |aFileNameW->lpstrFilter[len+1]!= L'\0'| too? If
we ever hit that expression, that means that [len] was zero, so doesn't that
mean we will be looking at a character beyond the string terminator? Also
|L'\0'| is the same as |0| ... can't we just get away with checking
|while(aFileNameW->lpstrFilter[len])|?


+    while ((aFileNameW->lpstrFilter[len] != L'\0') ||
+	    (aFileNameW->lpstrFilter[len+1] != L'\0'))
+    {
+      ++len;
+    }
Attachment #107642 - Flags: superreview?(kin)

Comment 17

16 years ago
Comment on attachment 107642 [details] [diff] [review]
Fixing the nits

Ok, duh, I read the URL provided by yokoyama above regarding the fact that the 
string is actually a collection of null terminated strings, plus an extra null
at the end of the last string in the buffer, so  disregard my questions above
about the  [len+1] check and the |strlen()|.

So can we still remove the need for |L'\0'|?

sr=kin@netscape.com
Attachment #107642 - Flags: superreview?(kin) → superreview+
(Assignee)

Comment 18

16 years ago
>So can we still remove the need for |L'\0'|?
Thanks and I'll remove the |L'\0'|.
(Assignee)

Comment 19

16 years ago
fix checked into the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 20

16 years ago
We can choose types in "Save as type" drop-down box now.

But it does not work at all.
When type is changed, new type does not be effective. The previous 
saving type is still applyed.

Reproducable on 2002-12-03-04-trunk (Win98SE Japanese).

Comment 21

16 years ago
Confirming comment #20 on win98 spanish with 2002120308, no matter what type
it's selected, the pages is saved as html only. I suggest reopening this bug.

Comment 22

16 years ago
Confirming the behaviour in comment 20 and comment 21, build 2002120308 win95NO.
Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 23

16 years ago
changing the summary to reflect the real problem.
I'll take a look.
Flags: blocking1.3a?
Summary: "Save as type" drop-down box in "Save Page As" dialog doesn't have "Web Page, HTML only" option and might show invalid option(s) → "Save as type" drop-down box in "Save Page As" dialog always saves as html only in Win9x
(Assignee)

Comment 24

16 years ago
Created attachment 108234 [details] [diff] [review]
assign the new filter index

nsFilePicker needs two elements back from the Open/Save dlgbox.
lpstrFile and nFilterIndex.  We missed the nsFilterIndex. 

shanjian: can you review?
Attachment #107642 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #108234 - Flags: review?(shanjian)

Updated

16 years ago
Attachment #108234 - Flags: review?(shanjian) → review+
(Assignee)

Comment 25

16 years ago
Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

kin: can you super review?
Attachment #108234 - Flags: superreview?(kin)

Comment 26

16 years ago
Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

sr=kin@netscape.com
Attachment #108234 - Flags: superreview?(kin) → superreview+
(Assignee)

Comment 27

16 years ago
Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

Impact:
- Open/SaveAs dlgbox
- Windows 9x platforms only
Attachment #108234 - Flags: approval1.3a?
Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

a=asa for checkin to 1.3a
Attachment #108234 - Flags: approval1.3a? → approval1.3a+

Updated

16 years ago
Flags: blocking1.3a? → blocking1.3a+
Keywords: regression
(Assignee)

Comment 29

16 years ago
fix checked in
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Flags: blocking1.3a+ → blocking1.3a?
Resolution: --- → FIXED
Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

>   if (ofnA.lpstrFile) {
>     ConvertAtoW(ofnA.lpstrFile, MAX_PATH, aFileNameW->lpstrFile);
>   }

Hey, I forget the bug number, but is this the code that stops you attaching
multiple files to emails?
(Assignee)

Comment 31

16 years ago
neil: not sure.  I never knew such bug exists.  If the code you point out
      is the cause, then the code only gets executed in Win9x.  Is the bug 
      only in Win9x?
(Assignee)

Comment 32

16 years ago
neil: Never mind my previous question.  You are correct. I have
      a patch ready for review; but I can't find the mail bug you are referring.
      Do you know the bug number?

Seth: do you recall?  
(Assignee)

Comment 33

16 years ago
It's 180333.  I'll attach a patch.

Updated

16 years ago
Flags: blocking1.3a?

Comment 34

16 years ago
Since I don't have access to a Win 98 machine, could the reporter please verify
this with the latest trunk ?
I can verify that this is now fixed on Windows 95/98.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.