When more than one app can handle an activity, the first displayed app won't launch

RESOLVED FIXED

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: djf, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Steps to reproduce:

1) Long press on the homescreen to change the wallpaper. You'll see 3 choices: wallapper, gallery, camera.  The first choice (wallpaper) does nothing. The other two work.

2) Open the SMS app, start to compose a message, and tap the attach (paperclip) button.  You'll see many choices. They all seem to work, except for the first one (video).

If you select the first app listed, the app doesn't even appear to ever run. It is just never started and the activity request fails somehow.
(Reporter)

Comment 1

4 years ago
Needinfo on Alive, who was working on activities lately and Fabrice who knows activities well.
Flags: needinfo?(fabrice)
Flags: needinfo?(alive)
(Reporter)

Comment 2

4 years ago
I forgot to mention that I can reproduce this on tonight's nightly mozilla-central-eng build for unagi
I wander this is a gecko problem...because from my point of view, I see the mozContentEvent is dispatched.
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/activities.js#L71

I will check from b2g runtime later.
Flags: needinfo?(alive)
(Reporter)

Comment 4

4 years ago
I figured it out.

This is a regression caused by the big third party keyboard patch. That patch changed list_menu.js by adding this one line:

value = parseInt(value)

So list_menu is now passing a number to the callback instead of a string. 

Activities.js used to pass that string to gecko, but now it passes a number.  The number 0 is falsy, but the string "0" is truthy. 

The workaround is to convert back to a string in activities.js, and I'll attach a patch to do that.

Fabrice: if you want you could fix gecko to handle the numeric value 0.
(Reporter)

Comment 5

4 years ago
Created attachment 800657 [details]
link to patch on github

This patch fixes a regression caused by the list_menu.js change from https://github.com/mozilla-b2g/gaia/commit/9fb5802df60a9081846d704def01df814ed8fbd4
Attachment #800657 - Flags: review?(alive)
Comment on attachment 800657 [details]
link to patch on github

Thanks for spending time on this.r=me.
Attachment #800657 - Flags: review?(alive) → review+
(In reply to David Flanagan [:djf] from comment #4)
> I figured it out.
> 
> This is a regression caused by the big third party keyboard patch. That
> patch changed list_menu.js by adding this one line:
> 
> value = parseInt(value)
> 
> So list_menu is now passing a number to the callback instead of a string. 
> 
> Activities.js used to pass that string to gecko, but now it passes a number.
> The number 0 is falsy, but the string "0" is truthy. 
> 
> The workaround is to convert back to a string in activities.js, and I'll
> attach a patch to do that.
> 
> Fabrice: if you want you could fix gecko to handle the numeric value 0.

http://mxr.mozilla.org/mozilla-central/source/b2g/components/ActivitiesGlue.js#50

I think http://hg.mozilla.org/mozilla-central/rev/ba5b2970ceb5 has resolved this from gecko?
Anyway, IMO ensure data type correctness shall be done from both frontend and backend, so this patch is still a valid fix.
Isn't this a dupe of bug 912065 even though the patches are different? Are we taking two different patches here to fix this or should we take only one patch?
(Reporter)

Comment 10

4 years ago
(In reply to Alive Kuo [:alive] from comment #8)
> Anyway, IMO ensure data type correctness shall be done from both frontend
> and backend, so this patch is still a valid fix.

Actually, it looks like gecko is expecting a numeric array index, not a string, so coercing to a string seems wrong now that gecko is handling the 0 case correctly.

Whath should we do here?

- close this bug as a dupe.

- land my patch as it is. (but I don't recommend that)

- use this bug as a followup to the gecko patch and change activities.js:29 so that it passes the number 0 instead of the string '0'.  I suppose we should let the gecko patch propagate out a bit before making that gaia change, though, since this would break single-choice activity invocations for people who don't have up-to-date gekco.

Fabrice: will you decide which option is best here?
Yep, I changed gecko to expect a numeric index, since this is what it actually means, and our code there was fragile...

My preference is to make sure that gaia send the number 0 and not a string.
Flags: needinfo?(fabrice)
Per comment 11, let's close this since there's gecko patch already.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.