Victoria Gerchinhoren [:vicky] Reporter
	Description • 10 years ago

Apply refresh to current DSDS implementation. Effort should be small, but there are many cases where the color or font style has changed as a result of the general VR.

Find attached the visual mockups and a PDF with the detailed specification of changes per screen.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 7 • 10 years ago

Reset the blocking-b2g flag to backlog.
This bug blocks the meta bug comms_2.0.

	Pau Masiá [:Pau]
	Comment 8 • 10 years ago

Update of header colors to match the visual design refresh

	Pau Masiá [:Pau]
	Comment 9 • 10 years ago

Updated header and message bubbles colors.

	Eric Chang [:ericcc] [:echang]
	Comment 13 • 10 years ago

Functional Regression test cases
http://mzl.la/1lOnQXn

	Julien Wajsberg [:julienw] Assignee
	Comment 14 • 10 years ago

The SIM indication was already 1.3rem, so I didn't change this. In the end, there was not much difference to what we had in v1.4, but I had to change some code because now the SIM indication's style is not the same than the button's style.

	Julien Wajsberg [:julienw] Assignee
	Comment 16 • 10 years ago

correct font

	Julien Wajsberg [:julienw] Assignee
	Comment 18 • 10 years ago

Comment on attachment 8433602 [details]
dsds-refresh-send-button-disabled.png

Sory, I see the separateor is not correct

	Julien Wajsberg [:julienw] Assignee
	Comment 19 • 10 years ago

Hey Vicky, it's not clear to me how the SIM indication is displayed when there is a subject line.

	Julien Wajsberg [:julienw] Assignee
	Comment 20 • 10 years ago

Here is a WIP PR.

I don't know how to make the separator bigger yet. I tried changing the max-height for the input but it makes other things be wrong:

* placeholder + input are too high
* I don't know exacty how it should look like when there is a subject and an empty input.

	Victoria Gerchinhoren [:vicky] Reporter
	Comment 21 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #19)
> Hey Vicky, it's not clear to me how the SIM indication is displayed when
> there is a subject line.

With the new send button this should not be a problem. See attachment.

	Julien Wajsberg [:julienw] Assignee
	Comment 23 • 10 years ago

Ok, then I won't focus on this and just move forward. Thanks Victoria !

	Julien Wajsberg [:julienw] Assignee
	Comment 24 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

Hey Steve,

this PR is essentially to fix the small regressions coming from previous patches.

I expect we'll need to do something else once we'll have the new button, but I'd like to have a clean state before that.

	Julien Wajsberg [:julienw] Assignee
	Comment 25 • 10 years ago

Oleg, if you work on DSDS support with the new button, please base your work on this PR too.

	Julien Wajsberg [:julienw] Assignee
	Comment 26 • 10 years ago

Another possibility is waiting that bug 1013296 lands to get this right.

	Julien Wajsberg [:julienw] Assignee
	Comment 27 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

Will wait for the other bugs.

	Steve Chung [:steveck]
	Comment 28 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Another possibility is waiting that bug 1013296 lands to get this right.

I think the proper landing sequence could be bug 1008127 which put counter to another element, and this patch to move the prefer-sim indicator from after to before element, and bug 1013296 for the last one because it need set the icon to the after element. WDYT?

	Julien Wajsberg [:julienw] Assignee
	Comment 29 • 10 years ago

I was thinking that it's easier to do the DSDS stuff right at the end, because people working on the other bugs (like Arnau) have no DSDS device to test this properly (even if with my awesome simulator it's easier ;) ).

