Closed Bug 990537 Opened 6 years ago Closed 5 years ago

[DSDS] Messaging. Apply Visual Refresh to DSDS scenarios.

Categories

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

x86
macOS
defect
Not set

Tracking

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

VERIFIED FIXED
2.0 S5 (4july)
feature-b2g 2.0
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: vicky, Assigned: julienw)

References

Details

(Whiteboard: [p=2])

Attachments

(15 files, 10 obsolete files)

20.16 KB, image/png
Details
24.85 KB, image/png
Details
36.68 KB, image/png
Details
97.89 KB, image/png
Details
1.05 MB, application/pdf
Details
46 bytes, text/x-github-pull-request
steveck
: review+
rik
: review+
Details | Review
19.76 KB, image/png
Details
46.45 KB, image/png
vicky
: ui-review+
Details
30.91 KB, image/png
vicky
: ui-review-
Details
45.53 KB, image/png
vicky
: ui-review+
Details
40.59 KB, image/png
vicky
: ui-review+
Details
51.54 KB, image/png
vicky
: feedback-
Details
46 bytes, text/x-github-pull-request
Pike
: feedback+
Details | Review
5.53 MB, video/3gpp
Details
9.22 MB, video/3gpp
Details
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.
Attached image Visual Mockup.DSDS.Messages Inbox (obsolete) —
Attached image Visual Mockup.DSDS.Messages Sim Picker (obsolete) —
Attachment #8399955 - Attachment description: VDR Mockup: Compose. 4 recipients → Visual Mockup.DSDS.Compose.4 recipients
Attachment #8399957 - Attachment description: VDR Mockup: Messages thread idle mode → Visual Mockup.DSDS.Messages thread idle mode
Attachment #8399959 - Attachment description: VDR Mockup: Messages Inbox → Visual Mockup.DSDS.Messages Inbox
Attachment #8399961 - Attachment description: VDR Mockup: Messages Sim Picker → Visual Mockup.DSDS.Messages Sim Picker
Attachment #8399953 - Attachment description: Visual refresh Spec for Messaging → Visual refresh Spec for DSDS Messaging
Attached file Visual refresh Spec for DSDS Messaging (obsolete) —
Attachment #8399953 - Attachment is obsolete: true
Assignee: nobody → schung
blocking-b2g: --- → 2.0?
Reset the blocking-b2g flag to backlog.
This bug blocks the meta bug comms_2.0.
blocking-b2g: 2.0? → backlog
QA Contact: echang
Update of header colors to match the visual design refresh
Attachment #8399955 - Attachment is obsolete: true
Updated header and message bubbles colors.
Attachment #8399957 - Attachment is obsolete: true
Attachment #8399959 - Attachment is obsolete: true
Attachment #8399961 - Attachment is obsolete: true
Attachment #8399968 - Attachment is obsolete: true
Attachment #8417339 - Attachment description: Visual Mockup.DSDS.Compose.4 recipients → Visual Mockup.DSDS.Compose.4 recipients (N)
Attachment #8417342 - Attachment description: Visual Mockup.DSDS.Messages thread idle mode → Visual Mockup.DSDS.Messages thread idle mode (N)
Attachment #8417343 - Attachment description: Visual Mockup.DSDS.Messages Inbox → Visual Mockup.DSDS.Messages Inbox (N)
Attachment #8417344 - Attachment description: Visual Mockup.DSDS.Messages Sim Picker → Visual Mockup.DSDS.Messages Sim Picker (N)
Attachment #8417346 - Attachment description: Visual refresh Spec for DSDS Messaging → Visual refresh Spec for DSDS Messaging (N)
feature-b2g: --- → 2.0
QA Contact: echang
Depends on: 963013
Flags: in-moztrap?(nhirata.bugzilla)
Blocks: sms-sprint-2
Target Milestone: --- → 2.0 S3 (6june)
Whiteboard: [p=2]
Assignee: schung → felash
QA Contact: echang
Functional Regression test cases
http://mzl.la/1lOnQXn
Summary: [DSDS] [Meta] Messaging. Apply Visual Refresh to DSDS scenarios. → [DSDS] Messaging. Apply Visual Refresh to DSDS scenarios.
Attached image dsds-refresh-send-button-disabled.png (obsolete) —
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.
Attachment #8433589 - Flags: ui-review?(vpg)
Attached image dsds-refresh-send-button-enabled.png (obsolete) —
Attachment #8433590 - Flags: ui-review?(vpg)
Attached image dsds-refresh-send-button-disabled.png (obsolete) —
correct font
Attachment #8433589 - Attachment is obsolete: true
Attachment #8433589 - Flags: ui-review?(vpg)
Attached image dsds-refresh-send-button-enabled.png (obsolete) —
Attachment #8433590 - Attachment is obsolete: true
Attachment #8433590 - Flags: ui-review?(vpg)
Attachment #8433604 - Flags: ui-review?(vpg)
Attachment #8433602 - Flags: ui-review?(vpg)
Comment on attachment 8433602 [details]
dsds-refresh-send-button-disabled.png

