Closed Bug 1026692 Opened 6 years ago Closed 6 years ago

Access to mute button

Categories

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

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.1, tracking-b2g:backlog, b2g-v2.1 verified)

VERIFIED FIXED
2.1 S3 (29aug)
feature-b2g 2.1
tracking-b2g backlog
Tracking Status
b2g-v2.1 --- verified

People

(Reporter: wmathanaraj, Assigned: thills)

References

Details

(Keywords: feature, Whiteboard: [tako][planned-sprint c=6][2.1-feature-qa+][in-sprint=v2.1-S2])

User Story

As a user I always want to have access to the mute button in the dialer so I can quickly mute the call.


AC1: I want mute button even when the dial pad is open

Attachments

(15 files, 12 obsolete files)

191.91 KB, image/jpeg
Details
214.62 KB, image/jpeg
Details
1.28 MB, application/pdf
Details
157.49 KB, image/png
Details
46 bytes, text/x-github-pull-request
rik
: review-
Details | Review
265.31 KB, image/jpeg
Details
994.78 KB, application/zip
Details
346.20 KB, application/zip
Details
4.27 KB, patch
rik
: review-
Details | Diff | Splinter Review
109.59 KB, application/zip
Details
1.34 KB, patch
Details | Diff | Splinter Review
1.21 KB, patch
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
rik
: review+
Details | Review
154.91 KB, application/zip
Carol
: ui-review+
Details
No description provided.
blocking-b2g: --- → backlog
feature-b2g: --- → 2.1
User Story: (updated)
Keywords: feature
Whiteboard: [tako]
Assignee: nobody → drs+bugzilla
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.
Flags: needinfo?(chuang)
QA Contact: echang
QA Whiteboard: [Gaia::Dialer]
QA Whiteboard: [Gaia::Dialer] → [COM=Gaia::Dialer]
Target Milestone: --- → 2.1 S1 (1aug)
Whiteboard: [tako] → [tako][planned-sprint]
Whiteboard: [tako][planned-sprint] → [tako][planned-sprint c=3]
We're going to take this if Carrie agrees that this is ready to go.
Flags: needinfo?(drs+bugzilla)
Flags: needinfo?(cawang)
We decided that we need the assets to get going, so this isn't actionable yet.
Whiteboard: [tako][planned-sprint c=3] → [tako]
Target Milestone: 2.1 S1 (1aug) → ---
I'm fine with the visual design here. Carol will provide the assets. Thanks!
Flags: needinfo?(cawang)
Flags: needinfo?(drs+bugzilla)
Attached image Call_incall_keypad.jpg
"Call_incall_keypad with mute button" screen layout revised:
Just a small tweak on the keypad's position.
Attachment #8456222 - Attachment is obsolete: true
Attached image new_hangup_btn.jpg
Tweak on the size of the new hang-up btn.
Attachment #8456223 - Attachment is obsolete: true
For [Dialer] Call with new taller hang up button, I filed a new bug for it. Please see Bug 104313.
Attached file Call_INCALL-KEYPAD_Visual Spec.zip (obsolete) —
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!!
Flags: needinfo?(chuang)
Putting this in next sprint for now as it's actionable and we will likely take it.
Target Milestone: --- → 2.1 S2 (15aug)
Carol: I believe you meant bug 1043133 instead.
Depends on: 1043133
QA Whiteboard: [COM=Gaia::Dialer] → [COM=Gaia::Dialer][2.1-feature-qa+]
(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#.
Whiteboard: [tako] → [tako][planned-sprint c=]
Assignee: drs+bugzilla → thills
Whiteboard: [tako][planned-sprint c=] → [tako][planned-sprint c=6]
No longer depends on: 1043133
See Also: → 1043133
Status: NEW → ASSIGNED
Attached patch WIP for feedback (obsolete) — Splinter Review
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.
Attachment #8469989 - Flags: feedback?(anthony)
Attached image WIP attachment with white bar.. (obsolete) —
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.
I think we should take the occasion of this redesign/new functionality to convert them to real <button> elements.
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 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.
Attachment #8469989 - Flags: feedback?(anthony)
QA Whiteboard: [COM=Gaia::Dialer][2.1-feature-qa+] → [COM=Gaia::Dialer]
Whiteboard: [tako][planned-sprint c=6] → [tako][planned-sprint c=6][2.1-feature-qa+]
Hi Anthony,
I've updated the image assets to sprite. Thanks!
Attachment #8461301 - Attachment is obsolete: true
Attached patch 1026692v2.diff (obsolete) — Splinter Review
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
Attachment #8469989 - Attachment is obsolete: true
Attachment #8469991 - Attachment is obsolete: true
Attachment #8471125 - Flags: feedback?(anthony)
Attached file Screenshots of muted and unmuted (obsolete) —
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.
Attachment #8471125 - Flags: feedback?(anthony)
Attached image muted2.png (obsolete) —
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
Attachment #8471128 - Attachment is obsolete: true
Flags: needinfo?(chuang)
Attached patch updated after latest feedback (obsolete) — Splinter Review
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.
Attachment #8471125 - Attachment is obsolete: true
Attachment #8472027 - Flags: feedback?(anthony)
Update the image asset:
Add one more 'Focus' State for the mute button.
Attachment #8470617 - Attachment is obsolete: true
Attached file 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!
Flags: needinfo?(chuang) → needinfo?(thills)
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.
Attachment #8472027 - Flags: feedback?(anthony)
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.
Flags: needinfo?(chuang)
Attached patch 1026692v4.diff (obsolete) — Splinter Review
Attachment #8472396 - Flags: feedback?(anthony)
Attached image mute3.png
Attachment #8472025 - Attachment is obsolete: true
QA Contact: echang → jlorenzo
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
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 :)
Flags: needinfo?(chuang)
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.
Attached file 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
Attachment #8473939 - Flags: review?(anthony)
Whiteboard: [tako][planned-sprint c=6][2.1-feature-qa+] → [tako][planned-sprint c=6][2.1-feature-qa+][in-sprint=v2.1-S2]
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
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
Flags: needinfo?(thills) → needinfo?(chuang)
Attachment #8472027 - Attachment is obsolete: true
Attachment #8472396 - Attachment is obsolete: true
Attachment #8472396 - Flags: feedback?(anthony)
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.
Attachment #8473939 - Flags: review?(anthony) → review-
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!
Flags: needinfo?(chuang) → needinfo?(thills)
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.
Flags: needinfo?(chuang)
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!!
Attachment #8472233 - Attachment is obsolete: true
Flags: needinfo?(chuang) → needinfo?(anthony)
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.
Flags: needinfo?(chuang)
(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.
Flags: needinfo?(thills)
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
Attachment #8475207 - Flags: review?(anthony)
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
Attachment #8475207 - Flags: review?(anthony) → review-
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
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.
Attachment #8475207 - Attachment is obsolete: true
Attachment #8475375 - Flags: review?(anthony)
Attachment #8475207 - Attachment description: changes from last review... pull request is also updated. → changes from last review... Remove the inline styling and move some items from the subclasses into the .kh_button selectors
Attachment #8475207 - Attachment is obsolete: false
Attachment #8475375 - Attachment description: updated after latest feedback → Review changes to add contain/cover for background-size and move a few more selectors into the base class.
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
Attachment #8475375 - Attachment description: Review changes to add contain/cover for background-size and move a few more selectors into the base class. → PART 1: Review changes to add contain/cover for background-size and move a few more selectors into the base class.
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.
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.
Attachment #8475505 - Flags: review?(anthony)
Hi Tamara,
I would like to go with the one Without padding, Thank you!!
Flags: needinfo?(chuang) → needinfo?(thills)
Flags: needinfo?(anthony)
Comment on attachment 8475505 [details] [review]
Pull Request for squashed changes (parts 1-3)

r+ with two small comments to address before landing.
Attachment #8475505 - Flags: review?(anthony) → review+
Attachment #8475375 - Flags: review?(anthony)
Attached file Screens for review
Hi Carol,

Here are the screens for your review.

Thanks,

-tamara
Attachment #8476970 - Flags: ui-review?(chuang)
Comment on attachment 8476970 [details]
Screens for review

Hi Tamara, 
The buttons looks good! Thanks!
Attachment #8476970 - Flags: ui-review?(chuang) → ui-review+
Flags: needinfo?(thills)
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/0cb0ebfa4965f62f63dab5972650b0e11346ff4c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified with the test suite in comment #14 on today's master.
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.