Closed Bug 660833 Opened 14 years ago Closed 14 years ago

Forms with <input type='file' multiple> does not accept more than 253 files at once.

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla7

People

(Reporter: christoph.iserlohn, Assigned: bbondy)

References

Details

Attachments

(1 file, 4 obsolete files)

Fix as before but with simplified combo box length code and whitespace fixed
5.69 KB, patch
jimm
: review+
jimm
: checkin+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/534.24 (KHTML, like Gecko) Chrome/11.0.696.71 Safari/534.24 Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 and Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110531 Firefox/7.0a1 If you have a file input and select more than 253 files in the file-chooser dialog the input form remains empty. Reproducible: Always Steps to Reproduce: <!DOCTYPE html> <html> <head> <title></title> </head> <body> <form action='#' method='post' enctype='multipart/form-data'> <input size='40' name='uploads[]' type='file' multiple> </form> </body> </html> Actual Results: The input form remains empty. Expected Results: The input form contains the file-\path-names. It works fine on Firefox 3.6.17 and Firefox 4.0.1 under Ubuntu 11.04 (64-Bit)
Further testing revealed that 253 files are not a hard limit. It also depends on the filename-length. For the 253 files the length was 15 characters for each file.
Version: unspecified → Trunk
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
http://msdn.microsoft.com/en-us/library/ms646839%28v=vs.85%29.aspx has this to say: If the OFN_ALLOWMULTISELECT flag is set and the user selects multiple files, the buffer contains the current directory followed by the file names of the selected files. ... If the buffer is too small, the function returns FALSE and the CommDlgExtendedError function returns FNERR_BUFFERTOOSMALL. In this case, the first two bytes of the lpstrFile buffer contain the required size, in bytes or characters. In other words, this API expects the caller to provide a fixed-size buffer, and then if the filenames the user selects don't all fit in that buffer it will just return an error. As far as I can tell you can then resize your buffer and call the API again and the user has to select the files _again_. We currently use 4096 for the fixed buffer size, which explains the observed behavior. We don't do the "ask the user again" thing though; we probably should... or something. In any case, this is not a DOM issue; this is an issue in the Windows nsIFilePicker implementation, and not a problem on other OSes as comment 0 points out.
Component: DOM → Widget: Win32
QA Contact: general → win32
Status: UNCONFIRMED → NEW
Ever confirmed: true
http://support.microsoft.com/kb/131462 seems to be the way to make this work "sanely" on Windows. This is probably the worst API I've dealt with recently. :(
(In reply to comment #3) > http://support.microsoft.com/kb/131462 seems to be the way to make this work > "sanely" on Windows. This is probably the worst API I've dealt with > recently. :( http://msdn.microsoft.com/en-us/library/bb776913%28v=vs.85%29.aspx We should investigate using the newer selection dialogs. The dialog we currently use was superseded in Vista. I'd guess the new common item dialogs support all the same features. (I think there's a widget bug on this.)
Assignee: nobody → netzen
I tried searching via Google, via bugzilla, and via scanning list of windows widget bugs, but couldn't find the other bug Jim mentioned. Should I do both the fix and the new dialogs in 2 separate patches on this ticket? Or post a new ticket for the new UI and do the posted task in this ticket?
One bug per problem s probably better.
Blocks: 661991
Regarding the new Common Item Dialogs in Vista and later I added a new ticket here: https://bugzilla.mozilla.org/show_bug.cgi?id=661991 A patch is coming shortly for the problem reported in this ticket for the GetOpenFileNameW calls.
No longer blocks: 661991
Added a hook to the file picker dialog to resize the buffer needed as one selects more files than the current buffer contains.
Attachment #537286 - Flags: review?(jmathies)
>+static void SetFileDialogComboBoxMaxLen(HWND parentHWND, WPARAM len) >+{ >+ const int CONTROL_ID_OF_DROPDOWN = 0x47C; >+ HWND hLast = NULL; >+ do { >+ hLast = FindWindowEx(parentHWND, hLast, NULL, NULL ); >+ if(hLast && ::GetDlgCtrlID(hLast) == CONTROL_ID_OF_DROPDOWN) { >+ SendMessage(hLast, CB_LIMITTEXT, len, 0); >+ break; >+ } >+ } while(hLast != NULL); >+} Wouldn't SendDlgItemMessage be a better choice here? Also, does the control ID vary between versions of Windows, or is it always 0x47C?
This new patch which will do the same as the last patch, but also not assume a specific control ID.
Attachment #537286 - Attachment is obsolete: true
Attachment #537286 - Flags: review?(jmathies)
Attachment #537297 - Flags: review?(jmathies)
I took out the dialog ID and just look for a ComboBox in general now so couldn't use SendDlgItemMessage, but thanks for the suggestion Kai. Please feel free to review the new patch.
Just to clarify, I meant please feel free to give feedback on the patch :).
Like the last patches, has a hook to resize the buffer needed as one selects more files than the current buffer contains. Also like the old patch sets the combo box max length appropriately. It will now (as of this patch) also allocate FILE_BUFFER_SIZE more bytes than is needed so that if the user selects a file and holds down shift and down to select additional items, we will not continuously reallocate
Attachment #537297 - Attachment is obsolete: true
Attachment #537297 - Flags: review?(jmathies)
Attachment #537343 - Flags: review?(jmathies)
Comment on attachment 537343 [details] [diff] [review] Dynamically allocates a buffer big enough to hold the size of multiple selected files. >+static void SetFileDialogComboBoxMaxLen(HWND parentHWND, WPARAM len) I just played around a little with the open file dialog, and it looks like the combo box text field is not tied in any way to the return buffer and is just cosmetic. In other words, it's possible for the textbox's maximum length to be large enough to accommodate everything even if the return buffer is not. And if the textbox is not large enough, GetOpenFileName will still succeed and we will still get the full file list despite the selection being too large for the box. So increasing the maximum length of the combo textbox is optional and does not affect the core function. As for whether we should increase the limit, in WinXP, the maximum length of the text box is quite large (quite possibly unbounded; in my test I exceeded 37K TCHARS--which is higher than the default limit of 30K TCHARS--and still did not hit the limit). But as the length of text grew, there started to be noticeable lags in the updating of this textbox, which might be why the limit was cut down to less than 6K TCHARS in Win7. When the user has that many files selected, I don't think that text box is going to be of much use to the user! So given the uselessness of the textbox with that many files selected and given the lag associated with updating it, I think it's best that we don't tinker with that limit. And if we do decide to increase the limit, why not just set it to 0 (make it effectively unbounded) instead of manually adjusting it upwards with each selection? >+ UINT cbLength = 0; If you want to use Windows naming conventions ("Hungarian notation"), that should be cch, not cb (the sample in the KB article is wrong). cch means "count of characters" and cb means "count of bytes". Since we are dealing with strings, cch is the correct prefix (this is true even when dealing with 8-bit strings). That having been said, much of Mozilla's Windows code uses Mozilla's own style conventions (i.e., no need to use "Hungarian" at all). > It will now (as of this patch) also allocate FILE_BUFFER_SIZE more bytes > than is needed so that if the user selects a file and holds down shift and > down to select additional items, we will not continuously reallocate If, in one operation, I shift-select a bunch of files, does it send a notification for each item that was selected, or just a single notification for the whole shebang? I think it's probably the latter (the former wouldn't make much sense), but if MSFT is crazy enough to use the former, then maybe a doubling scheme would be more appropriate.
Thanks for the comments Kai. > the maximum length of the text box is quite large (quite possibly unbounded; in my test I exceeded 37K... For me on Win7 w/ FF4 there is a max cap at 260. Note that when pasting text it bypasses the limit > increasing the maximum length of the combo textbox is optional and does not affect the core function. Agree that it is optional, as this bug description was only for allowing more selections, not being able to type them. If the reviewer does not want this, I can take it out. > If, in one operation, I shift-select a bunch of files, does it send a notification for each item... Yes it will send a new notification for each additional item that is selected. This is why I decided to allocate more than is needed, otherwise we would have to reallocate on each selection change when holding shift + down arrow. > then maybe a doubling scheme would be more appropriate. I considered a doubling as well, but thought allocating a constant amount so that a reallocation is only needed (and not using too much memory) every let's say ~100 or so selection changes. Again will do as reviewer requests. > If you want to use Windows naming conventions ... I'll remove this on my next update of the patch after the reviewer reviews. Thanks again for your comments Kai, much appreciated.
Comment on attachment 537343 [details] [diff] [review] Dynamically allocates a buffer big enough to hold the size of multiple selected files. Review of attachment 537343 [details] [diff] [review]: ----------------------------------------------------------------- r-'ing for now so I can take a look at the updates. No big issues though. As far as the combo box is concerned, increasing that size as well seems fine. ::: widget/src/windows/nsFilePicker.cpp @@ +116,5 @@ > + SetFileDialogComboBoxMaxLen(GetParent(hwnd), FILE_BUFFER_SIZE); > + break; > + case WM_NOTIFY: > + { > + LPOFNOTIFY lpofn = (LPOFNOTIFYW) lParam; LPOFNOTIFYW lpofn = .. Projects use different default character widths so always be explicit in your use. @@ +123,5 @@ > + if (lpofn->hdr.code == CDN_SELCHANGE) { > + HWND parentHWND = GetParent(hwnd); > + > + // Get the required size for the selected files buffer > + UINT cbLength = 0; nit - as Kai points out we don't use this notation. There's no harm in it and you'll find it around in the source, but generally try to use something readable like 'newBufLength'. https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide @@ +124,5 @@ > + HWND parentHWND = GetParent(hwnd); > + > + // Get the required size for the selected files buffer > + UINT cbLength = 0; > + int bufferSize = CommDlg_OpenSave_GetSpec(parentHWND, NULL, 0); Use the W variant here as well and with similar calls. @@ +135,5 @@ > + // multiple files, the buffer contains the current directory > + // followed by the file names of the selected files. So > + // make room for the directory > + bufferSize = CommDlg_OpenSave_GetFolderPath(parentHWND, NULL, 0); > + if(bufferSize >= 0) Either check the flag here (in the event someone makes use of FilePickerHook in some other case) or clear up the comment explaining FilePickerHook is specific to the OFN_ALLOWMULTISELECT case. (I'd go with the comment clean up.)
Attachment #537343 - Flags: review?(jmathies) → review-
Thanks for the feedback Jim, I'll make these changes and submit a new patch.
Updated patch with Jim's comments. Thanks for the suggestions.
Attachment #537343 - Attachment is obsolete: true
Attachment #541276 - Flags: review?(jmathies)
As I briefly mentioned in comment 14, sending CB_LIMITTEXT with a length of zero makes the input box effectively unbounded; set it once during the dialog init and you won't have to bother with it again.
I'll submit a new patch tonight Jim for re-review with the simplified CB_LIMITTEXT code as Kai mentioned.
(In reply to comment #20) > I'll submit a new patch tonight Jim for re-review with the simplified > CB_LIMITTEXT code as Kai mentioned. (some tabs snuck into that comment update as well.)
Noticed will fix that too.
Attachment #541276 - Attachment is obsolete: true
Attachment #541276 - Flags: review?(jmathies)
Attachment #541369 - Flags: review?(jmathies)
Also forgot to mention that the old patch was setting the wrong combo box with class "ComboBox" instead of "ComboBoxEx32" which is the one we wanted to set. So this new patch actually works for the max text length.
Comment on attachment 541369 [details] [diff] [review] Fix as before but with simplified combo box length code and whitespace fixed Review of attachment 541369 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/src/windows/nsFilePicker.cpp @@ +111,5 @@ > + HWND comboBox = FindWindowEx(GetParent(hwnd), NULL, > + L"ComboBoxEx32", NULL ); > + if(comboBox) > + SendMessage(comboBox, CB_LIMITTEXT, 0, 0); > + } According to msdn, the default limit 30K characters. Do we even need this?
Attachment #541369 - Flags: review?(jmathies) → review+
That's what MSDN says, but for me on win7 x64 it is a 260 character limit, so I think we should keep it.
Attachment #541369 - Flags: checkin?(jmathies)
Comment on attachment 541369 [details] [diff] [review] Fix as before but with simplified combo box length code and whitespace fixed http://hg.mozilla.org/integration/mozilla-inbound/rev/84533f5e86bf
Attachment #541369 - Flags: checkin?(jmathies) → checkin+
http://hg.mozilla.org/mozilla-central/rev/84533f5e86bf
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 668663
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 3
Status: RESOLVED → VERIFIED
This fix for Vista and Windows 7 has to be undone. It will be fixed once we do the new Common File Dialogs in Vista+ from Bug 661991. The fix will remain in place for XP and below though. See 684506 Comment 5 for details. All work and other details being done in Bug 684506.
Depends on: 692469
Depends on: 702743
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: