Closed Bug 801635 Opened 12 years ago Closed 12 years ago

Disable <input type='file'> on B2G v1

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
firefox21 --- fixed
b2g18 + fixed

People

(Reporter: timdream, Assigned: mounir)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

Patch
4.06 KB, patch
sicking
: review+
sicking
: approval-mozilla-b2g18+
Details | Diff | Splinter Review 
STR:

1. Go to http://people.mozilla.com/~tchien/input-file.html
2. Tab on the input field or the [browser] button

Expected:

Anything that is:

* File selection [1], "pick" web activity [2], or something

Actual:

* Nothing

Note:

We could also consider disable the <input> at step 1.
Andrea, can you take a look?
blocking-basecamp: ? → -
Assignee: nobody → amarchesini
Attached patch patch alpha (obsolete) — — Splinter Review 
I need a feedback about this approach.
Right now we don't have any app handling 'file' activities, but I think this is the right way to fix this issue.
Attachment #672747 - Flags: feedback?(philipp)
(In reply to Andrea Marchesini (:baku) from comment #2)
> Right now we don't have any app handling 'file' activities, but I think this
> is the right way to fix this issue.

I agree that we should use activities for selecting files (e.g. shell it out to the gallery/music/... apps).
Comment on attachment 672747 [details] [diff] [review]
patch alpha

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

::: b2g/chrome/content/shell.js
@@ +438,5 @@
>      var names = { 'content-handler': 'view',
>                    'dial-handler'   : 'dial',
>                    'mail-handler'   : 'new',
> +                  'sms-handler'    : 'new',
> +                  'file-picker'    : 'file' }

I think that the activity name we want here is "pick".

::: b2g/components/FilePicker.js
@@ +49,5 @@
> +               .QueryInterface(Ci.nsILocalFile);
> +  },
> +
> +  /* readonly attribute nsILocalFile file; */
> +  set file(a) { throw "readonly property"; },

You don't need to write these setters. XPConnect will ensure that an exception is thrown as appropriate. This code won't ever be called.
I think that to make that happen with activities, the picker apps will have to return blobs. So shouldn't we block on bug 782766 ?
The general approach here seems good to me. We should however make sure that it's possible to register activity handler for the various types of filepickers that we support in Fennec.

In fennec you can write <input type=file accept="image/*> to see all the apps which can provide an image.

You can also write <input type=file accept="video/*"> and <input type=file accept="audio/*"> to get apps which can provide video and audio files respectively.

This is already hooked up to the nsIFilePicker interface so that we call appendFilter() with the appropriate filterAudio/filterVideo/filterImages flags set as appropriate.

Mounir probably has opinions on what we should put in the WebActivity that is started in response to this.
Depends on: 782766
I agree with Jonas: we should use a 'pick' activity and change the type depending on the "accept" value. No accept, or an invalid value should send a simple activity with { name = "pick", data = {} }. With accept, it could be things like { name = "pick", data = { type = "image/*" } }.
Comment on attachment 672747 [details] [diff] [review]
patch alpha

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

