Closed Bug 1017416 Opened 7 years ago Closed 7 years ago

[ringtones] touch targets in create screen are off

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: tif, Assigned: etienne)

References

Details

(Whiteboard: interaction-design [priority])

Attachments

(4 files)

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
Whiteboard: interaction-design
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.
Duplicate of this bug: 1021406
Nominating to get this on the radar. Lots of folks hit this in the bug bash and thought the functionality was broken.
blocking-b2g: --- → 2.0?
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Blocking based on the video attachment - "no action on basic elements like save, ringtone checkout unless user taps multiple times"

Jim, please investigate
Assignee: nobody → squibblyflabbetydoo
blocking-b2g: 2.0? → 2.0+
Whiteboard: interaction-design → interaction-design [priority]
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
Flags: needinfo?(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.
Flags: needinfo?(etienne)
Attached file 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.
Assignee: squibblyflabbetydoo → etienne
Attachment #8440800 - Flags: review?(alive)
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.
(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 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.
Attachment #8440800 - Flags: review?(alive) → review+
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).
Sorry, I should elaborate: the patch on that bug also seems to fix the touch events too, since the ringtone manager is launched differently.
(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.
(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.
https://github.com/mozilla-b2g/gaia/commit/059704ca682198041b16978e58764928e593c16b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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!
Flags: needinfo?(ryanvm)
Flags: needinfo?(etienne)
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).
Flags: needinfo?(ryanvm)
Jim do you think the rest of the issues will be fixed as part of bug 1024779?
Flags: needinfo?(etienne) → needinfo?(squibblyflabbetydoo)
That bug seems to fix things for me, yeah.
Flags: needinfo?(squibblyflabbetydoo)
(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.
(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?
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.
(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.
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(squibblyflabbetydoo)
Resolution: --- → FIXED
Blocks: 1028048
Cloned to bug 1028048
Flags: needinfo?(squibblyflabbetydoo)
This issue has been failed verified on Flame 2.1, 2.0
Please refer to Comment 14 of bug 1028048
You need to log in before you can comment on or make changes to this bug.