Closed Bug 1274596 Opened 4 years ago Closed 3 years ago

Implement the fakepath requirement

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Ms2ger, Assigned: baku, Mentored)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file, 5 obsolete files)

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.)
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
Anne, wdyt, should we implement this?
Flags: needinfo?(annevk)
Whiteboard: btpp-followup-2016-07-05
Given that Chrome, Edge, and Safari implement this I think we should just follow suit. Surprised we haven't hit issues yet.
Flags: needinfo?(annevk)
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)
Whiteboard: btpp-followup-2016-07-05 → [lang=c++] btpp-followup-2016-07-05
Mentor: manishearth
Keywords: good-first-bug
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
Flags: needinfo?(manishearth)
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.
Flags: needinfo?(manishearth) → needinfo?(annevk)
Flags: needinfo?(annevk)
(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!
Priority: -- → P3
Whiteboard: [lang=c++] btpp-followup-2016-07-05 → [lang=c++]
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.
Flags: needinfo?(manishearth)
foo->bar is the same as (*foo).bar in C++. It dereferences the left hand side and accesses a field or method
Flags: needinfo?(manishearth)
where can I find more information about the classes and methods for these functions I'm dealing with?
Flags: needinfo?(manishearth)
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.
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.
Flags: needinfo?(manishearth)
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.
Hi, 

No I'm not working on it. I have started working on other bugs. Thanks!
The bug is yours!
Assignee: nobody → upanchak
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.
Assignee: upanchak → nobody
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?
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.
Assignee: nobody → danbarragan
Also adds a test file under dom/html/test
Attachment #8792767 - Flags: review?(manishearth)
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
Attachment #8792767 - Flags: review?(manishearth)
Attachment #8792767 - Flags: review?(annevk)
Attachment #8792767 - Flags: feedback+
Attachment #8792767 - Flags: feedback?(michael)
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.
Attachment #8792767 - Flags: feedback?(michael) → feedback+
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.
Assignee: danbarragan → nobody
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?
Attachment #8792913 - Flags: review?(amarchesini) → review-
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.
Attachment #8792767 - Flags: review?(annevk) → review?(amarchesini)
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.
Attachment #8792767 - Flags: review?(amarchesini)
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.
Attachment #8792767 - Attachment is obsolete: true
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.
Attached patch bug-1274596-fix (obsolete) — Splinter Review
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 ?
Assignee: nobody → amod.narvekar
Status: NEW → ASSIGNED
Attachment #8812548 - Flags: feedback?(manishearth)
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
Attachment #8812548 - Flags: feedback?(manishearth) → feedback-
Attached patch bug-1274596-fix (obsolete) — Splinter Review
performed the same actions for "path". Also, if you would prefer declare "fakepath" globally, please let me know.
Attachment #8812548 - Attachment is obsolete: true
Attachment #8814453 - Flags: review?(manishearth)
Comment on attachment 8814453 [details] [diff] [review]
bug-1274596-fix

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

I think this is fine. r?baku
Attachment #8814453 - Flags: review?(manishearth) → review?(amarchesini)
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 :)
Attachment #8814453 - Flags: review?(amarchesini) → review-
(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()
Hi Andrea,
I will get back to bug within a day or two. Any documentation for the test would be useful.
I wont be able to find time to work on this bug further. Unassigning myself.
Assignee: amod.narvekar → nobody
Status: ASSIGNED → NEW
Attached patch fakepatch.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8792913 - Attachment is obsolete: true
Attachment #8814453 - Attachment is obsolete: true
Attachment #8826953 - Flags: review?(bugs)
Attached patch fakepatch.patchSplinter Review
test included
Attachment #8826953 - Attachment is obsolete: true
Attachment #8826953 - Flags: review?(bugs)
Attachment #8827071 - Flags: review?(bugs)
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.
Attachment #8827071 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fd2b8d8d767
Implement the fakepath requirement in HTMLInputElement, r=smaug
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c613cebdbb3
Fixing tests for HTMLInputElement.value + fakepath, r=me
https://hg.mozilla.org/mozilla-central/rev/8fd2b8d8d767
https://hg.mozilla.org/mozilla-central/rev/1c613cebdbb3
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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!
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.