953093 - [Fugu][File upload selector] Choosing the system ringtones will go back to the webpage

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Ringtones, defect)

Description

[Bingqing Li]

Environment:
--------------------------
V1.3
Gaia      01e9da49be2cc4bc134eeefc434740d572ec2246
Gecko     http://hg.mozilla.org/releases/mozilla-aurora/rev/af28fe58e263
BuildID   20131224004001
Version   28.0a2

V1.2
Gaia      724d36716bcbbc5cee1af18b94cc328c289d0ccc
Gecko     http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/931864adcf68
BuildID   20131224004001
Version   26.0


Steps to reproduce:
---------------------------
1. Go to http://people.mozilla.com/~tchien/input-file.html
2. Tab on the input field or the [browser] button
3. Click on the system ringtones on the list.


Actual results:
---------------------------
V1.2
1. There is no icon for system ringtones.
2. Go back to the webpage, can not upload file.

V1.3
Go back to the webpage, can not upload file.

Comment 1

[Gregor Wagner [:gwagner]]

David, do you have any input here?

Comment 2

[Preeti Raghunath(:Preeti)]

1.4? as ringtone work is being done in 1.4

Comment 3

[David Flanagan [:djf]]

For picking ringtones, we use a Pick activity with type="ringtone". And for picking alert tones we use type="alerttone".  

It seems to me that the bug is that the file picker allows the users to pick these types. So one approach would be to try to exclude these from the choices that the user sees.  Maybe we could adopt a convention where pick activity handlers that want to be listed in a filepicker include show_in_filepicker:true in their manifest. Then modify the file picker to require that field, and modify all the pick handlers in Gaia to set it to true or false.  

On the other hand, it is a valid pick activity, and I suppose a user might want to upload one of the system ringtones somewhere...  

I don't know why it doesn't work.  Someone could take a look at apps/ringtones/js/pick.js to figure out what is going wrong. A logcat would be a good first place to start.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #2)
> 1.4? as ringtone work is being done in 1.4

This is incorrect. input=file support exists in 1.1 or later & the ringtone picker exists in 1.3. This is broken UX for a core piece of the web, which happens to be used commonly in partner apps. We need to disable this entry point in 1.3, as this is broken UX.

Comment 5

[Jason Smith [:jsmith]]

This is simple to fix. We need to add required: true to the filter type:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.webapp#L96

Comment 6

[Preeti Raghunath(:Preeti)]

1.3+ for blocking in 1.3. This is a broken feature of ringtone support

Comment 7

[Jim Porter (:squib)]

(In reply to Jason Smith [:jsmith] from comment #5)
> This is simple to fix. We need to add required: true to the filter type:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.
> webapp#L96

Can you elaborate on this? Should we add that to the ringtone pick activity?

Comment 8

[Jim Porter (:squib)]

Taking since I know ringtones now (sort of).

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jim Porter (:squib) from comment #7)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > This is simple to fix. We need to add required: true to the filter type:
> > 
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.
> > webapp#L96
> 
> Can you elaborate on this? Should we add that to the ringtone pick activity?

Sure. See https://developer.mozilla.org/en-US/docs/WebAPI/Web_Activities#Activity_handler_description under "required" in "filters." Setting true here will indicate that the data provided from the web activity must exhibit the requirements in the filter. In input=file's case, setting required true will cause it to ignore the set ringtone pick activity, as the data requested here doesn't fit the filter ringtone, alerttone. Note - we had this exact bug in the contacts app & fixed it by using required: true in this exact manner. Here's what the app manifest needs to look like:

{
  "name": "System Ringtones",
  "description": "Gaia Ringtone Picker",
  "type": "certified",
  "role": "system",
  "orientation": "default",
  "developer": {
    "name": "The Gaia Team",
    "url": "https://github.com/mozilla-b2g/gaia"
  },
  "permissions": {
    "settings":{ "access": "readonly" }
  },
  "activities": {
    "pick": {
      "filters": {
        "type": ["ringtone", "alerttone"],
        "required": true
       },
      "disposition": "inline",
      "returnValue": true,
      "href": "/pick.html"
    }
  },
  "locales": {
    "en-US": {
      "name": "System Ringtones",
      "description": "Gaia Ringtone Picker"
    }
  },
  "icons": {
    "60": "/style/icons/Ringtones_60.png",
    "90": "/style/icons/Ringtones_90.png",
    "120": "/style/icons/Ringtones_120.png"
  },
  "default_locale": "en-US"
}

Comment 10

[Jim Porter (:squib)]

Ok, that makes sense. For my own edification, is this a correct summary of the issue? 

1) The <input type="file"> accepts *any* file, thus including ringtones implicitly.
2) Adding `"required": true` signifies that the ringtone app's pick activity shouldn't be listed for picks that accept *any* file, only picks that *specifically* ask for ringtones or alert tones.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Jim Porter (:squib) from comment #10)
> Ok, that makes sense. For my own edification, is this a correct summary of
> the issue? 
> 
> 1) The <input type="file"> accepts *any* file, thus including ringtones
> implicitly.
> 2) Adding `"required": true` signifies that the ringtone app's pick activity
> shouldn't be listed for picks that accept *any* file, only picks that
> *specifically* ask for ringtones or alert tones.