Sory, I see the separateor is not correct
Attachment #8433602 - Attachment is obsolete: true
Attachment #8433602 - Flags: ui-review?(vpg)
Attachment #8433604 - Attachment is obsolete: true
Attachment #8433604 - Flags: ui-review?(vpg)
Hey Vicky, it's not clear to me how the SIM indication is displayed when there is a subject line.
Flags: needinfo?(vpg)
Attached file github PR
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.
(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.
Flags: needinfo?(vpg)
Attached image DSDS.sms_compose.png
Ok, then I won't focus on this and just move forward. Thanks Victoria !
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.
Attachment #8433627 - Flags: review?(schung)
Oleg, if you work on DSDS support with the new button, please base your work on this PR too.
Another possibility is waiting that bug 1013296 lands to get this right.
Blocks: 1013296
No longer blocks: 1013296
Depends on: 1013296
Depends on: 1008127
Comment on attachment 8433627 [details] [review]
github PR

Will wait for the other bugs.
Attachment #8433627 - Flags: review?(schung)
(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?
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 :)
Status: NEW → ASSIGNED
Unassigning myself until we finish the dependency and since I'm away next week.
Assignee: felash → nobody
Depends on: 1026369
No longer depends on: 1026369
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!
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.
Assignee: nobody → felash
Keywords: late-l10n
Per comment 32 I updated the target milestone. Hope you don't mind.
Target Milestone: 2.0 S3 (6june) → 2.0 S5 (4july)
Attachment #8448811 - Flags: ui-review?(vpg)
Attachment #8448813 - Flags: ui-review?(vpg)
Attachment #8448815 - Flags: ui-review?(vpg)
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 !
Attachment #8433627 - Flags: feedback?(schung)
Attachment #8433627 - Flags: feedback?(anthony)
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".
Attachment #8433627 - Flags: feedback?(anthony)
(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.
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).
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?
Attachment #8448815 - Flags: ui-review?(vpg) → ui-review+
Attachment #8448813 - Flags: ui-review?(vpg) → ui-review+
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.
Attachment #8448812 - Flags: ui-review?(vpg) → ui-review-
Attachment #8448811 - Flags: ui-review?(vpg) → ui-review+
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.
Attachment #8433627 - Flags: feedback?(schung)
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?
Attachment #8449220 - Flags: feedback?(vpg)
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?
Attachment #8449220 - Flags: feedback?(vpg) → feedback-
(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.
(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.
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.
Attachment #8433627 - Flags: review?(schung)
Attachment #8433627 - Flags: review?(anthony)
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!
Attachment #8433627 - Flags: review?(schung) → review+
Comment on attachment 8433627 [details] [review]
github PR

Good to go with the tests renamed.
Attachment #8433627 - Flags: review?(anthony) → review+
fixed the tests and landed:

master: 1391064a106b66b02b0b7b9e7e43c9bc775ba586
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8433627 - Flags: approval-gaia-v2.0?(bbajaj)
Filed bug 1034100 for the indicator overload issue.
(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.
Attachment #8433627 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0-
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?
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(eitan)
(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.
Flags: needinfo?(francesco.lodolo)
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.
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.
Flags: needinfo?(francesco.lodolo)
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).
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
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.
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.
Attached file v2.0 Github PR
Hey Pike,

is it what you had in mind? Would this remove the "late-l10n" flag from this patch?
Attachment #8450952 - Flags: feedback?(l10n)
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.
Attachment #8450952 - Flags: feedback?(l10n) → feedback+
(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.
Keywords: late-l10n
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
Attachment #8450952 - Flags: approval-gaia-v2.0?(bbajaj)
Attachment #8450952 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Clearing ni flag. Looks like no aria-label was removed?
Flags: needinfo?(eitan)
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.
Duplicate of this bug: 1036296
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap+
Attached video flame2.0.3gp
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
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.