1026692 - Access to mute button

Status: VERIFIED FIXED

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

Comment 1

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Mockup. Call incall keypad with mute button

Comment 2

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Mockup. Call with new taller hang up button

Comment 3

[Victoria Gerchinhoren [:vicky]]

Visuals delivered to cover the inclusion of the mute button when the dialpad is displayed in an active call. Bare in mind that:

A- In order to include this extra button in the layout, the numbers spacing in the keypad was modified reducing the vertical gap between rows, creating the enough space on the bottom to hold a circular button big enought for the required hierarchy of this kind of action.

B - Modifying the height in one view of the same instance, required the modification of the other view (idle view with no dial pad). So this other view is also affected.

C- Specs of spacing should be provided by new dialer owner Carol. NI to her to attach it to the bug when ready.

Thanks.

Comment 4

[Doug Sherk (:drs) (inactive)]

We're going to take this if Carrie agrees that this is ready to go.

Comment 5

[Doug Sherk (:drs) (inactive)]

We decided that we need the assets to get going, so this isn't actionable yet.

Comment 6

[Carrie Wang [New account: [e-mail]]]

I'm fine with the visual design here. Carol will provide the assets. Thanks!

Comment 7

[Carol Huang [:Carol]]

Attachment: Call_incall_keypad.jpg

"Call_incall_keypad with mute button" screen layout revised:
Just a small tweak on the keypad's position.

Comment 8

[Carol Huang [:Carol]]

Attachment: new_hangup_btn.jpg

Tweak on the size of the new hang-up btn.

Comment 9

[Carol Huang [:Carol]]

For [Dialer] Call with new taller hang up button, I filed a new bug for it. Please see Bug 104313.

Comment 10

[Carol Huang [:Carol]]

Attachment: Call_INCALL-KEYPAD_Visual Spec.zip

the attachment is "Call incall keypad with mute button" Visual Spec.
The keypad layout didnt change, I only move the keypad a bit higher since we have circular btns in the bottom.
let me know if there's any question, thanks!!

Comment 11

[Doug Sherk (:drs) (inactive)]

Putting this in next sprint for now as it's actionable and we will likely take it.

Comment 12

[Anthony Ricaud (:rik)]

Carol: I believe you meant bug 1043133 instead.

Comment 13

[Carol Huang [:Carol]]

