1017416 - [ringtones] touch targets in create screen are off

Status: RESOLVED FIXED

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

Description

[Tiffanie Shakespeare [:tif]]

STR
1. Have music on device
2. Go to Settings -> Sound -> Manage Ringtones
3. Tap New in upper right
4. Select song
5. Tap UI in create screen

Expected:
Tapping on the element does action

Actual:
Tapping next to the element does the action - see video attachments

Comment 1

[Tiffanie Shakespeare [:tif]]

Attachment: Save button

Comment 2

[Tiffanie Shakespeare [:tif]]

Attachment: default ringtone checkbox

Comment 3

[Tiffanie Shakespeare [:tif]]

Attachment: play button

Comment 4

[Marcia Knous [:marcia]]

Seeing this as well now that it has landed - I have a logcat if it is helpful.

For me it eventually takes if I hammer the button about 5-6 times.

Comment 6

[Marcia Knous [:marcia]]

Nominating to get this on the radar. Lots of folks hit this in the bug bash and thought the functionality was broken.

Comment 7

[Hema Koka [:hema]]

Blocking based on the video attachment - "no action on basic elements like save, ringtone checkout unless user taps multiple times"

Jim, please investigate

Comment 8

[Jim Porter (:squib)]

Etienne: I'm told you know about edge gestures. Do you have any idea what's happening here? I'm opening the share window via window.open here: https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/manage.js#L36

Comment 9

[Etienne Segonzac (:etienne)]

Looks like an edge gesture bug, I see [JavaScript Error: "TypeError: StackManager.getCurrent(...) is null" {file: "app://system.gaiamobile.org/js/edge_swipe_detector.js" line: 118}] in the logs.

So basically we know we should forward those touch events but we don't have the correct iframe to forward them to.

I'll take a look.

That said this is only valid for touch at the very edges of the screen, I have no idea why the play button might have issues.

Comment 10

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

The ringtones app has the system role, which explains the issue :)

Filtering the system app specifically in the stack manager, I don't see what can cause an appcreated event for system.gaiamobile.org but following the comment's hint.

Alternatively we could always go trough the AppWindowManager to get the current iframe but it feels inconsistent.

Comment 11

[Jim Porter (:squib)]

Yeah, I'm not 100% sure we want a "system" role, but I was following along with how the Keyboard app does its thing. The only real requirement is that we don't show the Ringtones app on the homescreen.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jim Porter (:squib) from comment #11)
> Yeah, I'm not 100% sure we want a "system" role, but I was following along
> with how the Keyboard app does its thing. The only real requirement is that
> we don't show the Ringtones app on the homescreen.

What the keyboard settings did is a really bad hack. We are trying several ways to fix it.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8440800 [details] [review]
Gaia PR

r+ with nit.
BTW I am not sure if the check is still available: no any system app should be launched via AppWindow.

Comment 14

[Jim Porter (:squib)]

We could also solve this with the patch on bug 1024779, which turns the ringtone manager into an inline activity (but it still has the "system" role).

Comment 15

[Jim Porter (:squib)]

Sorry, I should elaborate: the patch on that bug also seems to fix the touch events too, since the ringtone manager is launched differently.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jim Porter (:squib) from comment #15)
> Sorry, I should elaborate: the patch on that bug also seems to fix the touch
> events too, since the ringtone manager is launched differently.

I know, but it's fine to take this to prevent future bug.

Comment 17

[Jim Porter (:squib)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #16)
> (In reply to Jim Porter (:squib) from comment #15)
> > Sorry, I should elaborate: the patch on that bug also seems to fix the touch
> > events too, since the ringtone manager is launched differently.
> 
> I know, but it's fine to take this to prevent future bug.

Agreed. I just wanted to be sure everyone following along here was up-to-date.

Also, if we shouldn't be using a "system" role in the ringtone app's manifest, I'd be happy to change that too if we have (or create) an alternative way to tell the system that an app shouldn't have an icon on the homescreen.

Comment 18

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/059704ca682198041b16978e58764928e593c16b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/ebbe6f08b2440f89261cfd7ec76d85345f818654

Comment 20

[ankit93040]

Hi Etieene

Still after these fixes the issue remains as shown in the attachments videos.

The issue is still remains. It shouldn't be resolved.

Please check once.

thanks!

Comment 21

[ankit93040]

In addition to comment#20

I tested this on v2.0 & v1.4.

The issue is not solved.

You may check it by clicking in between the (play/pause) button & checkkox(use as default ringtone).

Comment 22

[Etienne Segonzac (:etienne)]

Jim do you think the rest of the issues will be fixed as part of bug 1024779?

Comment 23

[Jim Porter (:squib)]

That bug seems to fix things for me, yeah.

Comment 24

[Jim Porter (:squib)]

(In reply to Jim Porter (:squib) from comment #23)
> That bug seems to fix things for me, yeah.

I spoke too soon; this doesn't seem to fix things for the X button, at least.

Is there some other way I should be creating a popup window from an app? window.open just seems to be causing a bunch of problems.

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jim Porter (:squib) from comment #24)
> (In reply to Jim Porter (:squib) from comment #23)
> > That bug seems to fix things for me, yeah.
> 
> I spoke too soon; this doesn't seem to fix things for the X button, at least.
> 
> Is there some other way I should be creating a popup window from an app?
> window.open just seems to be causing a bunch of problems.

Could you let me know what's the problem with window.open?

Comment 26

[Jim Porter (:squib)]

So right now, this is what we do to create a new ringtone from the ringtone manager:

1) Open the ringtone manager as an inline activity from the settings app
2) Click "+" which opens a pick activity, launching the music app
3) Pick a song from the music app, which returns the result to the ringtone manager
4) Taking the result from the pick activity, use window.open to open share.html and send it the pick activity's data via postMessage once share.html has loaded.

