Abdulrahman Alqabandi Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

Check attached PoC.

Works only on nightly builds. This was tested on 51.0a1 (2016-08-16)


Actual results:

The folder upload dialogue is not descriptive enough, which could lead to some users being confused what it does exactly.

On Windows 8.1:
Title of folder upload is 'File upload' and the confirmation button is 'Select Folder', nothing that will indicate multiple files will be uploaded, or that a folder will be uploaded.

On Ubuntu:
Title of the dialogue is 'File upload' and the confirmation button is 'Open', again, nothing that indicates you're uploading an entire folder and its files.


Expected results:

The title should be changed to 'Folder upload' and/or the confirmation button should be changed to 'Upload folder'

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 8 years ago

Just a small change to the test to make it work in Chrome too.

	Abdulrahman Alqabandi Reporter
	Comment 2 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #1)
> Created attachment 8781920 [details]
> test which works in Chrome too
> 
> Just a small change to the test to make it work in Chrome too.

On chrome it does indicate you are uploading a folder. It says 'Select folder to upload' this is on Windows OS, is something else showing for other OS?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 3 • 8 years ago

(In reply to Abdulrahman Alqabandi[test] from comment #0)

> On Windows 8.1:
> Title of folder upload is 'File upload' and the confirmation button is
> 'Select Folder', nothing that will indicate multiple files will be uploaded,
> or that a folder will be uploaded.
How is File _upload_ and Select _folder_ not indicating multiple files will be uploaded?


> Expected results:
> 
> The title should be changed to 'Folder upload' and/or the confirmation
> button should be changed to 'Upload folder'

Yeah, this could be improved. Looks like Chrome uses "Upload" as the button on linux, which isn't any better, but
it has "Select folder to upload" as the title.

(not that anyone ever reads the title of filepicker dialog)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 8 years ago

So, need to add something to http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/dom/locales/en-US/chrome/layout/HtmlForm.properties#8 
and then http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/dom/html/HTMLInputElement.cpp#941 should select the title based on the type of the picker.

	Abdulrahman Alqabandi Reporter
	Comment 5 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Abdulrahman Alqabandi[test] from comment #0)
> 
> > On Windows 8.1:
> > Title of folder upload is 'File upload' and the confirmation button is
> > 'Select Folder', nothing that will indicate multiple files will be uploaded,
> > or that a folder will be uploaded.
> How is File _upload_ and Select _folder_ not indicating multiple files will
> be uploaded?
> 
> 
> > Expected results:
> > 
> > The title should be changed to 'Folder upload' and/or the confirmation
> > button should be changed to 'Upload folder'
> 
> Yeah, this could be improved. Looks like Chrome uses "Upload" as the button
> on linux, which isn't any better, but
> it has "Select folder to upload" as the title.
> 
> (not that anyone ever reads the title of filepicker dialog)

Assumption is that non advanced users can be fooled if it does not strictly indicate a folder is being uploaded.

	Abdulrahman Alqabandi Reporter
	Comment 6 • 8 years ago

Google bug report on this: https://bugs.chromium.org/p/chromium/issues/detail?id=252888

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 8 years ago

Change the title to "Select Folder to Upload".

We're a bit inconsistent with using capital letters in titles, so I ended up using what Chrome has.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 8 years ago

We may want to tweak also the button label, especially because OSX doesn't seem to show any title, I was told.

For gtk, http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/widget/gtk/nsFilePicker.cpp#386

If I read some documentation right, getting NSOpenPanel* op and then [op setPrompt:@"<some value>"];
should work there. Is that right mstange?

On Windows we could use SetOkButtonLabel for IFileDialog (Vista+), but it is unclear what to do in XP.
jimm?
Though, the title is quite clear, so perhaps not dealing with XP is fine.

	Abdulrahman Alqabandi Reporter
	Comment 9 • 8 years ago

Since you are working on this, do you mind checking out Bug 1295922, if its not too much.

	Markus Stange [:mstange]
	Comment 10 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #8)
> If I read some documentation right, getting NSOpenPanel* op and then [op
> setPrompt:@"<some value>"];
> should work there. Is that right mstange?

That is correct. See also the discussion in bug 1207114.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 8 years ago

This should match what Chrome does. At least on linux is uses default button labels for file picking, but 'Upload' for directories.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 14 • 8 years ago

dveditz, are you ok with the approach I'm proposing here:
When directory picking is used, use "Select Folder to Upload" as the title of the filepicker where possible and use "Upload" and the button label where possible.

I was told OSX may not have the title at all at least in some versions, and changing the button label in WindowsXP isn't apparently quite trivial.

That is close to what Chrome does.

	Daniel Veditz [:dveditz]
	Comment 16 • 8 years ago

Wow, the usability here is pretty horrible. On OS X (10.11) there's no titlebar and it's not at all clear you're picking a whole directory to be uploaded. I'm not sure changing the positive button from "Open" to "Upload" (as in Chrome) gets that across. It's not any better in Chrome or Safari (though this example doesn't fully work in Safari -- the "Choose" button stays disabled).

