870888 - [MMS] [UX] Compose / Reply: the cursor in the input field is missaligned

Status: VERIFIED FIXED

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

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: illustrates the missalignment

The cursor in the input field is upper that what it should and missaligns the text. This also creates an undesired extra blank space in the bottom.

See attachment illustrating.

Comment 1

[Daniel Coloma:dcoloma]

tef? is to block for 1.0.1 releases and MMS is not part of that release, I am removing the nomination, not sure what flag/version you want to use here.

Comment 2

[Mike Pennisi [:jugglinmike]]

Attachment: Correct Compose input alignment

This patch removes absolute positioning from the elements inside the Compose field. Those elements now remain in the document flow and the rendering engine can managing re-calculating dimensions in response to text input (including the position of the input cursor).

This refactoring obviates the need for the brittle and imprecise "ThreadUI.updateInputHeight" method.

Please note that, although not strictly related to this bug, I have removed the `!important` directive from the following declaration for the "contenteditable" element:

  line-height: 1.8rem !important;

Because it is not actually necessary.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 752823 [details] [diff] [review]
Correct Compose input alignment

I don't exactly understand because it does not resemble the visual spec at all. Here are the problems that I see:

* the send button is at the bottom instead of being at the right
* we don't see the counter nor the message type
* the attachment button is not completely at the left
* the compose area goes over the last message(s)

So, well, this is not ready, even if I'd love to see this method go to hell.

On the plus side, typing feels a lot more responsive on the device :)

Comment 4

[Mike Pennisi [:jugglinmike]]

Hey Julien,

> * we don't see the counter nor the message type

This is a trivial fix, unfortunately, the others present more of a problem

> * the send button is at the bottom instead of being at the right
> * the attachment button is not completely at the left

These have to do with the width of the buttons. The widths can't be hard-coded because different localizations will need different widths. I suspect you see the "Send" button rendering bug because you are using the French localization (?)

As it stands, this is also a problem in master--it's just the failure isn't as obvious. Currently, the buttons are absolutely positioned, and the `contenteditable` element has hard-coded padding values to prevent it from rendering "underneath" them. In contexts where the localization of "Send" needs a lot of space, parts of the `contenteditable` element will render beneath.

> * the compose area goes over the last message(s)

If I understand this correctly, I see the same behavior in master: as the composition element grows, the user loses the ability to bring the final messages into view.

Even if we give up on removing the `ThreadUI.updateInputHeight` method, these problems will remain. The more I work with this interface, the more I think that the traditional box model is simply not powerful enough to support it.

I think we should give serious thought to using flexbox. I know that the current implementation is behind spec, but I think transitioning from the old specification to the new will be much easier than trying to hack in temporary solutions and implement everything in flexbox from scratch later on. I don't see any official stance on flexbox in the Gaia mailing list, but a quick `grep` of Gaia's `apps/` directory shows that the Bluetooth, Browser, and costconstrol apps are already using it in some capacity...

Would you be open to a new patch that utilized flexbox?

Comment 5

[Mike Pennisi [:jugglinmike]]