The main issue I'm seeing right now is that when you try to tap the "X" button in the top left of share.html, it doesn't work and we get this error in the logcat:

E/GeckoConsole( 1840): [JavaScript Error: "TypeError: StackManager.getCurrent(...) is null" {file: "app://system.gaiamobile.org/js/edge_swipe_detector.js" line: 118}]

If the steps I laid out above are a bad way of doing what I want, let me know. Basically, I'm trying to emulate a share activity entirely within the ringtones app. I guess I could make it a *real* share activity, but I don't want anyone else to be able to handle that activity.

Comment 27

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jim Porter (:squib) from comment #26)
> So right now, this is what we do to create a new ringtone from the ringtone
> manager:
> 
> 1) Open the ringtone manager as an inline activity from the settings app
> 2) Click "+" which opens a pick activity, launching the music app
> 3) Pick a song from the music app, which returns the result to the ringtone
> manager
> 4) Taking the result from the pick activity, use window.open to open
> share.html and send it the pick activity's data via postMessage once
> share.html has loaded.
> 
> The main issue I'm seeing right now is that when you try to tap the "X"
> button in the top left of share.html, it doesn't work and we get this error
> in the logcat:
> 
> E/GeckoConsole( 1840): [JavaScript Error: "TypeError:
> StackManager.getCurrent(...) is null" {file:
> "app://system.gaiamobile.org/js/edge_swipe_detector.js" line: 118}]

This sounds a sad stack manager bug. We could fix it.

> 
> If the steps I laid out above are a bad way of doing what I want, let me
> know.

If share.html is part of your ringtone app I don't see anything you did wrong. Let's open a bug to fix it.

Comment 28

[Jim Porter (:squib)]

Actually, it looks like the problems with window.open got a bit more serious since I last pulled from master. Now, I'm getting the following error about 50% of the time:

E/GeckoConsole( 7239): [JavaScript Error: "TypeError: popup is null" {file: "app://ringtones.gaiamobile.org/gaia_build_defer_manage.js" line: 89}]

That's referring to the second line of this code:

      var popup = window.open('share.html');
      popup.addEventListener('load', function loaded() { // popup is null here
        popup.removeEventListener('load', loaded);
        popup.postMessage(result, window.location.origin);
      });

The other 50% of the time, the window opens fine, but there are two headers; one seems to be from the system app, and the other is in share.html. This seems to be fallout from bug 1021658, so I'll file a new bug with this info and mark dependencies.

Comment 29

[Jim Porter (:squib)]

I'm going to reopen this bug to work on the issues in comment 27, since the errors I'm getting seem to be the same as before. It's possible the change in bug 1024779 (which turned the ringtone manager from a full-fledged app with role: "system" to an inline activity) re-broke this...

I also filed bug 1027991 for comment 28.

Comment 30

[Jason Smith [:jsmith]]

Jim - Can you open a followup instead? This will help later down the line if we need to link this bug against something else, as we'll be able to identify this patch against any regressing causes. It also helps keep sheriffing a bit clearer if we use one patch per bug, as this helps ensure that we only need to do one sweep of the status flags once (rather than multiple times, which risks inconsistency on someone missing something).

Comment 31

[Jim Porter (:squib)]

Cloned to bug 1028048

Comment 32

[Alissa（leave from Mozilla）]

This issue has been failed verified on Flame 2.1, 2.0
Please refer to Comment 14 of bug 1028048