(In reply to Anthony Ricaud (:rik) from comment #12)
> Carol: I believe you meant bug 1043133 instead.

Hi Anthony,
Yes, it's bug 1043133 for [Dialer] Call with new taller hang up button. Sorry for the mistake bug#.

Comment 14

[Eric Chang [:ericcc] [:echang]]

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-description=mute&filter-productversion=196&filter-suite=130

Comment 15

[Tamara Hills [:thills]]

Attachment: WIP for feedback

This is my approach so far with the UI.  I had to add the mute button in there.  I still have not hooked anything up and I think I need to refine the spacings some more.  I'm struggling right now with removing the white space right at the top of the keyboard-container.  I'll attach a screenshot of that.

Comment 16

[Tamara Hills [:thills]]

Attachment: WIP attachment with white bar..

Comment 17

[Anthony Ricaud (:rik)]

Let's focus on adding the button in this bug. Having the keypad covering all the screen is a different problem that should be split in a separate bug.

Comment 18

[Anthony Ricaud (:rik)]

I think we should take the occasion of this redesign/new functionality to convert them to real <button> elements.

Comment 19

[Anthony Ricaud (:rik)]

Carol: For asssets with different states, it is easier for us to work with sprites. That's because it responds faster to user inputs (we display another part of an image already loaded) rather than loading a new image when the user taps.

Comment 20

[Anthony Ricaud (:rik)]

Comment on attachment 8469989 [details] [diff] [review]
WIP for feedback

I've put together a redux version of this patch. I may have forgotten some details: http://jsbin.com/lozakeni/1/edit

I should have given you those tips before you started, sorry for that.

Comment 21

[Carol Huang [:Carol]]

Attachment: Call_INCALL-KEYPAD_Visual Spec_0811.zip

Hi Anthony,
I've updated the image assets to sprite. Thanks!

Comment 22

[Tamara Hills [:thills]]

Attachment: 1026692v2.diff

Attached is the latest.  

1.  Wanted thoughts on the approach of keeping the movement of the sprite when the mute button is depressed in the keypad.js.  I wasn't sure of another way to do this via CSS since it needs to persist as pressed and there is logic to go along with that to keep it in synch with the mute state.  It seems ugly to have to put that in the js.

2.  Also, I left the white bar in there since we decided to deal with that separately.

3.  Visually, the buttons don't show up very well (especially when state is muted).  I think we should bring this to Carol's attention, but wanted to check with you first.  I'll attach a couple screenshot so you can see how difficult it can be to see white on top of #ded9d6

Comment 23

[Tamara Hills [:thills]]

Attachment: Screenshots of muted and unmuted

Comment 24

[Anthony Ricaud (:rik)]

Comment on attachment 8471125 [details] [diff] [review]
1026692v2.diff

Review of attachment 8471125 [details] [diff] [review]:
-----------------------------------------------------------------

I'm surprised we don't need more changes in keypad.js to account for the markup changes. Have you tested that all buttons work after this?

::: shared/js/dialer/keypad.js
@@ +282,4 @@
>  
>        if (this.hideBar) {
>          this.hideBar.classList.remove('hide');
> +        CallScreen.calls.classList.contains('muted')?

This is bad practice to have an object know about the DOM structure of another one. If CallScreen changes the way it says we are muted, this will break.
Therefore, I recommend moving this code to call_screen.js. You should change toggleMute to also add an active-state class to the new button you're introducing.
We should also remove hideBarHideAction and hideBarHangUpAction from keypad.js but let's not do it in this bug. Could you open a follow up ?

::: shared/style/dialer/keypad.css
@@ +290,5 @@
> +  border: none;
> +  border-radius: 100%;
> +  width: 5rem;
> +  height: 5rem;
> +  background-position: 45% -20%;

Don't use % for background-position for sprites.

Comment 25

[Tamara Hills [:thills]]

Attachment: muted2.png

Hi Carol,

Wanted to get some input from you on this screenshot.  Two issues:

1.  When the phone is muted, it's very hard to see the icon as it's white on grey?  Just want to make sure I'm not missing something.

2.  Also, we are looking to address just the mute button in this bug and not readjust the layouts and remove the separator that is currently there in between the contact and where the dialpad starts.  This attachment shows the layout set to 36.5.  As you can see, there is not much space at the top of the numbers.  

Thanks,

-tamara

Comment 26

[Tamara Hills [:thills]]

Attachment: updated after latest feedback

Hi Anthony,

I moved the mute button out of keypad.js and into the callscreen and changed the % to rem and updated the tests.

Thanks.

Comment 27

[Carol Huang [:Carol]]

Attachment: Call_INCALL-KEYPAD_Visual Spec_0813.zip

Update the image asset:
Add one more 'Focus' State for the mute button.

Comment 28

[Carol Huang [:Carol]]

Attachment: Bug 1026692_revise.pdf

Hi Tamara,
I've looked at the screenshot you provide and have some comment.(please see the attachment)

1. Add one more state for the mute button (normal/pressed/focused)
2. Icons(mute/dismiss keypad) need to be center aligned both vertically and horizontally in the circular button.
3. Move down those three buttons (1rem from the bottom of the screen)
let me know if you have any other question.

I'll file another bug for removing the seperator between contact and number pad. thanks!

Comment 29

[Anthony Ricaud (:rik)]

Comment on attachment 8472027 [details] [diff] [review]
updated after latest feedback

Review of attachment 8472027 [details] [diff] [review]:
-----------------------------------------------------------------

A much simpler patch, nice! :)

::: shared/style/dialer/keypad.css
@@ +168,5 @@
>    overflow: visible;
>  }
>  
> +body.showKeypad #keyboard-container {
> +  height: 36.5rem;

We shouldn't define this height. And remove it from oncall.css too. Let the dimensions and margins of the keypad and the hide bar affect the overall height of this container.

@@ +291,5 @@
> +  border: none;
> +  border-radius: 100%;
> +  width: 5rem;
> +  height: 5rem;
> +  background-position: .34rem .69rem;

This is very specific and therefore prone to break. Let's ask Carol to provide a sprite that is 50px high for each state. We can center horizontally via CSS but we rely on the sprite heights to get the vertical centering.

