:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 8 years ago

https://html.spec.whatwg.org/multipage/forms.html#fakepath-srsly

(I'd be happy to see this wontfixed, of course; would need a spec bug, though.)

	Manish Goregaokar [:manishearth]
	Comment 1 • 8 years ago

If we don't want to WONTFIX this, this is probably a [good-first-bug], https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#1568

	Andrew Overholt [:overholt]
	Comment 2 • 8 years ago

Anne, wdyt, should we implement this?

	Anne (:annevk)
	Comment 3 • 8 years ago

Given that Chrome, Edge, and Safari implement this I think we should just follow suit. Surprised we haven't hit issues yet.

	Manish Goregaokar [:manishearth]
	Comment 4 • 8 years ago

The fix for this is to update https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#2156 (GetValueInternal) so that we return the value appended to "C:\fakepath\" instead of the actual value.

Willing to mentor, but I'd prefer if someone more experienced in Gecko C++/DOM mentored.

(Unsure if I should also mark this as good-first-bug, not sure what the threshold is when it comes to C++ stuff)

	kevin
	Comment 5 • 8 years ago

Hello!

I just started working on bugs here, I was wondering if you can mentor me on this bug please. What other information do I need to know about the problem? are there any inputs? outputs? 

Thank you for your time!
Kevin

	Manish Goregaokar [:manishearth]
	Comment 6 • 8 years ago

Hi!

You need to work on the code in https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#1760 and https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#504

In the case of a file name (https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#1776), we write the leaf file/directory name into aValue. So if the file is /home/foo/bar/baz.txt, we write baz.txt. It seems like we write the entire path for privileged (chrome) code, and should keep it that way.

In the code for regular content (https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#1774), we should take the value produced and append it to C:/fakepath, and write the result to aValue.

I'm not certain if we should do this appending there or in https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#504. I'll find out.

	Nika Layzell [:nika] (ni? for response)
	Comment 7 • 8 years ago

(In reply to Manish Goregaokar [:manishearth] from comment #6)
> I'm not certain if we should do this appending there or in
> https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.
> cpp#504. I'll find out.

I'd append it in GetDOMFileOrDirectoryName(). 

Make sure that the suffix is non-empty before adding C:\fakepath\ to the beginning of the path. Perhaps read the suffix into a nsAutoString on the stack, check if it's empty, and if it isn't write out the prefix followed by the outstring. You might be interested in our internal strings guide (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings) if you're trying to figure out how our strings work.

Let me or Manish know if you have any other questions :)

Thanks!

	kevin
	Comment 8 • 8 years ago

So I am just looking at the code right now and trying to figure out what it is doing... 
Line 504 is getting file or directory
if it is a file we're doing this:
aData.GetAsFile()->GetName(aName) what does this actually mean?

I see the GetAsFile() is a function that is returning a mValue.mFile.Value()
GetName(aName) is a function from file.cpp that assigns a pointer mImpl->GetPath(aPath)

I am particularly interested what the -> is doing... I am used to using the arrow to dereference things in a class with pointers...

Sorry for the newbie question, but this has me really confused.

	Manish Goregaokar [:manishearth]
	Comment 9 • 8 years ago

foo->bar is the same as (*foo).bar in C++. It dereferences the left hand side and accesses a field or method

	kevin
	Comment 10 • 8 years ago

where can I find more information about the classes and methods for these functions I'm dealing with?

	kevin
	Comment 11 • 8 years ago

Sorry I don't know where to start in this bug... Perhaps there is something easier in c++ to work on? I am still a novice in programming.

	Manish Goregaokar [:manishearth]
	Comment 12 • 8 years ago

You can click on the types in DXR and jump to their definitions.

https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#504

You will need to create a temporary string, use it instead of aName in the GetName methods, and then append the raw string "C:\fakepath\" and  the temporary string to aName.

	Manish Goregaokar [:manishearth]
	Comment 13 • 8 years ago

There's some documentations for the string type here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsString

	Umesh Panchaksharaiah
	Comment 14 • 8 years ago

Kevin, are you working in this bug? If not, I can have a look at it.

Manish, if the bug is indeed available, request the bug to be assigned to
me and I can work on it under your mentorship.  Thanks.

	kevin
	Comment 15 • 8 years ago

Hi, 

No I'm not working on it. I have started working on other bugs. Thanks!

	Nika Layzell [:nika] (ni? for response)
	Comment 16 • 8 years ago

The bug is yours!

	Umesh Panchaksharaiah
	Comment 17 • 8 years ago

I assigned this bug to myself yesterday. However, I may not
be able to work on this right away and hence removing myself
to avoid holding up the bug.

	Dan
	Comment 18 • 8 years ago

Hi folks,

Completely new to Mozilla an OSS in general (excited to start!), but I have a solid c++ background. It might take me some time to get acquainted with your build system and processes, but I feel like I can handle this issue.

Can I take it?

	Manish Goregaokar [:manishearth]
	Comment 19 • 8 years ago

Sure, go ahead!

Let me know if you have any questions.

Build instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions, and I've already linked to the code above.

	Dan
	Comment 20 • 8 years ago

Also adds a test file under dom/html/test

	Manish Goregaokar [:manishearth]
	Comment 21 • 8 years ago

Comment on attachment 8792767 [details] [diff] [review]
Append the html spec's fake path to local file names in GetDOMFileOrDirectoryName

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

Overall looks good to me. Not sure if the html file is necessary. 
There already is a manual test in testing/web-platform/tests/html/semantics/forms/the-input-element/file-manual.html.

I myself can't sign off on this, but bouncing it to people who can.


Maybe we can add a mochitest or something? Not sure which test framework lets us mock file uploads.

::: dom/html/HTMLInputElement.cpp
@@ +505,5 @@
>  GetDOMFileOrDirectoryName(const OwningFileOrDirectory& aData,
>                            nsAString& aName)
>  {
> +  /**
> +   *  For historical reasons, file names need to be prefixed with a fake path 

nit: extra whitespace at end of line

	Nika Layzell [:nika] (ni? for response)
	Comment 22 • 8 years ago

Comment on attachment 8792767 [details] [diff] [review]
Append the html spec's fake path to local file names in GetDOMFileOrDirectoryName

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

::: dom/html/HTMLInputElement.cpp
@@ +514,3 @@
>    if (aData.IsFile()) {
> +    // If the file name is not empty, append the file name to the fake path
> +    nsAutoString aTempName;

nit: Only argument variable names are prefixed with an `a`, name this variable `tempName` or perhaps `fileName`

@@ +518,5 @@
> +    if (!aTempName.IsEmpty()) {
> +      aName.AssignASCII(kFakePathName);
> +      aName.Append(aTempName);
> +    }
> +    else {

nit: Please write it like `} else {`

@@ +519,5 @@
> +      aName.AssignASCII(kFakePathName);
> +      aName.Append(aTempName);
> +    }
> +    else {
> +      aName = aTempName;

This feels like a strange assignment, as we know that aTempName.Length() == 0. I would `aName.Truncate();` before the `if (aData.IsFile())` line to make sure that we have an empty string in the error case too, and just leave off this else branch.

::: dom/html/test/bug1274596-fileInputFakePath.html
@@ +11,5 @@
> +				document.getElementById('filePath').textContent = path;
> +			}
> +		</script>
> +	</body>
> +</html>
\ No newline at end of file

You will want to make this into a mochitest here, We don't usually write manual tests. A quick grep around suggests that your best bet would probably be to use MockFilePicker (http://searchfox.org/mozilla-central/source/testing/specialpowers/content/MockFilePicker.jsm) to populate the input element for you. 

Mochitests are a bit tricky to write the first time, and this one is extra annoying because it requires using mock objects and triggering OS events, so I've written up a basic test for you, and added a bunch of comments. I'll attach it as a patch.

	Dan
	Comment 24 • 8 years ago

Hey folks,

I'm truly sorry about this, but I just realized that I previously misinterpreted my employment agreement at my current work and that I'm actually not allowed to make contributions to this project. I won't be able to finish this patch. Thanks for the quick and great mentoring and review. I'll assign this bug back to default.

	Andrea Marchesini [:baku] Assignee
	Comment 25 • 8 years ago

Comment on attachment 8792913 [details] [diff] [review]
Part 2: Automated tests for input fakepath behavior

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

::: dom/html/test/browser_bug1274596.js
@@ +4,5 @@
> +let tempFile;
> +add_task(function*() {
> +  tempFile = FileUtils.getDir("TmpD", [], false);
> +  tempFile.append(FILENAME);
> +  tempFile.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0o644);

createUnique

@@ +37,5 @@
> +  MockFilePicker.returnValue = MockFilePicker.returnOK;
> +  MockFilePicker.displayDirectory = FileUtils.getDir("TmpD", [], false);
> +  MockFilePicker.returnFiles = [tempFile];
> +
> +  try {

why this try/finally?

	Anne (:annevk)
	Comment 26 • 8 years ago

Comment on attachment 8792767 [details] [diff] [review]
Append the html spec's fake path to local file names in GetDOMFileOrDirectoryName

C++ is not mine to review.

	Andrea Marchesini [:baku] Assignee
	Comment 27 • 8 years ago

Comment on attachment 8792767 [details] [diff] [review]
Append the html spec's fake path to local file names in GetDOMFileOrDirectoryName

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

First of all, thanks for this patch!
Can you please apply all the comments written by the previous reviewers?
About the test, add it into dom/base/test/mochitest.ini and write it as Michael suggested.

	Nika Layzell [:nika] (ni? for response)
	Comment 28 • 8 years ago

Comment on attachment 8792767 [details] [diff] [review]
Append the html spec's fake path to local file names in GetDOMFileOrDirectoryName

In comment 24, they state that they cannot contribute due to company policy, so I'm obsoleting this patch.

	Amod [:AMoz]
	Comment 29 • 8 years ago

hi,
I am returning to Bugzilla after a very long time. Seeing no activity on this page past 1.5 months, I would like to take this bug. Using #Comment6 for reference, I would start working on this bug.

	Manish Goregaokar [:manishearth]
	Comment 30 • 8 years ago

Go ahead!

	Amod [:AMoz]
	Comment 31 • 8 years ago

I have edited at one of the two instances  mentioned, as I didnt understand where to make changes at Line 504. Is there any way through which I could test my changes (atleast for compilation errors) for that particular file ?

	Manish Goregaokar [:manishearth]
	Comment 32 • 8 years ago

Comment on attachment 8812548 [details] [diff] [review]
bug-1274596-fix

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

::: dom/html/HTMLInputElement.cpp
@@ +1760,5 @@
>  HTMLInputElement::GetValue(nsAString& aValue, CallerType aCallerType)
>  {
>    GetValueInternal(aValue, aCallerType);
> +  char fakepath[]="C:/fakepath";
> +  aValue.Insert(fakepath,0);

Only if the value is not empty

	Amod [:AMoz]
	Comment 33 • 8 years ago

performed the same actions for "path". Also, if you would prefer declare "fakepath" globally, please let me know.

	Manish Goregaokar [:manishearth]
	Comment 34 • 8 years ago

Comment on attachment 8814453 [details] [diff] [review]
bug-1274596-fix

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

I think this is fine. r?baku

	Andrea Marchesini [:baku] Assignee
	Comment 35 • 8 years ago

Comment on attachment 8814453 [details] [diff] [review]
bug-1274596-fix

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

Thanks for the patch! But this is the wrong place for this change. What you really want to change is:

https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#1788
GetValueInternal(), after GetDOMFileOrDirectoryName(), if the file name is not empty, you should assign the 'fakepath' string to aValue, and then append the file name value.
Then, the correct string is: "C:\fakepath\" (or "C:\\fakepath\\").

And a test is needed :)

	Gaurav Sehgal
	Comment 36 • 7 years ago

(In reply to Andrea Marchesini [:baku] from comment #35)
> Comment on attachment 8814453 [details] [diff] [review]
> bug-1274596-fix
> 
> Review of attachment 8814453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! But this is the wrong place for this change. What you
> really want to change is:
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLInputElement.
> cpp#1788
> GetValueInternal(), after GetDOMFileOrDirectoryName(), if the file name is
> not empty, you should assign the 'fakepath' string to aValue, and then
> append the file name value.
> Then, the correct string is: "C:\fakepath\" (or "C:\\fakepath\\").
> 
> And a test is needed :)

So the change should be made 
1)inside GetNonFileValueInternal() after line 1791 when GetValue(aValue, true) has been executed by   modifying aValue or
2)inside GetValueInternal()

	Amod [:AMoz]
	Comment 37 • 7 years ago

Hi Andrea,
I will get back to bug within a day or two. Any documentation for the test would be useful.

	Amod [:AMoz]
	Comment 38 • 7 years ago

I wont be able to find time to work on this bug further. Unassigning myself.

	Andrea Marchesini [:baku] Assignee
	Comment 40 • 7 years ago

test included

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

Comment on attachment 8827071 [details] [diff] [review]
fakepatch.patch

I had to manually test how this behaves with webkitdirectory. Seems to be like Blink. Perhaps worth to add a test for that too.

	Pulsebot
	Comment 42 • 7 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd2b8d8d767
Implement the fakepath requirement in HTMLInputElement, r=smaug

	Pulsebot
	Comment 43 • 7 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c613cebdbb3
Fixing tests for HTMLInputElement.value + fakepath, r=me

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8fd2b8d8d767
https://hg.mozilla.org/mozilla-central/rev/1c613cebdbb3

	Chris Mills [:cmills]
	Comment 45 • 7 years ago

I have documented this, adding a description to the <input> page and updated browser compat info:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#File_inputs
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#Browser_compatibility

I have also added a note to the Fx53 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM

Let me know if this is OK. Thanks!