949457 - Make Compose into a flex layout

Status: VERIFIED FIXED

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

Description

[Fernando Campo (:fcampo)]

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.

Comment 1

[Borja Salguero [:borjasalguero]]

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!

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

1.4? to triage for 1.4

Comment 3

[Julien Wajsberg [:julienw]]

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

Comment 4

[Borja Salguero [:borjasalguero]]

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

Comment 5

[Julien Wajsberg [:julienw]]

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

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

move to 1.5? to be considered together with visual refresh

Comment 7

[Borja Salguero [:borjasalguero]]

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/ !!

Comment 8

[Julien Wajsberg [:julienw]]

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

Comment 9

[Borja Salguero [:borjasalguero]]

Attachment: WIP Patch

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

Comment 10

[Borja Salguero [:borjasalguero]]

Attachment: Regular message without scroll

Comment 11

[Borja Salguero [:borjasalguero]]

Attachment: Large subject

Comment 12

[Borja Salguero [:borjasalguero]]

Attachment: Large subject & body. Scroll the whole area.

Comment 13

[Borja Salguero [:borjasalguero]]

Tests fixed as well :)

Comment 14

[ayman maat :maat]

Comment on attachment 8380695 [details]
Regular message without scroll

fine by me, good work!

Comment 15

[ayman maat :maat]

Comment on attachment 8380687 [details] [review]
WIP Patch

patch seems fine from a UX PoV. Good work

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
WIP Patch

Ayman's review is more a ui-review.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
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!

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

this is part of the visual refresh for 1.5

Comment 19

[Dietrich Ayala (:dietrich)]

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?)

Comment 20

[Julien Wajsberg [:julienw]]

(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.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 22

[Borja Salguero [:borjasalguero]]

(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.

Comment 23

[Borja Salguero [:borjasalguero]]

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 24

[Borja Salguero [:borjasalguero]]

Comment on attachment 8380687 [details] [review]
WIP Patch

All suggestions addressed. r?

Comment 25

[Julien Wajsberg [:julienw]]

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).

Comment 26

[Victoria Gerchinhoren [:vicky]]

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

Comment 27

[Julien Wajsberg [:julienw]]

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.

Comment 28

[Borja Salguero [:borjasalguero]]

> 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.

Comment 29

[Borja Salguero [:borjasalguero]]

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 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
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 31

[Borja Salguero [:borjasalguero]]

Comment on attachment 8380687 [details] [review]
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 32

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
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 :(

Comment 33

[Borja Salguero [:borjasalguero]]

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!

Comment 34

[Julien Wajsberg [:julienw]]

(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 35

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
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!

Comment 36

[Anthony Ricaud (:rik)]

(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.

Comment 37

[Julien Wajsberg [:julienw]]

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

Comment 38

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
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 39

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
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...

Comment 40

[Borja Salguero [:borjasalguero]]

(In reply to Julien Wajsberg [:julienw] from comment #39)
> Comment on attachment 8380687 [details] [review]
> 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.

Comment 41

[Julien Wajsberg [:julienw]]

> 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 42

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
WIP Patch

added some more comments on github, but not too many ;)

Comment 43

[Borja Salguero [:borjasalguero]]

Comment on attachment 8380687 [details] [review]
WIP Patch

Just updated with your suggestions. Some of them are answered directly in Github.

Comment 44

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
WIP Patch

still 3 small comments and I think we're good after this

Comment 45

[Borja Salguero [:borjasalguero]]

Comment on attachment 8380687 [details] [review]
WIP Patch

Last changes added.

Comment 46

[Julien Wajsberg [:julienw]]

Comment on attachment 8380687 [details] [review]
WIP Patch

r=me

ok, let's land this amazing work !!

Thanks Borja for your patience :)

Comment 47

[Borja Salguero [:borjasalguero]]

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! ;)

Comment 49

[Loli (:lolimartinezcr)]

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