@@ +297,5 @@
>  }
>  
> +.kh__button--mute {
> +  background-image: url('keypad/images/keypad/actionicon_activecall_mute.png');
> +  background-repeat: no-repeat;

background-repeat should be in .kh__button

@@ +302,4 @@
>  }
>  
> +.kh__button--mute.active-state {
> +  background-position: .34rem -3.35rem;

.kh__button.active-state and .kh__button:active are more generic selectors. This will avoid repetition on line 313.

@@ +319,5 @@
> +  height: 6.5rem;
> +  background: #e00;
> +  border: none;
> +  border-radius: 100%;
> +  background-position: 1.2rem 1.1rem;

1.2rem here could probably be replaced by center.

Comment 30

[Anthony Ricaud (:rik)]

Carol: Thanks for iterating so fast on our requests! Could you provide sprites that are 50px high (aka the height of the button) for each state? That will help us center them vertically and avoid seeing parts of the other states. Width is not important as long as the icon is centered in it.

Comment 31

[Tamara Hills [:thills]]

Attachment: 1026692v4.diff

Comment 32

[Tamara Hills [:thills]]

Attachment: mute3.png

Comment 33

[Tamara Hills [:thills]]

Hi Carol,

thanks for your feedback.  I'll add the third state for the button and fix the alignments.  I just need the updated sprites as per Anthony's comment 30.

Thanks,
-tamara

Comment 34

[Carol Huang [:Carol]]

Hi Tamara,
I was just wondering that is it possible we just show 40x40 px area for the icon instead of changing the asset size. because it means once we change the circle button size, we need to change the asset again.
Let me know if it make sense.
If it doesnt work as what I described, I will sure update the sprite soon. Thank you :)

Comment 35

[Anthony Ricaud (:rik)]

Tamara: Let's use a :before technique to solve this. We'll put the icon in a 40x40 :before area and that will solve it for everything.

Comment 36

[Tamara Hills [:thills]]

Attachment: Link to pull request

Hi Rik,

Attached is pull request.  I got the :before technique working.  The issue with the separator is still there.  I believe we should fix this when we have a follow up to fix the separator because it's very difficult not to interfere with dialer styles on this.  I put a workaround that needs to be removed when this is done.  I believe Carol is goign to file this bug.

-tamara

Comment 37

[Tamara Hills [:thills]]

Hi Carol,

Can you let me know the bug number of the follow up to remove the separator between the name and the dialpad as per comment 28?

thanks,

-tamara

Comment 38

[Anthony Ricaud (:rik)]

Comment on attachment 8473939 [details] [review]
Link to pull request

Run the images through ./tools/png-recompress.sh or something like http://imageoptim.pornel.net/.

For the ::before, put a |position: relative| on the .kh__button and .kh__call-end-button. Then use |top: 0.5rem| and |left: 0.5rem| to position the ::before in the proper place.

Comment 39

[Carol Huang [:Carol]]

Attachment: Dialer_screenshots_0819.jpg

Hi Tamara,
I also looked at the patch you have on attachment 8473939 [details] [review]. 
The attachment is the screenshot result.

I was just wondering if the patch is just for the 3 buttons because the patch is quite different from the screenshot you have on attachment 8472398 [details].
If the patch is just for the buttons, you can ignore this comment. thank you!

Comment 40

[Anthony Ricaud (:rik)]

Carol: For the separator, we are not going to fix it in this bug. This is not crucial to 2.1 and not easy to fix properly. So we'll do that in another bug.

For the buttons alignment, could you provide us the margins? In the spec, I can see the vertical margins but not the horizontal ones. Thanks.

Tamara: I forgot to say in my review that you need to use: |background-size: contains| to have HiDPI images stay in the buttons.

Comment 41

[Carol Huang [:Carol]]

Attachment: Call_INCALL-KEYPAD_Visual Spec_0819.zip

Visual spec update!
Add spec information
1. the distance between mute/hang-up/dismiss keyboard is 3.8 rem(w)
2. this set of buttons(mute/hang-up/dismiss keyboard) are horizontally center aligned in the screen

@Tamara,
Sorry I forgot to write these information down on the spec.

@Anthony,
I know we're not going to fix the separator on this bug. I just wanted to make sure how the screen look like now in order to fix the layout :) (because on 320x480 looks good but on 480x854 is not, I'm kinda confused.)
let me know if you guys have any other question.
Thank you!!

Comment 42

[Tamara Hills [:thills]]

Attachment: screenshots of flame and hamachi layouts

Hi Carol,

Just to make sure we're on the same page, there is about 4rem on either side of the buttons... so, it's like this:

4rem<mute>3.8rem<hangup>3.8rem<back button>4rem

I attached screenshots so you can see the height layout on both the flame and the hamachi.  You'll see the flame still has the image showing in part of it.

Comment 43

[Tamara Hills [:thills]]