I end up with a very similar patch at my first (and only) try to resolve this last summer. So I feel like this is a good approach
Attachment #672747 - Flags: feedback+
I think this is a dupe of 796384
(In reply to Mounir Lamouri (:mounir) from comment #7)
> I agree with Jonas: we should use a 'pick' activity and change the type
> depending on the "accept" value. No accept, or an invalid value should send
> a simple activity with { name = "pick", data = {} }. With accept, it could
> be things like { name = "pick", data = { type = "image/*" } }.

Just remember that we don't support regexep or any kind of wildcard when matching activities...
> Just remember that we don't support regexep or any kind of wildcard when
> matching activities...

I think we have to support wildcard, otherwise I have to hardcode the types.
Another problem I have is If width/height are missing, the wallpaper app is not part of the picker list.

Then, the result contains a data URI. The nsIFilePicker must return a nsIFile, so probably we can add a filename too in the result object. can we?
(In reply to Andrea Marchesini (:baku) from comment #11)
> > Just remember that we don't support regexep or any kind of wildcard when
> > matching activities...
> 
> I think we have to support wildcard, otherwise I have to hardcode the types.

I don't disagree

> Another problem I have is If width/height are missing, the wallpaper app is
> not part of the picker list.
> 
> Then, the result contains a data URI. The nsIFilePicker must return a
> nsIFile, so probably we can add a filename too in the result object. can we?

What about returning a Blob? Content processes don't have access to random file locations.
Attached patch patch 1 (obsolete) — — Splinter Review 
This patch will work only when 782766 is fixed and when the pick mozAction result contains DOM Blob objects.
Attachment #672747 - Attachment is obsolete: true
Attachment #672747 - Flags: feedback?(philipp)
Attachment #674766 - Flags: review?(fabrice)
Comment on attachment 674766 [details] [diff] [review]
patch 1

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

r- because I have a couple of questions:

- in bug 794407 we will disable activities that are not triggered by a user interaction. Will this work here?
- filePicker.init() takes a parent parameter which is a nsIDOMWindow. You can probably just do |a = new parent.MozActivity(...)| and avoid to manage the IPC roundtrip yourself.

::: b2g/chrome/content/shell.js
@@ +433,5 @@
>      });
>    },
>  
>    receiveMessage: function shell_receiveMessage(message) {
> +    var activities = { 'content-handler': { activity: 'view', response: null },

nit: let activities = ...

::: b2g/components/FilePicker.js
@@ +13,5 @@
> + * In JS, you can actually cheat, because a thrown exception will cause the
> + * CreateInstance call to fail in turn, but not all languages are so lucky.
> + * (Though ANSI C++ provides exceptions, they are verboten in Mozilla code
> + * for portability reasons -- and even when you're building completely
> + * platform-specific code, you can't throw across an XPCOM method boundary.)

I know you copied/pasted, but maybe we don't need this (interesting) prose twice!

@@ +53,5 @@
> +
> +    var ioService = Cc["@mozilla.org/network/io-service;1"]
> +                  .getService(Ci.nsIIOService);
> +
> +    return this.mFileURL = ioService.newFileURI(this.file);

Just use Services.io

::: b2g/installer/package-manifest.in
@@ +446,5 @@
>  @BINPATH@/components/RadioInterfaceLayer.manifest
>  @BINPATH@/components/RadioInterfaceLayer.js
>  @BINPATH@/components/MmsService.manifest
>  @BINPATH@/components/MmsService.js
>  @BINPATH@/components/RILContentHelper.js

You also need to add the b2g components, right?
Attachment #674766 - Flags: review?(fabrice) → review-
Thank for the review.

> - filePicker.init() takes a parent parameter which is a nsIDOMWindow. You
> can probably just do |a = new parent.MozActivity(...)| and avoid to manage
> the IPC roundtrip yourself.

This works.

> - in bug 794407 we will disable activities that are not triggered by a user
> interaction. Will this work here?

Probably not. If we are deciding to disable activities that are not triggered by the user, probably the current implementation is fine.

Other problems with this patch are:

1. it works if the result of the pick activity contains a 'blob'. 782766

2. Bug 805292 - A "pick" activity without type prop, doesn't work
Depends on: 805292
This blocks a crasher which is already a blocker so I'm going to add this bug to the blocker list.
blocking-basecamp: - → +
Andrew, I would love to see this feature added, but this is feature work which doesn't appear to be on the "At-Risk Feature List for MVP" list of feature work approved to go ahead post feature freeze.

This is also a duplicate of bug 796384 which is marked as blocking-basecamp- and doesn't really block bug 804311 which as discussed in the comments could be solved in another way.

Are you sure this should be blocking-basecamp+ or should does it need some extra kind of approval to land?
I'm sorry, I didn't realize this was a new feature.  I will reset it to -.  Should we make 796384 a dupe of this or vice versa?
blocking-basecamp: + → -
Component: Builds → General
I would like this to be re-considered. We are already adding a bunch of features and denying this one is denying the fact that we are making a Web-based OS. Sending anything to a server without this feature is going to be quite hard and painfull.

For what it worth, during MozFest, a few people randomly mentioned the fact that they were not "able to upload pictures on twitter or facebook" because those websites assume that <input type='file'> works.

The fact that iOS wasn't supporting that for v1 isn't an argument against this feature IMO. Otherwise, we should just drop installable app support...
blocking-basecamp: - → ?
Andreas, Chris, Alex - This is feature work. Mounir makes a good case in comment 20. Please provide your input as to whether this is required at this point in the schedule. Will we block on this?
Flags: needinfo?(akeybl)
Chris for a product decision. See comment 20
Flags: needinfo?(akeybl) → needinfo?(clee)
I'm going to move this to non-blocking. We wouldn't hold release if we didn't have this. It's sad because it'll break a lot of sites, but we have other more important requirements.

Andrea: Once you are done with other blockers, feel free to take this though. Just because it's not a blocker doesn't mean that we wouldn't take a fix if we had one.

So this is definitely a lower priority than any blockers, but a higher priority than non-B2G work.
blocking-basecamp: ? → -
Flags: needinfo?(clee)
Priority: -- → P4
(In reply to Jonas Sicking (:sicking) from comment #24)
> I'm going to move this to non-blocking. We wouldn't hold release if we
> didn't have this. It's sad because it'll break a lot of sites, but we have
> other more important requirements.

I wonder how a Web-based operating system can have higher requirements than making the Web works
?

This is sad and disappointing.
Blocks: twitter.com
I have just been bitten by this bug. Could we at least print a warning to logcat or something? The current behavior of silently doing nothing is quite unsatisfying.
Do we have an ETA on this? We are going to have plenty of FirefoxOS hackatons at the end of January and I am sure that we can have some hackatonians write a file picker, provided that there is a way to plug it in.
(In reply to David Rajchenbach Teller [:Yoric] <away until Jan 7th> from comment #28)
> Do we have an ETA on this? We are going to have plenty of FirefoxOS
> hackatons at the end of January and I am sure that we can have some
> hackatonians write a file picker, provided that there is a way to plug it in.

I don't think we have a WebAPI for file system access, so this isn't something that even can be implemented in an HTML app at this moment.
I believe that we can already implement a file picker for https://wiki.mozilla.org/WebAPI/DeviceStorageAPI, can't we? This would already be a useful subset of the feature.
We don't want to hook this up to a filepicker. What we want is to hook this up such that a webactivity is called when the <input type=file> is activated. That is both easier and more powerful.

The patch attached is a start, but needs to be finished.
(In reply to Jonas Sicking (:sicking) from comment #31)
> We don't want to hook this up to a filepicker. What we want is to hook this
> up such that a webactivity is called when the <input type=file> is
> activated. That is both easier and more powerful.

And that is exactly the ETA I am asking. I was simply replying to kairo that we already have technology that will allow third-party developers to build a file picker that lets users browse the Device Storage, which meant that yes, hooking up a webactivity to <input type="file"> was already something useful, without having to extend the underlying platform.
There is one feature I would really like to see in open web applications: the ability to reopen files opened in a previous run, without requiring the user to re-pick the file. While the typical <input type="file"> + the FileReader combo does not allow this, it is my understanding that this is possible for files accessible through the Device Storage API, provided we have the pseudo-path of the file.

In other words, I would like the |picker| web activity to return not only a Blob but also a path, wherever applicable.

So, possibly a return value such as |{data: Blob, path: object}|, where |path| contains all the information that the application may need to reopen the file, or |null| if the file has been opened in a manner that will not let it be reopened without user interaction on the next run.
Please file a separate bugs for things other than implementing <input type=file>
Filed bug 824313 for deciding what the web activity should return.
I'd like to renominate this so that we disable <input type=file> for the time being. If we don't do that, web authors will have to rely on hacks or user-agent sniffing. By disabling it, web authors can use feature detection like |input.type == 'file'|.
tracking-b2g18: --- → ?
I like the idea. For an application, I was preparing to perform feature detection the other way round, by detecting the presence of a "pick" activity, but I believe that's complementary.
Does the crash here still remain?

Given that we have a patch to implement this feature, I'm not actually sure if it's more work to disable <input type=file> or to implement it.

Though implementing it surely carries more risk.

Either way, it's probably worth getting the crash fixed.
blocking-b2g: --- → tef?
> Does the crash here still remain?

No. The crash has been fixed in another bug by another patch.
Currenty input type="file" is disabled.
In what way is <input type=file> disabled? In a way which webpages can detect?
We don't have nsIFilePicker for B2G.
(In reply to Andrea Marchesini (:baku) from comment #41)
> We don't have nsIFilePicker for B2G.

That's nothing the web page can detect, right?
So far, I think only Rik's proposal introduces a technique that can be detected by the web page. An alternative would be to detect whether there is an available activity for "pick", but that sounds complicated and fragile.
blocking-b2g: tef? → shira?
tracking-b2g18: ? → +
If we want to disable <input type='file'> for B2G, writing such a patch would be pretty easy. I would gladly do that.
Assignee: amarchesini → mounir
Component: General → DOM: Core & HTML
Product: Boot2Gecko → Core
Summary: File input is not supported nor disabled → Disable <input type='file'> on B2G v1
Version: unspecified → Trunk
Attached patch Patch — — Splinter Review 
[Approval Request Comment]
Bug caused by (feature/regressing bug #): ...
Developer impact if declined: developers will have to do sniffing or other dirty hack to try to 'guess' if <input type='file'> is working.
Risk to taking this patch (and alternatives if risky): not really risky, the alternative would be to push a real implementation (but riskier).
String or UUID changes made by this patch: none

This patch allows web content to detect that <input type='file'> is disabled. It is quite safe.
Attachment #674766 - Attachment is obsolete: true
Attachment #704132 - Flags: review?(jonas)
Attachment #704132 - Flags: approval-mozilla-b2g18?
Status: NEW → ASSIGNED
Attachment #704132 - Flags: review?(jonas)
Attachment #704132 - Flags: review+
Attachment #704132 - Flags: approval-mozilla-b2g18?
Attachment #704132 - Flags: approval-mozilla-b2g18+
As an app developer, it would be very helpful if missing functionality like this was better documented.  At the Jan 3-4 workshop, we didn't receive any hint that functionality like this was still missing.  (I do appreciate the vast amount of work required to get an OS in user-ready condition; I'm just saying it's difficult to do my work without better documentation.)
Keywords: dev-doc-needed
Blocks: 832923
And a test fix because I got caught by the annoying getBoolPref() behaviour when there is no pref set (it's throwing instead of simply returning null, bug 776424).

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed17c53be925
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f9d00191107
This also caused b2g-mochitest orange on inbound due to layout/reftests/forms/input-file-width-clip-1.html.

https://tbpl.mozilla.org/php/getParsedLog.php?id=18985396&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18986011&tree=Mozilla-Inbound

Rather than backing out for this, I pushed

  https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea59ba8d604

to skip that test on b2g.
Mouir, can you uplift this patch to the b2g18v1_0_0 branch?
Keywords: checkin-needed
blocking-b2g: shira? → ---
I've filed bug 845034 requesting that file uploads be made to work...

Note that the current behavior turns <input type="file"> into a *text input box* and allows you to type in it. This breaks pretty badly on cases such as Wikipedia's image new upload feature where we have a hidden <input type="file"> under a button. Tapping the button just sets focus into the <input> and opens the onscreen keyboard, very confusingly.

If the behavior were more like what iOS used to do -- appear as a file upload control but disable it so it doesn't do anything -- then we could detect this case and not try to show a broken upload control.
FWIW, the previous comment problem has been solved on IRC.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: