We have some UX discrepancies and reflows that we need to get rid of, that happens when modifying the height and width of the different fields of the compose area. - Compose group, variable height till a certain limit, which would be different if we are on a conversation or a new message. - Subject input, variable to a max of two line height, can't use whole width because it would hide the character counter - Message, variable height depending on lines, variable width depending on the size of the button - Send button, variable width depending on locales (translation for word send can be longer) Creating a flexible layout for it, so gecko takes care of the measures and changes would be extremely helpful.

Joe, we would need to add this within the 1.4 scope, and the dependency related ---> https://bugzilla.mozilla.org/show_bug.cgi?id=949500 . Could you help us? Thanks!

1.4? to triage for 1.4

Borja, do you think you'll work on this or is it free to take?

Im back to Gaia! So Ill work on this :)

Great, this is really important and I think you'll be just fine with this because you know well how this should behave :)

move to 1.5? to be considered together with visual refresh

I've created a proposal that Im checking with UX. I hope to deliver the final patch next week! It's good to see the composer without any JS related \o/ !!

bug 973407 will likely give you conflicts but that should be easy to resolve since you probably remove most of this code ;)

First approach without any JS, using the flex layout. Tests are not fixed yet (That's why is a WIP patch! :) )

Tests fixed as well :)

Comment on attachment 8380695 [details] Regular message without scroll fine by me, good work!

Comment on attachment 8380687 [details] WIP Patch patch seems fine from a UX PoV. Good work

Comment on attachment 8380687 [details] WIP Patch Ayman's review is more a ui-review.

Comment on attachment 8380687 [details] WIP Patch I'm happy with this, we can definitely feel that all transitions from/to threads are a lot faster and smoother, and showing/hiding the keyboard too. I think we should split the patch in 2 patches: * one to eliminate updateInputHeight, this is the one we should do here, as this is the one that involves flex layout, and is quite simple from a behavior point of view * another one to eliminate the max height handling, as I think this is the trickiest to get right for all existing behaviors There are some strange behaviors but I think you're aware of this already and I hope they'll go away once we properly split the patch. Thanks for the awesome work!

this is part of the visual refresh for 1.5

What's left to do here? No activity in the last few weeks. Also, how tightly integrated into Messaging is this? Should the composer be moved into shared or componentized for other apps that have flexible multiline composition needs (Email, Contacts, and 3rd party)? (Obviously not in this bug - maybe a followup?)

(In reply to Dietrich Ayala (:dietrich) from comment #19) > What's left to do here? No activity in the last few weeks. Other work with higher priority, you know what it is ;) > > Also, how tightly integrated into Messaging is this? Should the composer be > moved into shared or componentized for other apps that have flexible > multiline composition needs (Email, Contacts, and 3rd party)? (Obviously not > in this bug - maybe a followup?) This is mostly CSS stuff, I'm not sure this could really be componentized unless the UX is the same. We could definitely at least share knowledge though.

Just FYI, under certain conditions display:flex can use excessive memory. See bug 977594. A quick glance at the patch suggests you aren't hitting these conditions, but I just wanted to make you aware of them.

(In reply to Dietrich Ayala (:dietrich) from comment #19) > What's left to do here? No activity in the last few weeks. Coming back to the activity here! After talking with Julien yesterday we are going to try to move forward with this as part of the Visual Refresh, so I'll take a look this week to Julien' suggestions. > Also, how tightly integrated into Messaging is this? Should the composer be > moved into shared or componentized for other apps that have flexible > multiline composition needs (Email, Contacts, and 3rd party)? (Obviously not > in this bug - maybe a followup?) As Julien said, this is a bunch of CSS which replace the way we did in the past (remember Portland some time ago! ;) ). When we added MMS, we were not tied to the Building Block 'input' anymore (due to textarea was replaced by a div witn 'content editable'), so it's not a common element. However I agree we should do this bottom-up, trying to implement it in Messaging app and translate our experience to other apps, even to Building Blocks. (In reply to Ben Kelly (:bkelly) from comment #21) >Just FYI, under certain conditions display:flex can use excessive memory. See bug 977594. A quick glance at the patch suggests you aren't hitting these conditions, but I just wanted to make you aware of them Thanks a lot for the info. I'll try to ensure that we are keeping our performance in a good shape.

Just updated the patch with Julien suggestions. Things to take into consideration: - This approach makes 'subject' & 'body' to live in the same space (it was approved by Ayman) - This visual approach is taking a _fixed_ height for the composer, regardless the scenario you are (thread of messages with or without carrier or 'new'). This keeps things simple, and removes the logic we had in the composer (which produces a lot of reflows). Victoria, could you confirm the visual for this? Check the following snapshot: https://bug949457.bugzilla.mozilla.org/attachment.cgi?id=8380697 @Julien: With this patch there is no need to calculate max-height anymore! Simple & functional, this will let us to have a faster implementation of the visual refresh. Thanks!

Comment on attachment 8380687 [details] WIP Patch All suggestions addressed. r?

We need the UX point of view as well. Omega, please see comment 23. I didn't try the patch yet, but the logic seems good to me: * better performance * good consistency (having a different max-height depending on the keyboard being displayed always felt wrong to me).

Hi, Already commented with Borja offline, but the solution for the height of the input area should be to grow to the maximum area possible until reaching the "to field" where it stops and starts to scroll the content. This will be the behaviour in the composer only, as there's nothing to interact with in the canvas that is hidden by this behaviour. In the messages thread, nevertheless, the input area leaves a gap between it and the header such that the user can tap on the canvas where the thread is living. Thanks! V

Borja, I thought of this overnight too, and I think we can try to use the the new "vh" CSS unit, maybe with a "calc", so that we can do this in CSS only.

> Already commented with Borja offline, but the solution for the height of the > input area should be to grow to the maximum area possible until reaching the > "to field" where it stops and starts to scroll the content. This will be the > behaviour in the composer only, as there's nothing to interact with in the > canvas that is hidden by this behaviour. In the messages thread, > nevertheless, the input area leaves a gap between it and the header such > that the user can tap on the canvas where the thread is living. As there are some css fixes to take into account for having the whole solution, I've opened the following bug https://bugzilla.mozilla.org/show_bug.cgi?id=990518, and it's related with how to update the layout when the keyboard suggestions are shown, and how it should takes the whole space available.

When testing I've found the following bug: https://bugzilla.mozilla.org/show_bug.cgi?id=990528 This is *not* related with this patch, it's related with the new styles in Edit Mode Building Block.

Comment on attachment 8380687 [details] WIP Patch I added a bunch of comments on github. Apart of this, The max-height behavior is regressing a lot but I think this is fine for now and this will make a more reliable basis to reintroduce it correctly in a follow-up patch. Please file a bug for reintroducing the max-height behavior :)

Comment on attachment 8380687 [details] WIP Patch I've addressed your suggestions. Some issues: - Max height behavior is tied to some other changes taking into account the suggestion bar of the keyboard. This work is going to be tracked here: https://bugzilla.mozilla.org/show_bug.cgi?id=990518 - Moving the content of the subject to one line was a requirement, and actually we have even a test for ensuring this, that's why Im keeping it. Making some tests between other devices, the received MMS is not keeping that break lines even if we send them. - There is a test failing which is not tied to this code (it's in Master). It seems to be related with the job done during the last days, where you have been working with Steve. Steve, Julien, could you take a look to this? We need to ensure that our tests are working. The test which is failing is: "[sms-test/unit/sms_test.js] SMS App Unit-Test Threads-list Threads-list rendering Render unread style properly: at (anonymous) (http://sms.gaiamobile.org:8080/test/unit/sms_test.js:275:9)" I hope it helps! r?

Comment on attachment 8380687 [details] WIP Patch I'm sorry, I really think we should use `white-space: pre-wrap` for the subject.. I'm sorry because my previous comment was not clear and did not point to the right URL demonstrating the benefits of it :(

Hi Julien, Some things related with last review: - Subject now is part of the composer. It means that it should have the same properties as the 'body', so no 'prevent' keys, no avoid 'line breaks'. From the user perspective, he is typing in the same area as the text, with the same behaviour. However, when sending through the network, this 'line break' should be removed (probably it's because the email compatibility). Please check our requirements: Page 12: https://mozilla.app.box.com/s/0u4jt353ei9ov2c150ip/1/1170795225/11918301182/1 "Interaction with new line key on keyboard: + There is no concept of new line in a subject, however the new line key will be available and selectable from the keyboard therefor:upon selection of new line key on keyboard + The interface will allow a new line to be entered into the subject string and this will be presented as a new line within the subject input field upon select send when a subject contains a new line + Replace all ‘new lines’ that are present in the subject string with ‘space’. + There is no need to notify the user that ‘new lines’ have been replaced with ‘space’ because basically the integrity of the message the subject string is attempting to convey is not lost or altered if we replace ’new line’ with ’space’. A notification itself might well = noise so we are comfortable operating with out one." So taking this into account, in your example http://jsfiddle.net/Q3cvr/1/, add a new character after a new line break (for example 'foo'), and the result is: plop plupfoo Instead of plop plup foo And furthermore, the result should be 'plop plup foo' following the spec (which is the current behaviour implemented in the patch). Please, could you clarify this? - IDs vs Classes. There are several points of view about this, but mainly the reasons behind your comment is the reusability, and in our case, there is not. This is a specific markup with an specific CSS which will be not reused. So in our case, we could apply IDs without any issue.Furthermore, as you know that CSS are read from right to left, reducing the levels of hierarchy in the query make our CSS performance better. However, If you consider that some of the IDs could be improved, please point them into the code and I'll sort them out. Thanks!

(In reply to Borja Salguero [:borjasalguero] from comment #33) > Hi Julien, > Some things related with last review: > > - Subject now is part of the composer. It means that it should have the same > properties as the 'body', so no 'prevent' keys, no avoid 'line breaks'. From > the user perspective, he is typing in the same area as the text, with the > same behaviour. > However, when sending through the network, this 'line break' should be > removed (probably it's because the email compatibility). Please check our > requirements: > > Page 12: > https://mozilla.app.box.com/s/0u4jt353ei9ov2c150ip/1/1170795225/11918301182/1 Damned, you're good at reading specs ;) I don't like this but this is the spec, so you're right, let's do this. > So taking this into account, in your example http://jsfiddle.net/Q3cvr/1/, > add a new character after a new line break (for example 'foo'), and the > result is: > > plop > plupfoo > > Instead of > > plop > plup > foo You're right, pressing <enter> generates a <br>... I don't know why I saw something else yesterday :( I just spent about 1 hour to try to find a workaround reading old Gecko code, this was not fun :) Ok, back to step 1! I'll read your code again tomorrow with these information, sorry for this. > > And furthermore, the result should be 'plop plup foo' following the spec > (which is the current behaviour implemented in the patch). Please, could you > clarify this? Yep this is right. > > - IDs vs Classes. There are several points of view about this, but mainly > the reasons behind your comment is the reusability This is reusability and maintanibility and specificity war. I am mainly concerned about the specificity war because the use of IDs is what made the current CSS grow out of control, but I like that other advantages are coming for free. > and in our case, there > is not. This is a specific markup with an specific CSS which will be not > reused. So in our case, we could apply IDs without any issue.Furthermore, as > you know that CSS are read from right to left, reducing the levels of > hierarchy in the query make our CSS performance better. In reality, the gain is really marginal. Using only a selector composed of 2 sub-selectors (eg: "#form-composer .subject") should have a very good performance. That said, I discussed with :Rik about the good practice (he's better than me about this), and the current good practice is to use a separate class for each stylable object (.composer-subject, .composer-body) so that the main CSS properties can be specified using a simple selector, and use the compound selectors to change things depending on the outer state (for example: .has-subject .composer-subject { display: initial; }) > However, If you > consider that some of the IDs could be improved, please point them into the > code and I'll sort them out. My question is really: why do you want IDs so badly where classes just work the same and perfectly fine? :) In JavaScript, you just need to switch your getElementById for a querSelector, and in CSS you just need to change a `#` to a `.`. Really, I'm not a CSS expert, but I listen to the CSS experts, and most CSS experts in the world that I respect agreed on these rules. Therefore my guess is that these rules have advantages and I should follow them instead of questioning them.

Comment on attachment 8380687 [details] WIP Patch setting the r? flag again for reading the javascript code again tomorrow. But feel free to change the markup + css in the mean time!

(In reply to Julien Wajsberg [:julienw] from comment #34) > My question is really: why do you want IDs so badly where classes just work > the same and perfectly fine? :) In JavaScript, you just need to switch your > getElementById for a querSelector, and in CSS you just need to change a `#` > to a `.`. Actually no. You shouldn't use a selector in querySelector that contains classes. Use an id or a class prefixed with 'js-'. The rationale for that is that you can add and remove classes (that don't have js-) for styling as needed without the fear of breaking the behaviour of the app. It makes a clear distinction which parts are for styling and which parts are for DOM selection.

Listen to Anthony, he's more expert than me ;)

Comment on attachment 8380687 [details] WIP Patch oki, I reviewed it again with the new information. I don't want to make it perfect, just good enough so that we can build on it for the next patches without changing everything again. thanks !

Comment on attachment 8380687 [details] WIP Patch Added more comments on github I think this is coming very well. Can you file a new bug for handling the new max-height behavior, since we completely remove the behavior here. I'm confident we can do a pure css way of it too ! Sorry for the delay, I wanted to be in good form to review the patch correctly...

(In reply to Julien Wajsberg [:julienw] from comment #39) > Comment on attachment 8380687 [details] > WIP Patch > > Added more comments on github > > I think this is coming very well. > > Can you file a new bug for handling the new max-height behavior, since we > completely remove the behavior here. I'm confident we can do a pure css way > of it too ! > > Sorry for the delay, I wanted to be in good form to review the patch > correctly... Hi Julien, Thanks again for the review. I know its tough to keep our review queue clean (Im having the same issue), but we should definitely prioritize and avoid to have gaps of 20 days between one review and another, because we loose completely the discussions and suggestions flow. Actually your request about having a separate bug for the max height was answered here: https://bugzilla.mozilla.org/show_bug.cgi?id=949457#c31 Please take your time to take a final review, and I'll try to sort your suggestions out as soon as I have your comments. As I mentioned before, I know that it's tough and I understand all reasons behind the gaps, but we should move forward asap here. This is *only* the first step, and the goal is well-defined: only remove the JS `black magic` behind the growth of the input in SMS App, moving to a flex panel with the minimum JS changes.

> avoid to have gaps of 20 days between one review and another I don't understand: last review was 9 days after the previous one, and only 3 days after your review request. It's not a 1-line patch, it's a complicated patch, so it's perfectly normal that it takes longer to review if you want that I do the review correctly. > Actually your request about having a separate bug for the max height was answered here: > https://bugzilla.mozilla.org/show_bug.cgi?id=949457#c31 Then Bug 990518's title is not correct. I've seen this bug but I thought it involved only the keyboard issue (which I think will be difficult to fix) whereas we need to take into account whether we're in composer or thread layout too, and probably other subtle issues that I don't remember now.

Comment on attachment 8380687 [details] WIP Patch added some more comments on github, but not too many ;)

Comment on attachment 8380687 [details] WIP Patch Just updated with your suggestions. Some of them are answered directly in Github.

Comment on attachment 8380687 [details] WIP Patch still 3 small comments and I think we're good after this

Comment on attachment 8380687 [details] WIP Patch Last changes added.

Comment on attachment 8380687 [details] WIP Patch r=me ok, let's land this amazing work !! Thanks Borja for your patience :)

https://github.com/mozilla-b2g/gaia/commit/d63efe7af7e7a336a28c19d9e52b0d66c4491d7d https://github.com/borjasalguero/gaia/commit/2a8a5aced77b0a168cf0010f8ff368789b45a441 Thanks a lot Julien! We deserve some beers & Jamón! ;)

Tested and working 1.5 Hamachi Gecko: 6fafefc Gaia: 0d705e6