(In reply to Tamara Hills [:thills] from comment #42)
> Created attachment 8475168 [details]
> Just to make sure we're on the same page, there is about 4rem on either side
> of the buttons... so, it's like this:
> 
> 4rem<mute>3.8rem<hangup>3.8rem<back button>4rem
Hi Carol,

There is currently more space on the right margin than on the left margin.  I think it will look a bit odd to even out the margins exactly.  Right now, the layout in the attach screenshots is roughly:

2.7rem<mute>3.8<hangup>3.8rem<backbutton>4.9rem.

Let me know if you want to change the side margins.

Comment 44

[Tamara Hills [:thills]]

Attachment: changes from last review... Remove the inline styling and move some items from the subclasses into the .kh_button selectors

Hi Anthony,

The pull request is updated, but I also included a .diff from last time if that's easier to look at than everything squashed.

The pull request: https://github.com/mozilla-b2g/gaia/pull/22941

Comment 45

[Anthony Ricaud (:rik)]

Comment on attachment 8475207 [details] [diff] [review]
changes from last review... Remove the inline styling and move some items from the subclasses into the .kh_button selectors

Review of attachment 8475207 [details] [diff] [review]:
-----------------------------------------------------------------

You still need |background-size: contains| to have HiDPI images stay in the buttons. Use |GAIA_DEV_PIXELS_PER_PX=2 APP=callscreen make install-gaia && b2g_reboot| to verify this.

For the horizontal alignment, please post two screenshots, one with padding-right: 2rem and one without so Carol can take a look.

::: shared/style/dialer/keypad.css
@@ +298,3 @@
>    background-position: 0 0;
> +  top: .5rem;
> +  left: .4rem;

This should be .5rem too.

@@ +308,5 @@
> +  background-image: url('keypad/images/keypad/actionicon_activecall_mute.png');
> +}
> +
> +.kh__button--mute:active {
> +  background-color: #9ef2e7;

This can be .kh__button:active and you can drop the .kh__button--hide:active

@@ +313,3 @@
>  }
>  
>  .kh__button--mute.active-state::before {

This should be .kh__button.active-state::before

Comment 46

[Tamara Hills [:thills]]

Attachment: screens with and without padding

Hi Carol,

Here is screens with and without padding.  The one with padding pushes things to the left a little more.

Let us know which you prefer.

Thanks,

-tamara

Comment 47

[Tamara Hills [:thills]]

Attachment: PART 1: Review changes to add contain/cover for background-size and move a few more selectors into the base class.

Hi Anthony,
Updates after latest review.  drs helped me with the cover vs. contain on the background-size and I tested out all the resolutions.

We still need the feedback from Carol on the horizontal, so not sure if you want to wait for that before reviewing or not.

Comment 48

[Tamara Hills [:thills]]

Attachment: PART 2 -- includes some review changes from drs

Anthony,

I'm posting this diff because drs did some review of the changes and this includes his feedback.  This diff has only his changes on top of the attachment 8475375 [details] [diff] [review].

The pull request has everything: https://github.com/mozilla-b2g/gaia/pull/22941

Thanks,

-tamara

Comment 49

[Tamara Hills [:thills]]

Attachment: PART 3: grouping of css attributes and adjustment of spacing

Anthony,

Part 3 is here.  The changes here is some additional feedbacks from drs to group the css attributes by function more closely.  Also, I removed some of the original lines in Part 2 (a miscommunication).

It may be easier to look at the spacing in the pull request as that is all squashed.

Comment 50

[Tamara Hills [:thills]]

Attachment: Pull Request for squashed changes (parts 1-3)

Hi Anthony,

Sorry this turned into a bit of a mess.  We added some spacing, then I misunderstood something and then we added more grouping.

What you might want to do is to look at part 1 to see the changes from your last review and then look at pull request (which has everything squashed) to see the spacing/grouping changes.

Comment 51

[Carol Huang [:Carol]]

Hi Tamara,
I would like to go with the one Without padding, Thank you!!

Comment 52

[Anthony Ricaud (:rik)]

Comment on attachment 8475505 [details] [review]
Pull Request for squashed changes (parts 1-3)

r+ with two small comments to address before landing.

Comment 53

[Tamara Hills [:thills]]

Attachment: Screens for review

Hi Carol,

Here are the screens for your review.

Thanks,

-tamara

Comment 54

[Carol Huang [:Carol]]

Comment on attachment 8476970 [details]
Screens for review

Hi Tamara, 
The buttons looks good! Thanks!

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/0cb0ebfa4965f62f63dab5972650b0e11346ff4c

Comment 56

[Johan Lorenzo [:jlorenzo]]

Verified with the test suite in comment #14 on today's master.