1013296 - Compose. Change send button to an paper plane icon

Status: VERIFIED FIXED

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

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: COmpose with new send button.

In order to match the email app send button, and to avoid spacing issues in different languages please replace the word "Send" for a paper plane icon.

This will also be very helpful when aligning character counter and MMS indicator.

Comment 1

[Pau Masiá [:Pau]]

Attachment: Assets. PNG. Sms. Send icon

Here are the assets.

Comment 2

[Stephany Wilkes]

How is implementation coming on this one? Sprint 3 ends Friday. Thanks!

Comment 3

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Working on it. 
The icon looks a bit larger than expected and need to fine-tune it with Pau.
I'll send a PR for review today.

Comment 4

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Attachment: patch in github

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8433972 [details] [review]
patch in github

I left some comments on github, but I think we should wait for Bug 990537 first. Besides that, a major issue is indicator will disappear after this background image change. Please ask for review again after rebasing based on bug 990537 properly, thanks.

Comment 6

[Julien Wajsberg [:julienw]]

After thinking more, I now think this bug should land before bug 990537. I'll then do whatever needed to make DSDS work fine in bug 990537, so don't spend time on making DSDS work here.

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8433972 [details] [review]
patch in github

Based on comment in https://github.com/mozilla-b2g/gaia/pull/19991/files#discussion_r13487895, I think we should wait at least for Bug 1008127 to have a proper place for send icon. If you want to land this patch first, maybe you could try to set the icon to send button background directly, but it seems violate the rule in building block...

Comment 8

[Julien Wajsberg [:julienw]]

I was told by Yura from the Accessibility team that it's better to use aria-label instead of font-size or text-indent for adding the label for screen readers.

Comment 9

[Julien Wajsberg [:julienw]]

Could be a good idea to wait for bug 1015867 too. I'm changing how the send button is positioned (using a flex positioning instead of the current "postion: fixed").

Comment 10

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Cool Julien, so I'll wait for your patch to land :)
About the accessibility thing:
is aria-label translated?
Because if it's not, my guess is having the text hidden with a l10n id would be a better option. WDYT?
Also adding Yura here for his input :)

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to Arnau March  [:arnau] from comment #10)
> About the accessibility thing:
> is aria-label translated?

Yes, aria-label is translated. See various bugs dependent on bug 893789, for example for the status bar etc., for the technique used.

> Also adding Yura here for his input :)

Taking the liberty of answering on his behalf instead. :)

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Marco Zehe (:MarcoZ) from comment #11)
> (In reply to Arnau March  [:arnau] from comment #10)
> > About the accessibility thing:
> > is aria-label translated?
> 
> Yes, aria-label is translated. See various bugs dependent on bug 893789, for
> example for the status bar etc., for the technique used.
> 
> > Also adding Yura here for his input :)
> 
> Taking the liberty of answering on his behalf instead. :)

Yes, what Marco said: 

{data-l10n-id}.ariaLanel = Your String

in the properties will do it.

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Sorry, I meant {data-l10n-id}.ariaLabel

Comment 14

[Stephany Wilkes]

Please update the status on this. Feature landing was two weeks ago and no more feature work is supposed to be in progress. We need to make a call on whether or not this is getting in.

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

Arnau is this patch ready for ui-review and r?

Comment 16

[Maria Angeles Oteo (:oteo)]

(In reply to Stephany Wilkes from comment #14)
> Please update the status on this. Feature landing was two weeks ago and no
> more feature work is supposed to be in progress. We need to make a call on
> whether or not this is getting in.

Hi Stephany, Arnau couldn't land this patch because it depended on bug 1008127 (See comment 7), today this dependency was removed so tomorrow (today is local holiday in Barcelona) Arnau will ask for review and I hope that the patch will be merged.
Thanks!

Comment 17

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8433972 [details] [review]
patch in github

Steve,
I have rebased my patch and changed the icon so it does not interfere with pseudo-elements in the button.

Comment 18

[Steve Chung [:steveck]]

Comment on attachment 8433972 [details] [review]
patch in github

One small issue here:https://github.com/mozilla-b2g/gaia/pull/19991/files#discussion_r14232064 but I think overall is good. Please ask review again when you are ready, thanks!

Comment 19

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Attachment: send.png

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8433972 [details] [review]
patch in github

Looks great! r=me, thanks!

Comment 21

[Steve Chung [:steveck]]

Travis is red because of other module failed. I restart the test but it should be fine if the test is still failed...

Comment 22

[Steve Chung [:steveck]]

Since we already created the corresponding bugs for failed tests(bug 1030764 in everyting.me and bug 1032037 in music). It should be fine to land this patch.

In master: 345321f1d5d8a42a49e6818e9b0dd9a3258af2ef

Comment 23

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Thanks Steve ;)

Comment 24

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8433972 [details] [review]
patch in github

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): new feature
[User impact] if declined: not following visual refresh targeted for 2.0
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]:

Comment 25

[Julien Wajsberg [:julienw]]

Comment on attachment 8433972 [details] [review]
patch in github

Note to relman: This bug was a late request from visual design team. I'd really want this patch to be uplifted to v2.0 because:
* it will make it easier to work on bug 990537 (otherwise we'll need 2 separate patches for it)
* it will improve i18n (because we don't show any text now -- except for screenreader users)
* it's quite small (mostly CSS)

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 8433972 [details] [review]
patch in github

Thanks for the additional info, the risk should be manageable given the changes. Approving for uplift.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Loli (:lolimartinezcr)]

Tested and working
Hamachi
2.0
Gecko-7a31c32
Gaia-1bd6e89

Hamachi
2.1
Gecko-6777a8c
Gaia-a12ea45.

Comment 29

[Coler]

This issue has been verified successfully on Flame2.0&2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_Send.mp4

Flame2.0 build version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141130000204
Version         32.0

Flame2.1 build version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0

Comment 30

[Coler]

Attachment: Verify_Flame_Send.MP4