So better break DSDS first (it's already broken anyway) and fix it then. My 2 cents :)

	Julien Wajsberg [:julienw] Assignee
	Comment 30 • 10 years ago

Unassigning myself until we finish the dependency and since I'm away next week.

	Stephany Wilkes
	Comment 31 • 10 years ago

Vicky, we are in a tough spot with how late this bug has gone on. It has missed feature landing by two full weeks now, and we were just reminded that there should no longer be ANY feature-b2g work in progress. The decision we need to make is whether we would stop 2.0 ship on this.

Let me know and thanks!

	Julien Wajsberg [:julienw] Assignee
	Comment 32 • 10 years ago

Stephany, we had quite a lot of things happening in the same area of code, and as a result we waited for the other bugs.

Now the path is clear and this should be resolved this week. Note that if it's not resolved we'll have blockers anyway, because DSDS is currently broken. So better land this soon, we all agree on this.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 33 • 10 years ago

Per comment 32 I updated the target milestone. Hope you don't mind.

	Julien Wajsberg [:julienw] Assignee
	Comment 38 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

Hey Anthony, Steve,

pending UI review, I'd like a first feedback on this.

There are 3 commits:
* first one is a first working implementation
* second one has the changes that I talked about with Anthony. I don't like that they change things in many apps, especially for a patch that I'll have to get an approval for. So Anthony, please tell me if you still prefer this approach.
* 3rd one is only moving some CSS from sms.css to compose.css, then it's easier to see the changes in the first commit.

Thanks !

	Anthony Ricaud (:rik)
	Comment 39 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

Looking at the code, I don't care which version we go with. They both look good to me. I'd prefer my approach if we had a way in the l10n lib to say "translate this with those arguments and use the existing l10n-id".

	Julien Wajsberg [:julienw] Assignee
	Comment 40 • 10 years ago

(In reply to Anthony Ricaud (:rik) from comment #39)
> Comment on attachment 8433627 [details] [review]
> github PR
> 
> Looking at the code, I don't care which version we go with.

I'll take the version with less changes then.

> They both look
> good to me. I'd prefer my approach if we had a way in the l10n lib to say
> "translate this with those arguments and use the existing l10n-id".

We can probably just set the good data attribute, but this is clumsy. I'll file a bug for our l10n colleagues about this.

	Steve Chung [:steveck]
	Comment 41 • 10 years ago

I think it's not possible to reproduce it in real case(maybe we still need to deal with in landscape mode), just wondering if we need to adjust our layout to avoid this rare case(like adjust the button height if we got prefer sim).

	Julien Wajsberg [:julienw] Assignee
	Comment 42 • 10 years ago

We moved from showing the counter when we had 10 characters left to showing it when we had 20 characters left (a partner request). Maybe we should use a middle ground: 15 characters left?

	Victoria Gerchinhoren [:vicky] Reporter
	Comment 43 • 10 years ago

Comment on attachment 8448812 [details]
2-line message with SIM indicator and character counter

Please, place center the counter text in the horizontal space reserved for the send button.

	Steve Chung [:steveck]
	Comment 44 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

Except the issue in attachment 8449220 [details] and Anthony's suggestion, the rest part looks fine. I'll leave this overlap concern to visual, since I'm not sure it's a good move to change the requirment from partner.

	Steve Chung [:steveck]
	Comment 45 • 10 years ago

Comment on attachment 8449220 [details]
Sim indicator overlap with the counter

Hi Vicky, do you have any suggestion that could simply adjust the position of each indicator to solve this overlap issue?

	Victoria Gerchinhoren [:vicky] Reporter
	Comment 46 • 10 years ago

Comment on attachment 8449220 [details]
Sim indicator overlap with the counter

Can you just make a higher area to include that number without overlapping? We should not have this situations, right?

	Julien Wajsberg [:julienw] Assignee
	Comment 47 • 10 years ago

(In reply to Steve Chung [:steveck] from comment #44)
> Comment on attachment 8433627 [details] [review]
> github PR
> 
> Except the issue in attachment 8449220 [details] and Anthony's suggestion,
> the rest part looks fine. I'll leave this overlap concern to visual, since
> I'm not sure it's a good move to change the requirment from partner.

look at bug 878603, it's not really a requirement, it was more a suggestion.

That said, I looked on my Peak v1.4, and we have a similar issue, so I suggest we handle this in a separate bug.

	Julien Wajsberg [:julienw] Assignee
	Comment 48 • 10 years ago

(In reply to Victoria Gerchinhoren [:vicky] from comment #46)
> Comment on attachment 8449220 [details]
> Sim indicator overlap with the counter
> 
> Can you just make a higher area to include that number without overlapping?
> We should not have this situations, right?

This is an edge case situation, but it might happen with real texts (I'll let you find some though :) ). The bigger the resolution is, the more likely the issue will happen.

About having an higher area, well, does that mean we should have a bigger input area in that case? Anyway, let's handle this separately as the issue exists in previous versions.

	Julien Wajsberg [:julienw] Assignee
	Comment 49 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

r? Steve for SMS and Anthony for the action button.

I pushed a separate commit for comment 43.

	Steve Chung [:steveck]
	Comment 50 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

I'm ok with firing a bug about the indicator after patch landed. There is a conflict in the html part but the changes is quite small, so r=me for the message part. Thanks!

	Anthony Ricaud (:rik)
	Comment 51 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

Good to go with the tests renamed.

	Julien Wajsberg [:julienw] Assignee
	Comment 52 • 10 years ago

fixed the tests and landed:

master: 1391064a106b66b02b0b7b9e7e43c9bc775ba586

	Julien Wajsberg [:julienw] Assignee
	Comment 53 • 10 years ago

Comment on attachment 8433627 [details] [review]
github PR

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 #): -
[User impact] if declined: DSDS regression with the previous visual refresh patches: we don't have the SIM information anymore.
[Testing completed]: Yes, both unit tests and on the device.
[Risk to taking this patch] (and alternatives if risky): low, mostly CSS changes, and few unit-tested JS changes.
[String changes made]: Yes

	Julien Wajsberg [:julienw] Assignee
	Comment 54 • 10 years ago

Filed bug 1034100 for the indicator overload issue.

	bhavana bajaj [:bajaj]
	Comment 55 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #53)
> Comment on attachment 8433627 [details] [review]
> github PR
> 
> 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 #): -
> [User impact] if declined: DSDS regression with the previous visual refresh
> patches: we don't have the SIM information anymore.
> [Testing completed]: Yes, both unit tests and on the device.
> [Risk to taking this patch] (and alternatives if risky): low, mostly CSS
> changes, and few unit-tested JS changes.
> [String changes made]: Yes

We need alternative solution here as we are way beyond the string freeze timeline if this has to resolve on 2.0.

	Julien Wajsberg [:julienw] Assignee
	Comment 56 • 10 years ago

Francesco, if I remove the "ariaLabel", does it still qualify as late-l10n since the other one should not be touched?

see https://github.com/azasypkin/gaia/commit/1391064a106b66b02b0b7b9e7e43c9bc775ba586#diff-d19e6e68e99e0ef8ddad10db5fcab668R245

Eitan, how removing the aria-label here would impair the accessibility? Is it good enough to go in v2.0 considering the general accessibility state of the application?

	Francesco Lodolo [:flod]
	Comment 57 • 10 years ago

(In reply to Julien Wajsberg [:julienw] from comment #56)
> Francesco, if I remove the "ariaLabel", does it still qualify as late-l10n
> since the other one should not be touched?

Yes. It becomes a new id, therefore a new string.

Any chance to set the ariaLabel attribute with js at runtime on 2.0? I think kgrandon did something like this in another patch to work around the string change.

	Axel Hecht [:Pike]
	Comment 58 • 10 years ago

That was bug 1030248, https://github.com/mozilla-b2g/gaia/pull/21244/files

	Francesco Lodolo [:flod]
	Comment 59 • 10 years ago

Talked with Julien on IRC, I didn't actually answered the question.

His proposal is to drop completely the string .ariaLabel on 2.0
> send-button-sim-indication.ariaLabel = using SIM {{n}}

So I actually looked at the patch, and I'm not sure why we have the following string exposed to localization, with a note to not localize it. Is that because it's needed to have an element with .ariaLabel? That would be strange.
> send-button-sim-indication = {{n}}

I'd personally be fine with not having send-button-sim-indication localizable. For the accessibility problem I'm not the right person to answer, but maybe we could reuse sim-name (SIM1, SIM2) on 2.0 to avoid a new string and loosing completely an Aria label.

	Julien Wajsberg [:julienw] Assignee
	Comment 60 • 10 years ago

The main issue here is that we share a component with the Dialer and Contacts app. That component is using the l10n library to populate the text, depending on the SIM id being configured. That's why there is a seemingly useless l10n string "send-button-sim-indication".

The component is not using the same variable name than sim-name ("n" instead of "id") so we can't reuse it without doing code change.

Also, since the shared component is doing the text change, we can't easily add a workaround to use sim-name with the good parameter. The only way (that I see) would be to redo what the shared component is doing (wrt the displayed text) in SMS. I'd rather not do this as it means more code, so riskier patch, while I tried very hard to have a very simple patch here.


Hope this makes sense.

	Francesco Lodolo [:flod]
	Comment 61 • 10 years ago

It makes sense, but it leaves us with the main problem: this patch adds new strings, either 1 or 2 depending on the approach, and that's way too late for that (note also that his call is not mine to make).

	Julien Wajsberg [:julienw] Assignee
	Comment 62 • 10 years ago

My main question for you is this: is

> send-button-sim-indication = {{n}}

(where we don't want to change it in any localization) still late-l10n? I mean, what's the work that needs to be done by the l10n team if we introduce this not-localized string?

I'm trying to understand, and maybe I can even help if this can keep the code safer.

	Francesco Lodolo [:flod]
	Comment 63 • 10 years ago

Personally, I don't expect locales to change that string, that's why I even suggested that it shouldn't be a localizable string.

We could:
* expose the new string to localization, it will be reported as missing, locales can decide what to do, but in this case falling back to en-US is safe.
* not expose the new string to localization. That, I think, would be a serious pain for the automation in charge of extracting strings (and that's a question for Pike).

I don't like any of those.

The ideal solution, given comment 60, would still be to create an exception for send-button-sim-indication (not rely on l10n.js, set it with JavaScript at run-time). Not much else that I can add.

	Axel Hecht [:Pike]
	Comment 64 • 10 years ago

Here's a hack, create a single-entry non-localized locale-like properties file with the hard coded string, something like apps/sms/hacky-2.0.properties, and include that in the head, without an INI file.

That could get you the hard coded string in the l10n context, without exposing it to localizers.

	Julien Wajsberg [:julienw] Assignee
	Comment 65 • 10 years ago

Here is what I came up with: 
https://github.com/julienw/gaia/commit/db439c6eb94c13e62f3362eb8f2b74711fbc6de7

Seems to work locally, I'll test on a device now.

	Julien Wajsberg [:julienw] Assignee
	Comment 66 • 10 years ago

Hey Pike,

is it what you had in mind? Would this remove the "late-l10n" flag from this patch?

	Axel Hecht [:Pike]
	Comment 67 • 10 years ago

Comment on attachment 8450952 [details] [review]
v2.0 Github PR

Yes, this is what I had in mind, and this would un-late-l10n this for me.

I'd prefer to go a step further still and move the hacky.properties outside of the locales dir, just to be extra explicit that that file shouldn't be localized.

Should be OK either way, though, my code uses

_name.endswith('.en-US.properties')

to detect if it should track a file in the l10n repo or not.

	Julien Wajsberg [:julienw] Assignee
	Comment 68 • 10 years ago

(In reply to Axel Hecht [:Pike] from comment #67)
> Comment on attachment 8450952 [details] [review]
> v2.0 Github PR
> 
> Yes, this is what I had in mind, and this would un-late-l10n this for me.
> 
> I'd prefer to go a step further still and move the hacky.properties outside
> of the locales dir, just to be extra explicit that that file shouldn't be
> localized.
> 

I've mixed feelings. What you say makes sense from the localizer point of view, but from the developer point of view, we want all properties to be in one directory :)

So I'll keep it like this.

	Julien Wajsberg [:julienw] Assignee
	Comment 69 • 10 years ago

Comment on attachment 8450952 [details] [review]
v2.0 Github PR

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 #): -
[User impact] if declined: DSDS regression with the previous visual refresh patches: we don't have the SIM information anymore.
[Testing completed]: Yes, both unit tests and on the device.
[Risk to taking this patch] (and alternatives if risky): low, mostly CSS changes, and few unit-tested JS changes.
[String changes made]: No

	Ryan VanderMeulen [:RyanVM]
	Comment 70 • 10 years ago

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

	Eitan Isaacson [:eeejay]
	Comment 71 • 10 years ago

Clearing ni flag. Looks like no aria-label was removed?

	Julien Wajsberg [:julienw] Assignee
	Comment 72 • 10 years ago

Eitan: yes we removed it for the v2.0 patch. I mean, we didn't add it in the v2.0 like we did in the master patch.

	Loli (:lolimartinezcr)
	Comment 74 • 10 years ago

Pending bugs: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1036296
https://bugzilla.mozilla.org/show_bug.cgi?id=1036308
https://bugzilla.mozilla.org/show_bug.cgi?id=1036316

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 75 • 10 years ago

https://moztrap.mozilla.org/manage/case/11762/
https://moztrap.mozilla.org/manage/case/11748/
https://moztrap.mozilla.org/manage/case/11743/
https://moztrap.mozilla.org/manage/case/10672/

	Alissa（leave from Mozilla）
	Comment 76 • 10 years ago

The position Add attachments icon is different between 2.1 and 2.0,Please check which one is needed.
FLame 2.0 new build:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0

Flame 2.1 new build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0