Just to be clear: I do not mean to suggest that we need flexbox to close this bug. The "simple" fix will likely require a few minor tweaks to the CSS. (In this case, the problems I've identified in comment 4 would still remain.)

These kind of ad-hoc modifications do little for the health of the code base, though, and I am always interested in improving things (even where it requires some re-factoring).

It might be that we should create a separate bug to explore that possibility. In either case, I wanted to formally recognize this situation (and get your input) before submitting the "quick and dirty" patch.

Comment 6

[Julien Wajsberg [:julienw]]

Another possbility without using flexbox is to use display:table. But that was also my conclusion, using one of those.

My suggestion right now would be to simply fix this bug in the simplest way, but file a new bug to remove updateInputHeight which (I think) is synchronous and therefore makes typing quite uncomfortable.

Comment 7

[Mike Pennisi [:jugglinmike]]

Attachment: Correct Compose input alignment v2

This patch attempts to fix the bug without drastically altering the layout. It
was still necessary to heavily modify the `ThreadUI.updateInputHeight` method.
A couple of notes explaining why:

- The branching logic for the maximum height is not necessary because the input
  field already defines a maximum height via CSS. This makes the container's
  `max-height` property redundant.
- The `verticalPadding` value is not necessary, as it is already reflected in
  the input field's bounding rectangle.
- The input field does not need a minimum height because we maintain a line
  break element within the field. See Bug 840069 and Bug 414223
- Modifying the container's `bottom` property has no effect because it is
  rendered as `position: static;`

In addressing this specific bug, I have identified the following layout bugs.
These exist in "master" and are not regressions:

- Bug 875362 - [MMS] Cannot scroll to bottom of thread list as input field grows
- Bug 875364 - [MMS] Input field may grow too large

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 753319 [details] [diff] [review]
Correct Compose input alignment v2

Review of attachment 753319 [details] [diff] [review]:
-----------------------------------------------------------------

some problems :
* in edit mode we see the input if it is big enough (I don't know if this is in current master)
* strange behaviors when switching between keyboard and no-keyboard modes, when the input is big enough to be scrollable when the keyboard is displayed
  * eg try to tap on the "left" arrow to go back to the thread list view, but say "cancel" to stay on the thread view
  * try to tap on the header to start an activity, and then cancel the activity

maybe it is related to the max-height change, but maybe also other changes in the JS.

I only tried with SMS with lots of characters (hint: use capital letters ;) ), I didn't try MMS at all.

::: apps/sms/style/sms.css
@@ -569,5 @@
>    background-image: url(images/paperclip-button.png);
>  }
>  
> -#messages-compose-form {
> -  max-height: 38rem;

removing this makes it impossible to scroll to the top of the input when you are in a thread. It works correctly for a new message though.

Comment 9

[Mike Pennisi [:jugglinmike]]

The "Send" button bug I identified in Comment 4 has been resolved: Bug 871986. Those changes have set my work back a bit, but I'm still on the case!

Comment 10

[Corey Frang [:gnarf]]

The correct cursor position should land in 1.1 - marking leo?

Comment 11

[Mike Pennisi [:jugglinmike]]

Attachment: Correct Compose input alignment v3

This patch fine-tunes the input area position with CSS margin rather than padding. Because the `contenteditable` element only automatically scrolls to the bottom of the cursor (which does not include the padding area), any area defined by padding-bottom area is hidden when the input element grows.

By setting `-moz-box-sizing` to `border-box`, the JavaScript no longer needs to take padding into consideration. Now that the input element defines a margin, some layout calculations must be updated to account for that.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 755511 [details] [diff] [review]
Correct Compose input alignment v3

corey and Mike will wrap-up the final bits.

Comment 13

[Mike Pennisi [:jugglinmike]]

Attachment: Correct Compose input alignment v4

This version addresses Julien's feedback (shared via IRC):

> I think something is still not completely right: the input
> text is not aligned with the button text and the attachment
> icon

There, he also identified a problem where the Compose field was
being obscured by the "Carrier" banner. This issue actually
concerns the entire subheader, but since it exists in master, I
have filed Bug 877683 to track it.

Comment 14

[Borja Salguero [:borjasalguero]]

Comment on attachment 755993 [details] [diff] [review]
Correct Compose input alignment v4

Review of attachment 755993 [details] [diff] [review]:
-----------------------------------------------------------------

Update the small comment and ready for merging!

::: apps/sms/js/thread_ui.js
@@ +538,1 @@
>        // Update the bottom bar height taking into account the padding

Change comment to 'margin'

Comment 15

[Borja Salguero [:borjasalguero]]

Thanks Mike for your patch. Surgical and working perfectly. Thanks!

Comment 16

[Mike Pennisi [:jugglinmike]]

Thanks Borja. I've changed the comment as requested.

Landed at commit: d0481ffd5cde29dd7247f4e227ae349cf13d33fa

Thanks also to Julien!

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted d0481ffd5cde29dd7247f4e227ae349cf13d33fa to:
v1-train: 1269d2714a50f29148bf5c9655460d8e4ea457c5

Comment 18

[Deepa Subramanian]

The user is able to the see the cursor in the input field with the text clearly displayed. Issue seemed to be fixed.

Environmental Variables
Leo Build ID: 20130807071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eefff
Gaia: 60ca81600a080dae33058b0692ecaa213556c926
Platform Version: 18.1