902981 - [Music] [User Story] Provide access to music controls in the notifications tray

Status: VERIFIED FIXED

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

Description

[Sri Kasetti]

User Story:

As a user, I want to access the controls to the music app in a notification tray or enter the music app by swiping on the play icon on the status bar while the homescreen is still unlocked

Comment 1

[Jim Porter (:squib)]

Taking and setting blocking bug (this should happen before lockscreen controls).

Comment 2

[Jim Porter (:squib)]

This is essentially done, and I'm just waiting for bug 902974 to land now. For anyone who'd like to follow along, the code is here: https://github.com/jimporter/gaia/compare/music-nowplaying-notification...music-notification-controls

Comment 3

[Jim Porter (:squib)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/12314

Ok, this is done. Note that the PR has the commits from bug 902981 in it.

Comment 4

[David Flanagan [:djf]]

(In reply to Jim Porter (:squib) from comment #3)
> Created attachment 807192 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12314
> 
> Ok, this is done. Note that the PR has the commits from bug 902981 in it.

Wait, this is bug 902981. And the PR just has one commit in it, so I'm confused.  Also, do you want Alive and I to both revhew the whole patch, or are you asking me to review the music changes and Alive to review the system changes?

Comment 5

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #4)
> (In reply to Jim Porter (:squib) from comment #3)
> > Created attachment 807192 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/12314
> > 
> > Ok, this is done. Note that the PR has the commits from bug 902981 in it.
> 
> Wait, this is bug 902981. And the PR just has one commit in it, so I'm
> confused.

Sorry, I meant bug 902974. The PR has only one commit in it now because bug 902974 landed, so you won't see its commits in the PR anymore.

> Also, do you want Alive and I to both revhew the whole patch, or
> are you asking me to review the music changes and Alive to review the system
> changes?

I think it makes the most sense for you to review the music changes (namely, shared/js/media/remote_controls.js), and Alive to review the system changes. I think it might make sense for both of you to look over the tests though, since the fake music app is supposed to be similar to the real music app.

Comment 6

[Jim Porter (:squib)]

Oh, as a note to reviewers, I'll be updating this with a few small changes in the system app, namely to allow fast-forwarding by holding the next or previous buttons (like the player in the music app and the bluetooth controls).

Comment 7

[Jim Porter (:squib)]

Hm, maybe not. For some reason, I can't manage to detect a long-press on the controls (I'm listening for contextmenu), even though it works on other elements. Does anyone have any ideas why that would be? The only difference I can see is that it works on a div, but not on a button.

Comment 9

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

Comment on attachment 807192 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12314

r+ for system part

Comment 10

[David Flanagan [:djf]]

Comment on attachment 807192 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12314

Sorry this took so long.  r+.

This was my first time reviewing IAC code, so it was pretty interesting for me.

Comment 12

[Jim Porter (:squib)]

Attachment: What it looks like

Here's what the patch looks like in action. Hopefully this is enough detail; I could film a video of this in action, but that'd be more work. :)

Comment 13

[Rob MacDonald [:robmac]]

I will take a look at this today or tomorrow but, in the meantime, I'll flag Eric for visual design review.

Comment 14

[Eric Pang [:epang] UX]

Attachment: proposed visual updates.png

Hey Jim, this is looking good :).  I just have a few suggestions. I've attached an image with a few small tweaks to update the layout and highlight states.  Let me know what you think or if you have any questions. Thanks!

Comment 15

[Jim Porter (:squib)]

(In reply to Eric Pang [:epang] from comment #14)
> Hey Jim, this is looking good :).  I just have a few suggestions. I've
> attached an image with a few small tweaks to update the layout and highlight
> states.  Let me know what you think or if you have any questions. Thanks!

Ok, I've updated the CSS to match this. Good catch on the border-radius for the buttons! I didn't see it on a desktop build; must be some sort of build system weirdness.

The smaller album cover looks really strange to me, though. Maybe that's just because I've been staring at the old version so long, so I'll defer to you here. :)

Comment 16

[Jim Porter (:squib)]

Two more small questions that I just thought of:

1) Users can tap the "now playing" info at the top to go to the music app. Should there be an on-tap state for that like how the buttons turn blue?

2) The (wrong) blue color I used for the buttons came from the music app. Should I change it to #00aacc there as well?

Comment 17

[Rob MacDonald [:robmac]]

Comment on attachment 819910 [details]
What it looks like

Looks great. Thanks for all of your hard work on this!

Comment 18

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/36048198ae3f87671ced8c0179bea1e029ec7d9c

Thanks everyone for reviews!

Comment 19

[Eric Pang [:epang] UX]

(In reply to Jim Porter (:squib) from comment #16)
> Two more small questions that I just thought of:
> 
> 1) Users can tap the "now playing" info at the top to go to the music app.
> Should there be an on-tap state for that like how the buttons turn blue?
> 
> 2) The (wrong) blue color I used for the buttons came from the music app.
> Should I change it to #00aacc there as well?

Hey Jim,

Sorry for the delayed response on this!  Hope it isn't too late..

1. The notification should have a hit state in general for row items.  I previously opened this bug about it: 917860

2.  Yes if you can update the highlight state to #00aacc that would be great

Thanks again for all the work you've done on this!

Comment 20

[Jim Porter (:squib)]

(In reply to Eric Pang [:epang] from comment #19)
> 1. The notification should have a hit state in general for row items.  I
> previously opened this bug about it: 917860

I hate to be a bother, but could you update attachment 821630 [details] to cover this? I want to make sure I get it right. Specifically, the things I'm most concerned about are:

1) How much of the now playing widget should get highlighted? I think we should highlight the area that you can tap to open the music app, but this means that, with the margins the way they are, the music icon isn't vertically centered in the highlighted area. This looks a bit strange.

2) Should the highlight state on the controls match the highlight state for the notifications? That would be more consistent, I think...

I'll probably end up doing most of these changes in bug 891024, which is a followup to this bug anyway.

> 2.  Yes if you can update the highlight state to #00aacc that would be great

I'll file a bug on this later today.

Comment 21

[Eric Pang [:epang] UX]

Attachment: notification highlight.png

Hey Jim, sorry for getting back to you about this so late, I didn't see the msg, next time ni me :).

1) How much of the now playing widget should get highlighted? I think we should highlight the area that you can tap to open the music app, but this means that, with the margins the way they are, the music icon isn't vertically centered in the highlighted area. This looks a bit strange.

- I was thinking that the entire row gets highlighted since that's how the other notifications function.  We can still use only the music icon area as the hit area, but when pressed highlight the entire notification. Currently the highlight is #b1f1ff at 60% opacity.

2) Should the highlight state on the controls match the highlight state for the notifications? That would be more consistent, I think...

- I agree this is a good idea for consistency :)

Comment 22

[Jim Porter (:squib)]

(In reply to Eric Pang [:epang] from comment #21)
> Hey Jim, sorry for getting back to you about this so late, I didn't see the
> msg, next time ni me :).

Noted.

> - I agree this is a good idea for consistency :)

I filed bug 938502 to track this. I'll post screenshots for you to look at once I make the changes. Thanks!

Comment 23

[Marcia Knous [:marcia]]

Verified fixed on Buri using:

Gaia   2cc78d696482e0434b584f5645af55e3105e59a2
SourceStamp 3c4fc4279e6a
BuildID 20131122123502
Version 28.0a1
Base image: 11-15

Have added a few test cases to Moztrap, but when I finish I will change that flag.

Comment 24

[Marcia Knous [:marcia]]

#10617 added to Moztrap.