bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED

Status

Firefox OS
Gaia::Ringtones
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Bingqing Li, Assigned: squib)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
djf
: review+
jsmith
: feedback+
Details | Review | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Hardware: x86_64 → ARM
(Reporter)

Updated

5 years ago
Summary: [File upload selector] Choosing the system ringtones will go back to the webpage → [Fugu][File upload selector] Choosing the system ringtones will go back to the webpage

Updated

5 years ago
blocking-b2g: --- → 1.3?
David, do you have any input here?
Flags: needinfo?(dflanagan)
1.4? as ringtone work is being done in 1.4
blocking-b2g: 1.3? → 1.4?
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.
Flags: needinfo?(dflanagan)

Updated

5 years ago
Component: Gaia::Browser → Gaia::Ringtones
(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.
blocking-b2g: 1.4? → 1.3?
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
1.3+ for blocking in 1.3. This is a broken feature of ringtone support
blocking-b2g: 1.3? → 1.3+
(Assignee)

Comment 7

5 years ago
(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?
(Assignee)

Comment 8

5 years ago
Taking since I know ringtones now (sort of).
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
(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"
}
(Assignee)

Comment 10

5 years ago
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.
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 8379277 [details] [review]
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...
Attachment #8379277 - Flags: review?(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.
Attachment #8379277 - Flags: review?(jsmith)
Attachment #8379277 - Flags: review?(dflanagan)
Attachment #8379277 - Flags: feedback+
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.
Attachment #8379277 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 15

5 years ago
Yeah, the ringtone pick activity works just fine from the settings app. I can land this once things are green, yes?
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.
(Assignee)

Comment 17

5 years ago
Landed. What are the magic flags I need to set again? Is blocking-b2g: 1.3+ sufficient?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 19

5 years ago
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?
I don't see that flag either.  It'd be for the attachment.

Byron, do you know what's happening here?
Flags: needinfo?(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.
Flags: needinfo?(glob)
(Assignee)

Comment 22

4 years ago
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
Attachment #8379277 - Flags: approval-gaia-v1.3?
Attachment #8379277 - Flags: approval-gaia-v1.3? → approval-gaia-v1.3+
Uplifted 043b24fbe6f79bc3c0785717a8143eb27833f721 to:
v1.3: 9cd4e12003dc50c92e4c49269124d56ec7b81f74
status-b2g-v1.3: --- → fixed
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.