The goal of this bug is to update the styles of the Composer. The markup will be modified in bug https://bugzilla.mozilla.org/show_bug.cgi?id=949457 . This probably needs some tweaks in Building Blocks as well.

Hi Julien, I'm removing the dependency here because after talking with Joan we are going to do the refactoring in 2 steps: - Fist one (this one) is *only* related with styles that does not affect the current composer (I mean markup and JS). This is only related with a refresh of the colors, fonts... That's why it's not blocked by the refactoring of the markup, and the deadline is the 31/1 - As second step, we are going to work on creating the new markup, and modify the JS related with the composer working quite close to Ayman, for ensuring that this refactor follows all requirements in the WF. The goal for this task (is a complicated one) it's 14/2. I hope it helps!

Good for me, I was only reflecting what you were saying in comment 0 ;)

Joan asked me about these cases (MMS indicator & character counter), I did the corresponding visual for this elements in the visual refresh.

Several comments: https://github.com/mozilla-b2g/gaia/pull/15947/files

Hi Rick, I applied the corrections

Joan, is it WIP or not? If it's WIP then it's not ready for review, right ? :) But anyway thanks for your work!

Hi Julien, yes it's WIP and the review is assigned to Borja, because he work with me on visual refresh. I talk with Borja

Comment on attachment 8370122 [details] WIP, SMS Compose patch This bug needs a new PR attached, and It has been reduced in priority. Let me know when we are back on this!

VD refresh issues are not blockers for the release, so not blocking.

Comment on attachment 8403475 [details] VR_Message_Composer.zip Thanks. A few comments though: 1. Can we please change hitstates to #b2f2ff? 2. Charachters left indicator should not be italic 3. Put "MMS" indicator in the same place an color of charachter indicator 4. Input text looks bold, check that it is regular style (placeholeder text also looks bolder than it should be) 5. Align file thumbnail in input area to the cursor baseline 6. Make suggestions background same color as "to field" area.

This shows the style with more than one line. Style is not working as expected.

Banner is not aligned with the bottom of the recipients layer.

Comment on attachment 8403473 [details] Path in github I've found some issues: - Banner: https://bugzilla.mozilla.org/attachment.cgi?id=8403966 - Recipients alignment: https://bug963018.bugzilla.mozilla.org/attachment.cgi?id=8403965 - Some CSS styles in 'px' instead of 'rem'. Could you take a look? Ask me to take a look again when ready! THanks!

Victoria, for accessibility reason, I'm worried that the "invalid" state for a recipient is conveyed only with colors instead of pictogram like we had before. Someone that do not see colors will have a lot of difficulties to see this. This is something I raised to José when he did his first proposal too, and that's why he came up with using a "!" in addition to colors.

Comment on attachment 8403473 [details] Path in github Not much more than what Borja said, except that the recipient line, when empty, is not well vertically centered. I'm also concerned about the invalid recipients design (I didn't raise this earlier because I couldn't really point what was disturbing me, and I realized only now, so sorry about this :( ).

