Closed
Bug 870888
Opened 12 years ago
Closed 12 years ago
[MMS] [UX] Compose / Reply: the cursor in the input field is missaligned
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
Tracking
(blocking-b2g:leo+, b2g18 verified)
People
(Reporter: vicky, Assigned: jugglinmike)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
borjasalguero
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
blocking-b2g: --- → tef?
Comment 1•12 years ago
|
||
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.
blocking-b2g: tef? → ---
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → mike
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #752823 -
Flags: review?(felash)
Comment 3•12 years ago
|
||
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 :)
Attachment #752823 -
Flags: review?(felash) → review-
Assignee | ||
Comment 4•12 years ago
|
||
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?
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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
Attachment #752823 -
Attachment is obsolete: true
Attachment #753319 -
Flags: review?(felash)
Comment 8•12 years ago
|
||
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.
Attachment #753319 -
Flags: review?(felash) → review-
Assignee | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
The correct cursor position should land in 1.1 - marking leo?
blocking-b2g: --- → leo?
Updated•12 years ago
|
blocking-b2g: leo? → leo+
Assignee | ||
Comment 11•12 years ago
|
||
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.
Attachment #753319 -
Attachment is obsolete: true
Attachment #755511 -
Flags: review?(felash)
Comment 12•12 years ago
|
||
Comment on attachment 755511 [details] [diff] [review]
Correct Compose input alignment v3
corey and Mike will wrap-up the final bits.
Attachment #755511 -
Flags: review?(felash) → review?(gnarf37)
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #755511 -
Attachment is obsolete: true
Attachment #755511 -
Flags: review?(gnarf37)
Attachment #755993 -
Flags: review?(fbsc)
Comment 14•12 years ago
|
||
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'
Attachment #755993 -
Flags: review?(fbsc) → review+
Comment 15•12 years ago
|
||
Thanks Mike for your patch. Surgical and working perfectly. Thanks!
Assignee | ||
Comment 16•12 years ago
|
||
Thanks Borja. I've changed the comment as requested.
Landed at commit: d0481ffd5cde29dd7247f4e227ae349cf13d33fa
Thanks also to Julien!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Comment 17•12 years ago
|
||
Uplifted d0481ffd5cde29dd7247f4e227ae349cf13d33fa to:
v1-train: 1269d2714a50f29148bf5c9655460d8e4ea457c5
status-b2g18:
--- → fixed
Comment 18•11 years ago
|
||
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
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•