This will be new and unexpected functionality for most users which makes it even easier to abuse. It might even be powerful enough to be worth a second confirmation dialog ("you have chosen to upload the following files... OK/Cancel"). Even if the user knows they're picking a folder for upload they might appreciate the chance to double-check the folder only contained the files they thought it did. Also, in testing at one point I double-clicked a folder thinking I was descending into it (to pick a sub-folder) and instead it submitted the contents--oops!

(In reply to Olli Pettay [:smaug] from comment #14)
> dveditz, are you ok with the approach I'm proposing here:
> When directory picking is used, use "Select Folder to Upload" as the title
> of the filepicker where possible and use "Upload" and the button label where
> possible.

That's better than what we have now, but I'm still unhappy with this functionality (in general, in all the browsers I've tested).

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 17 • 8 years ago

(In reply to Daniel Veditz [:dveditz] from comment #16)
> Wow, the usability here is pretty horrible. On OS X (10.11) there's no
> titlebar and it's not at all clear you're picking a whole directory to be
> uploaded. I'm not sure changing the positive button from "Open" to "Upload"
> (as in Chrome) gets that across. It's not any better in Chrome or Safari
> (though this example doesn't fully work in Safari -- the "Choose" button
> stays disabled).
Safari doesn't support webkitdirectory.

I should test Edge too since it does support webkitdirectory.



> This will be new and unexpected functionality for most users which makes it
> even easier to abuse.
Well, for Firefox users, but Chrome has had webkitdirectory for ages, and also Edge for a year or so.


> It might even be powerful enough to be worth a second
> confirmation dialog ("you have chosen to upload the following files...
> OK/Cancel"). 
I can see point here, but second confirmation dialog would be also rather ux hostile, IMO
I guess we would need totally new directory picker showing what all will be uploaded.

> 
> That's better than what we have now, but I'm still unhappy with this
> functionality (in general, in all the browsers I've tested).
dveditz, are you worried enough about this, meaning that we would postpone releasing this API for couple of Firefox releases to get some more complicated, yet safer UI, or do you think we can release with similar to Chrome's UI for now?

	Abdulrahman Alqabandi Reporter
	Comment 18 • 8 years ago

On Edge both the title and the confirmation button are 'Select Folder'

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 19 • 8 years ago

Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

baku for dom and ipc
mstange for OSX
jimm for Windows
karlt for Gtk

	Markus Stange [:mstange]
	Comment 20 • 8 years ago

Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

Review of attachment 8782155 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on nsFilePicker.mm

	Andrea Marchesini [:baku]
	Comment 21 • 8 years ago

Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

Review of attachment 8782155 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIFilePicker.idl
@@ +189,5 @@
> +   * If set to non-empty string, the nsIFilePicker implementation
> +   * may use okButtonLabel as the label for the button the user uses to accept
> +   * file selection.
> +   */
> +  attribute AString okButtonLabel;

don't you have to change the mockFilePicker as well?

	Karl Tomlinson (:karlt)
	Comment 22 • 8 years ago

Comment on attachment 8782155 [details] [diff] [review]
Use "Upload" as button label for directory picking

>+  const gchar* accept_button = nullptr;

Please remove the unused "= nullptr", to clarify that accept_button is never null.

>+  nsXPIDLCString buttonLabel;
>+  if (!mOkButtonLabel.IsEmpty()) {
>+    buttonLabel.Adopt(ToNewUTF8String(mOkButtonLabel));
>+    accept_button = buttonLabel.get();

nsAutoCString supports .get(), so please use this instead of the unnecessary allocation from nsXPIDLCString/ToNewUTF8String().
Either declare buttonLabel as NS_ConvertUTF16toUTF8 or use Append/CopyUTF16toUTF8().

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 23 • 8 years ago

ok. I was just using the same style as the other parts of the method.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 24 • 8 years ago

(In reply to Andrea Marchesini [:baku] from comment #21)
> ::: widget/nsIFilePicker.idl
> @@ +189,5 @@
> > +   * If set to non-empty string, the nsIFilePicker implementation
> > +   * may use okButtonLabel as the label for the button the user uses to accept
> > +   * file selection.
> > +   */
> > +  attribute AString okButtonLabel;
> 
> don't you have to change the mockFilePicker as well?
I shouldn't need to, since it is implemented in JS and wouldn't do anything with the okButtonLabel.
And the return value of SetOkButtonLabel is ignored.

But I'll test.

	Jim Mathies [:jimm]
	Comment 25 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #8)
> On Windows we could use SetOkButtonLabel for IFileDialog (Vista+), but it is
> unclear what to do in XP.
> jimm?
> Though, the title is quite clear, so perhaps not dealing with XP is fine.

If you want to try this it can be done via the BrowseCallbackProc [1], when the init event is received in the callback we could set the text of the dialog [2].

I don't think it's worth the effort though. :)

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/bb762598(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/ms633546(v=vs.85).aspx

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 26 • 8 years ago

Approval Request Comment
[Feature/regressing bug #]: bug 1288683 
[User impact if declined]: less clear title and button in filepickers
[Describe test coverage new/current, TreeHerder]: Manually tested, since we use OS provided filepickers
[Risks and why]: Should be safe
[String/UUID change made/needed]: new strings added

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 27 • 8 years ago

Comment on attachment 8781935 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1288683 
[User impact if declined]: less clear title and button in filepickers
[Describe test coverage new/current, TreeHerder]: Manually tested, since we use OS provided filepickers
[Risks and why]: Should be safe
[String/UUID change made/needed]: new strings added

	Al Billings [:abillings - ex-MoCo]
	Comment 28 • 8 years ago

As a sec-moderate, this doesn't need sec-approval to land. Clearing and adding Aurora approval.

How far back does this issue go? The sec-approval template questions weren't answered.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 29 • 8 years ago

This is FF51/FF50 issue only, as the the approval question said, by referring to bug 1288683. Sorry if it wasn't clear.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 30 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3d5deb347838fa22ccd7be4ed63ca271b7727c
https://hg.mozilla.org/integration/mozilla-inbound/rev/034773aa3becfa1bf6b35247f9654bf9baa10854

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 8 years ago

(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #27)
> [String/UUID change made/needed]: new strings added

Doesn't this require someone from the l10n team to sign off?

	Francesco Lodolo [:flod]
	Comment 32 • 8 years ago

It's actually up to release drivers to decide when evaluating the approval request (but thanks for asking).

Personally I'm not happy to see this pushed to Aurora this late in the cycle, there's a good chance of not having it translated for several languages. It comes down to evaluating how bad the current situation is, and how confusing it would be to have this string in English. If we really need to, the sooner it lands the better.

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 8 years ago

Any thoughts, Sylvestre/Ritu?

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/ba3d5deb3478
https://hg.mozilla.org/mozilla-central/rev/034773aa3bec

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 35 • 8 years ago

(In reply to Francesco Lodolo [:flod] from comment #32)
> It's actually up to release drivers to decide when evaluating the approval
> request (but thanks for asking).
> 
> Personally I'm not happy to see this pushed to Aurora this late in the
> cycle, there's a good chance of not having it translated for several
> languages. It comes down to evaluating how bad the current situation is, and
> how confusing it would be to have this string in English. If we really need
> to, the sooner it lands the better.

Hi Al, Flod, Olli, in general, I agree that we do not want to break string freeze rules for Aurora/Beta cycle. I think the only potentially un-localized string in this case is "Select Folder to Upload". Right? Is there a string like this that might already be localized and used elsewhere that we can re-use in this case? Flod? Olli?

	Al Billings [:abillings - ex-MoCo]
	Comment 36 • 8 years ago

I don't know anything about string freezes. What I *do* know is that if we take this bug fix on Aurora, we'll never ship this security problem. That's what we'd prefer to do and that's my argument for taking it on Aurora.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 37 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #35)
> (In reply to Francesco Lodolo [:flod] from comment #32)
> > It's actually up to release drivers to decide when evaluating the approval
> > request (but thanks for asking).
> > 
> > Personally I'm not happy to see this pushed to Aurora this late in the
> > cycle, there's a good chance of not having it translated for several
> > languages. It comes down to evaluating how bad the current situation is, and
> > how confusing it would be to have this string in English. If we really need
> > to, the sooner it lands the better.
> 
> Hi Al, Flod, Olli, in general, I agree that we do not want to break string
> freeze rules for Aurora/Beta cycle. I think the only potentially
> un-localized string in this case is "Select Folder to Upload". Right? Is
> there a string like this that might already be localized and used elsewhere
> that we can re-use in this case? Flod? Olli?

There are two strings, one in both patches.

"Select Folder to Upload" and
"Upload"


Couldn't find "Select Folder to Upload" to be used earlier for anything.

	Francesco Lodolo [:flod]
	Comment 38 • 8 years ago

We don't have any of the two strings. As said, given that everyone wants to land this, let's do it quickly.
How much can be disclosed of the reason for uplifting the strings?

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 8 years ago

I'm not going to speak for Al, but it doesn't seem like we need to be overly coy about the reason for the late string change since all affected releases will be fixed once tomorrow's Aurora nightlies go out.

https://hg.mozilla.org/releases/mozilla-aurora/rev/433caaa37ccf
https://hg.mozilla.org/releases/mozilla-aurora/rev/e58e63e362ff

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 40 • 8 years ago

(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #37)
> 
> There are two strings, one in both patches.
> 
> "Select Folder to Upload" and
> "Upload"
> 
> 
> Couldn't find "Select Folder to Upload" to be used earlier for anything.

Ok. Thanks! I am ok with these strings shipping in English when 50 goes to release.

	Al Billings [:abillings - ex-MoCo]
	Comment 41 • 8 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> I'm not going to speak for Al, but it doesn't seem like we need to be overly
> coy about the reason for the late string change since all affected releases
> will be fixed once tomorrow's Aurora nightlies go out.

An excellent point.