Yup, that's right.

Comment 12

[Jim Porter (:squib)]

Attachment: Fix this

Does this look good? I tested this out on the device and all is well, although it took me a bit to figure out that comment 9 used the wrong syntax for the "required" flag. :/


I guess I could write tests, but all our test infrastructure for the ringtones app is on a branch where we've been rewriting the app, and so we can't land that yet. That makes testing this for 1.3/1.4 somewhat more complex...

Comment 13

[Jason Smith [:jsmith]]

Comment on attachment 8379277 [details] [review]
Fix this

lgtm - I don't have module ownership on this, so I can't grant the r+. I'll let djf give the official seal of approval.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8379277 [details] [review]
Fix this

Looks good to me, and I'm completely comfortable with you landing this without tests.  In addition to verifying that the file upload element works correctly, I assume you've checked that you can still pick a ringtone from the settings app, right?

There was a travis failure in an unrelated calendar test. I've restarted it.

Jason: thanks for the pointer to the solution here. The type property is used with just about every activity, so much so that I think of it like the activity name, and I forget that it can have filters and required:true, etc.

Comment 15

[Jim Porter (:squib)]

Yeah, the ringtone pick activity works just fine from the settings app. I can land this once things are green, yes?

Comment 16

[David Flanagan [:djf]]

Yes, the tree is open again and this is a blocker, so land away.

Also, either uplift to v1.3 yourself or set the tracking flags so jhford uplifts.

Comment 17

[Jim Porter (:squib)]

Landed. What are the magic flags I need to set again? Is blocking-b2g: 1.3+ sufficient?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

At this stage, 1.3 blockers don't have automatic uplift approval. You need to request approval-gaia-v1.3 on the patch for consideration.

Comment 19

[Jim Porter (:squib)]

I don't see approval-gaia-v1.3 in the list of flags I can set (either for the whole bug or just the attachment). Am I doing something dumb?

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

I don't see that flag either.  It'd be for the attachment.

Byron, do you know what's happening here?

Comment 21

[:glob ✱]

(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #20)
> I don't see that flag either.  It'd be for the attachment.
> Byron, do you know what's happening here?

it used to be per-component opt-in for gaia components, which means unless it was explicitly requested when a component was created, it wouldn't be visible.  i've changed the visibility of the flag to all components under the firefoxOS product, which means it will now automatically be visible on new components.  if required, non-gaia components can opt-out.

Comment 22

[Jim Porter (:squib)]

Comment on attachment 8379277 [details] [review]
Fix this

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression; this has been around as long as the ringtones app has existed, I think
[User impact] if declined: "Ringtones" will show up in pick activities when it shouldn't, and users will just get sent back to the calling app/page
[Testing completed]: manually tested picking ringtones and making sure that they don't show up in places they shouldn't
[Risk to taking this patch] (and alternatives if risky): low risk; just a small change to the manifest
[String changes made]: none

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 043b24fbe6f79bc3c0785717a8143eb27833f721 to:
v1.3: 9cd4e12003dc50c92e4c49269124d56ec7b81f74