(In reply to Julien Wajsberg [:julienw] from comment #23) > Victoria, for accessibility reason, I'm worried that the "invalid" state for > a recipient is conveyed only with colors instead of pictogram like we had > before. Someone that do not see colors will have a lot of difficulties to > see this. This is something I raised to José when he did his first proposal > too, and that's why he came up with using a "!" in addition to colors. Hi Julien, I know about that conversation, and I think color blindness is not as bad as we some times put it. But in any case, there's no harm in putting back the "!" sign. JOan is already on it. Find attached the new version of this field.

(In reply to Julien Wajsberg [:julienw] from comment #24) > Comment on attachment 8403473 [details] > Path in github > > Not much more than what Borja said, except that the recipient line, when > empty, is not well vertically centered. you mean the caret cursor? > I'm also concerned about the invalid recipients design (I didn't raise this > earlier because I couldn't really point what was disturbing me, and I > realized only now, so sorry about this :( ).

(In reply to Joan Leon [:joan] from comment #28) > (In reply to Julien Wajsberg [:julienw] from comment #24) > > Comment on attachment 8403473 [details] > > Path in github > > > > Not much more than what Borja said, except that the recipient line, when > > empty, is not well vertically centered. > > you mean the caret cursor? > No, I mean the whole line is not verically aligned inside the grey zone. (In reply to Victoria Gerchinhoren [:vicky] from comment #25) > But in any case, there's no harm in putting back the "!" sign. Thanks !

Comment on attachment 8403473 [details] Path in github Adding Julien as well! Waiting until the PR will be rebased for adding my comments.

Borja, is this ready ?

Comment on attachment 8403473 [details] Path in github I revised the UI with Joan offline (side by side) looks OK. Thanks Joan for the hard work!

Comment on attachment 8403473 [details] Path in github I think this is a great step forward, but needs some work yet. There are some scenarios (overall when the phone number or name of the recipient produces an overflow), when the UI is not working as we expected from the visual refresh. In this case, probably there is some JS which needs to be modified, or CSS should work in a different manner. If you need help with the JS dont hesitate to ask me! I'll help you for sure! :)

Victoria, the spec doesn't say where the character counter should be if the user scrolls the message. After bug 949457 lands, the subject and the message scroll together (as it should have been since the start), and so I don't know how we should align the "MMS" indicator. Could you please help?

Comment on attachment 8403473 [details] Path in github Gave more comments on github thanks !

Note that I uploaded spec for the Subject's different scenarios.

Victoria, sorry, it's still not clear to me. Again, now, the subject and the message text scroll together, in a "combined" way. So that means that if you scroll down, the subject goes out of the composer and is invisible. Should the "MMS" indicator scroll with the subject, or should it stay in the same place? And another question :) If there are some pictures in the message (so it's a MMS), but there is no subject, I expect the "MMS" indicator to be aligned with the top of the composer (like the character counter is). But if there are some pictures and the user toggles the subject without writing anything in the subject yet; should the "MMS" indicator be aligned with the subject-to-be, or should it stay aligned with the text/pictures?

Hi Julien, Can we define the theme of subject as another bug (that blocks this bug)?

I don't mind, but we still need the information.

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #38) > Victoria, sorry, it's still not clear to me. > > Again, now, the subject and the message text scroll together, in a > "combined" way. So that means that if you scroll down, the subject goes out > of the composer and is invisible. Should the "MMS" indicator scroll with the > subject, or should it stay in the same place? Really? The subject goes off screen? I thought it would stick to the top when reaching it and the message would scroll behind it, this sounds more logical to me. But we can always ask IA to give extra input here. So NI Omega > > And another question :) If there are some pictures in the message (so it's a > MMS), but there is no subject, I expect the "MMS" indicator to be aligned > with the top of the composer (like the character counter is). But if there > are some pictures and the user toggles the subject without writing anything > in the subject yet; should the "MMS" indicator be aligned with the > subject-to-be, or should it stay aligned with the text/pictures? I think in that case, the best is that the MMS indicator stays in the same place with a sorter line until the user types something in the subject field, when the user starts writing, the line will grow to reach the undelining lenght.

(In reply to Victoria Gerchinhoren [:vicky] from comment #41) > (In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment > #38) > > Victoria, sorry, it's still not clear to me. > > > > Again, now, the subject and the message text scroll together, in a > > "combined" way. So that means that if you scroll down, the subject goes out > > of the composer and is invisible. Should the "MMS" indicator scroll with the > > subject, or should it stay in the same place? > > Really? The subject goes off screen? I thought it would stick to the top > when reaching it and the message would scroll behind it, this sounds more > logical to me. This was the previous behavior but Ayman prefered to scroll the subject along with the message; I think the rationale is that if the user wants to scroll down, he wants to see more of the message.

(In reply to Victoria Gerchinhoren [:vicky] from comment #41) > Really? The subject goes off screen? I thought it would stick to the top > when reaching it and the message would scroll behind it, this sounds more > logical to me. But we can always ask IA to give extra input here. > So NI Omega Yeah, I think it's better for subject to go off screen to leave more space for message input. The subject is not that important to be always visible.

(In reply to Omega Feng [:Omega] from comment #43) > (In reply to Victoria Gerchinhoren [:vicky] from comment #41) > > Really? The subject goes off screen? I thought it would stick to the top > > when reaching it and the message would scroll behind it, this sounds more > > logical to me. But we can always ask IA to give extra input here. > > So NI Omega > > Yeah, I think it's better for subject to go off screen to leave more space > for message input. The subject is not that important to be always visible. Yes, I agree that subject is not that important, but for me the important thing is to show the user how adding subject to a regular SMS will turn it into a MMS being a much more expensive type of message in our target countries. So the idea of the MMS indicator to be next to the subject when there is so, is just to somehow advice -with a visual connection of elements- that one of the reasons of being an MMS is that Subject addition (a user already knows that adding a file will cost more, the second situation of having a really long message turning into an MMS is already communicated by an in app message and this third way to convert the message into an MMS is the one left to be indicated). So if that whole element can stay there, it would be a good solution. If you can think of a better one, I am open, but would like to have that connection somehow. Thanks!

Created the visual mockup of the new MMS in-app message

(In reply to Victoria Gerchinhoren [:vicky] from comment #44) > (In reply to Omega Feng [:Omega] from comment #43) > Yes, I agree that subject is not that important, but for me the important > thing is to show the user how adding subject to a regular SMS will turn it > into a MMS being a much more expensive type of message in our target > countries. So the idea of the MMS indicator to be next to the subject when > there is so, is just to somehow advice -with a visual connection of > elements- that one of the reasons of being an MMS is that Subject addition > (a user already knows that adding a file will cost more, the second > situation of having a really long message turning into an MMS is already > communicated by an in app message and this third way to convert the message > into an MMS is the one left to be indicated). For your concern, I agree to leave Subject input there with MMS indicator.

Comment on attachment 8403473 [details] Path in github Re-enabling the flag to r? to Julien!

(In reply to Omega Feng [:Omega] from comment #48) > (In reply to Victoria Gerchinhoren [:vicky] from comment #44) > > (In reply to Omega Feng [:Omega] from comment #43) > > Yes, I agree that subject is not that important, but for me the important > > thing is to show the user how adding subject to a regular SMS will turn it > > into a MMS being a much more expensive type of message in our target > > countries. So the idea of the MMS indicator to be next to the subject when > > there is so, is just to somehow advice -with a visual connection of > > elements- that one of the reasons of being an MMS is that Subject addition > > (a user already knows that adding a file will cost more, the second > > situation of having a really long message turning into an MMS is already > > communicated by an in app message and this third way to convert the message > > into an MMS is the one left to be indicated). > > For your concern, I agree to leave Subject input there with MMS indicator. Then we'll need to handle this in a separate bug, thanks. Joan, can you please file a new bug for the subject handling, as we discussed in comment 39?

> Joan, can you please file a new bug for the subject handling, as we > discussed in comment 39? done!, please refer to Bug 1008127

Comment on attachment 8403473 [details] Path in github Some comments in Github! Take a look and let me know when ready! Great job Joan! :)

Comment on attachment 8403473 [details] Path in github This is going well but there is still some work to do. Added some comments on github. See also Victoria's answer in in bug 950175 comment 59 (which is the behavior that this patch shows, so it's already perfect ;) ).

Why do you ask the review again but you didn't fix the various comments?

(In reply to Julien Wajsberg [:julienw] from comment #58) > Why do you ask the review again but you didn't fix the various comments? Julien, I guess Joan has fixed what it is in his hands. As commented in github he is asking any of you for help on the javascript side. This is a huge bug, and it would be great if someone in your team could help him land it.

That's not true: a lot of the comments Borja and I made are in the CSS. Only a small part is in the JS. There are also some questions where we just need answers because, well, we're not in Joan's head and sometimes we just don't understand why some things are made this way. Also, I think the JS side should really be removed and transformed to a CSS-only solution. Especially, I'd like to know what part of the design the JS changes are trying to solve, and then we can either try to solve the issue or discuss with Vicky to find another closer design. Honestly, I think Joan and you have more skills than me to answer these questions, but someone from our team can definitely help. Hope this helps!

(In reply to Julien Wajsberg [:julienw] from comment #60) > That's not true: a lot of the comments Borja and I made are in the CSS. Only > a small part is in the JS. Sorry for that I just went through the comments on github and did not verified the PR updates. I trusted Joan would have fixed what was in his hands. > There are also some questions where we just need answers because, well, > we're not in Joan's head and sometimes we just don't understand why some > things are made this way. > > Also, I think the JS side should really be removed and transformed to a > CSS-only solution. Especially, I'd like to know what part of the design the > JS changes are trying to solve, and then we can either try to solve the > issue or discuss with Vicky to find another closer design. > > Honestly, I think Joan and you have more skills than me to answer these > questions, but someone from our team can definitely help. I have not been on top of all changes as I'm refreshing all apps but SMS and dialer. But I'll try to work closer with Joan to get more involved in SMS. > Hope this helps!

Hi, I update de PR with css changes, I add comment about JS https://github.com/mozilla-b2g/gaia/pull/18088#discussion_r12611472

There are still a lot of comments that are not addressed but let's focus on the JS issue for now. Oleg, would you have an idea on how to support these mockups: attachment 8394786 [details] and attachment 8394787 [details] using CSS only? A difficult part is to be able to make the composer height stop at the bottom of the recipients panel (but maybe we can cheat and always make it stop at the bottom of the bigger-style panel)

(In reply to Julien Wajsberg [:julienw] from comment #63) > There are still a lot of comments that are not addressed but let's focus on > the JS issue for now. > > Oleg, would you have an idea on how to support these mockups: attachment > 8394786 [details] and attachment 8394787 [details] using CSS only? A > difficult part is to be able to make the composer height stop at the bottom > of the recipients panel (but maybe we can cheat and always make it stop at > the bottom of the bigger-style panel) Looking at mockups I'm wondering why can't we use flex-boxes here, where top (recipients) and bottom(input) blocks can grow and middle block will occupy the rest of the space? I don't know this particular markup well though, maybe it's difficult to change.

(In reply to Oleg Zasypkin [:azasypkin] from comment #64) > (In reply to Julien Wajsberg [:julienw] from comment #63) > > There are still a lot of comments that are not addressed but let's focus on > > the JS issue for now. > > > > Oleg, would you have an idea on how to support these mockups: attachment > > 8394786 [details] and attachment 8394787 [details] using CSS only? A > > difficult part is to be able to make the composer height stop at the bottom > > of the recipients panel (but maybe we can cheat and always make it stop at > > the bottom of the bigger-style panel) > > Looking at mockups I'm wondering why can't we use flex-boxes here, where top > (recipients) and bottom(input) blocks can grow and middle block will occupy > the rest of the space? I don't know this particular markup well though, > maybe it's difficult to change. If we use flex-box for the list of recipients, the behavior of the pills does not behave as desired.

Several comments: https://github.com/mozilla-b2g/gaia/pull/18088

Just to note that I gave a lot of thought to this between yesterday and today (yeah, while I was trying to sleep ;) ) and I think we should leave the JS part out of this patch and move it to another bug (like we did for the subject handling). So that we can move forward with the bigger changes, and maybe rediscuss with Victoria for this specific issue. I couldn't find a CSS only solution, but I'll outline the requirements we have for this panel on the other bug so maybe someone will be able to help. Would that work for you Joan?

Okay for me, I think you'd better split, because this bug is becoming a meta.

So Joan, can you update your current patch with removing all things related to the recipient height handling between 1 line and more than 1 line? I think we can keep the current behavior for now, using the 1-line-and-a-half display, even if we have only 1 line of recipients.

Ok, working on it and I update the PR

Ok, working on it and I update the PR

I updated the Patch, now the recipients container only shows one line. https://github.com/mozilla-b2g/gaia/pull/18088

I haven't finished completely the review but still added some comments on github. Also, there is an issue when the send button is bigger (for example in french) but that should be fine when we switch to an icon, so let's not take care of this.

Victoria, I just think we miss an important piece of information: how a currently-edited recipient looks like. Historically it used to look like an "assimilated" recipient, only without the "recipient pill". Should it be the same with the VR?

NI for comment 74

Comment on attachment 8403473 [details] Path in github So, this looks good. now I'd like just some comments in the CSS, and I need Victoria answer for the currently edited recipients.

(In reply to Julien Wajsberg [:julienw] from comment #74) > Victoria, I just think we miss an important piece of information: how a > currently-edited recipient looks like. > > Historically it used to look like an "assimilated" recipient, only without > the "recipient pill". Should it be the same with the VR? Sure, this didn't change. I have already checked that behaviour wuth Joan. Thanks for bringing it up in any case ;)

Victoria, just to be clear ;) * should the "currently-edited" recipient have the same style as the "assimilated" recipient (italic, font-size), or should it have the same style than before the refresh (not italic, bigger font-size)? It's currently the latter.

(In reply to Julien Wajsberg [:julienw] from comment #78) > Victoria, just to be clear ;) > > * should the "currently-edited" recipient have the same style as the > "assimilated" recipient (italic, font-size), or should it have the same > style than before the refresh (not italic, bigger font-size)? > > It's currently the latter. Same before the refresh, so both states are really different. THanks!

Comment on attachment 8403473 [details] Path in github r=me, let's land this I can fix the comments myself but I left some questions about the "white-space: pre-wrap;" changes. Arnau, do you know the answer?

Joan, I don't know if you have the time to answer this (see comment 80)

(In reply to Julien Wajsberg [:julienw] from comment #80) > Comment on attachment 8403473 [details] > Path in github > > r=me, let's land this > > I can fix the comments myself but I left some questions about the > "white-space: pre-wrap;" changes. Arnau, do you know the answer? No idea, Joan will have to answer. I've installed the patch and looks like the option pre-wrap has no effect, maybe it was used to override a previous value, but FWIW white-space could be removed.

Julien, appears to be a residue of a test, as it can remove said Arnau ...

I updated PR https://github.com/mozilla-b2g/gaia/pull/18088

Thanks so much ! NI me for merging.

\o/ Good job guys!

I've noticed a localization issue: if "To" is made bigger because of localization, the panel will override it. I'm filing a follow-up for this.

Filed bug 1017018.

master: 4142df90c71abdc1e3a87cd37dff1a22d5e38b34 I also found bug 1017024 while testing, but let's handle this as a bug.

Tested and fine Hamachi 2.0 Gecko-c5087e3 Gaia-5dfe4ec