Closed Bug 1078384 Opened 5 years ago Closed 5 years ago

[MMS] Move MMS label to appear always at the end of the subject line

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: salva, Assigned: salva)

References

Details

Attachments

(3 files)

From bug 1026384 comment 23:

> There is an issue when the user adds an email recipient and no text is added
> in composer yet.
>
> In that case, we display "MMS" ahead of the send button, and this makes the
> composer bigger.

From bug 1026384 comment 34:

> Notice this is happening even when simply attaching a picture. I would
> recommend another different bug for this.

Solution provided in 1026384 comment 39:

> Fang and I discussed about it and decided to move the string and always
> show "MMS" at the end of subject line. Having a fixed position for the
> string can also avoid visual inconsistency that sometimes it appears in
> composer area but other times outside of it.
Jenny, can you try to provide a comprehensive UX spec around the MMS mode and subject ? I don't think this is clear enough to implement something here...
Flags: needinfo?(jelee)
(In reply to Julien Wajsberg [:julienw] from comment #1)
> Jenny, can you try to provide a comprehensive UX spec around the MMS mode
> and subject ? I don't think this is clear enough to implement something
> here...

Hi Julien, let's discuss this offline and I will upload a spec (if needed) when we have a conclusion. thanks!
Flags: needinfo?(jelee)
Assignee: nobody → salva
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

I think we need the final spec from Jenny and Fang first.

Also, we don't switch to MMS automatically when entering a thread for an email recipient, maybe we need to file a separate bug for this? This is what's causing the main issue with this implementation, so it's too bad we don't see it currently.

When we're ready with Jenny's suggestion, please ask Oleg because he knows this part of the CSS very well.
Attachment #8512595 - Flags: review?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #4)
> Comment on attachment 8512595 [details] [review]
> Aligning the MMS label at the end of the subject line, over the send button.
> 
> I think we need the final spec from Jenny and Fang first.

Agree.

> 
> Also, we don't switch to MMS automatically when entering a thread for an
> email recipient, maybe we need to file a separate bug for this? This is
> what's causing the main issue with this implementation, so it's too bad we
> don't see it currently.

I see. I'm going to file another bug for this as it is not related with visual style but it is very important for all the functionality.

> 
> When we're ready with Jenny's suggestion, please ask Oleg because he knows
> this part of the CSS very well.

Yep, I agree, asking for feedback to Oleg and once we have the spec, I will ask him again for review.
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Hi Oleg, we are still waiting for the final spec but can you provide me some feedback about the patch to always show the MMS label at the end of the subject composition line?
Attachment #8512595 - Flags: feedback?(azasypkin)
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Hey Salvador,

Codewise it looks code to me, except for the following points:

* multiline subject looks broken in Composer;
* you'll need to update one marionette integration test [1];
* mms label and char counter look misaligned (too close to the left side) - I think it's not related to your patch, but we'll need to fix that anyway once we get UX spec.

Thanks!

[1] https://github.com/mozilla-b2g/gaia/blob/3c1668ad793d4f1a40831c46638acef38e0db230/apps/sms/test/marionette/composer_test.js#L55
Attachment #8512595 - Flags: feedback?(azasypkin)
(In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> Comment on attachment 8512595 [details] [review]
> Aligning the MMS label at the end of the subject line, over the send button.
> 
> Hey Salvador,
> 
> Codewise it looks code to me, except for the following points:
> 
> * multiline subject looks broken in Composer;

Sorry, I don't understand you. AFAIK, subject is not multiline as seen in [1]

> * you'll need to update one marionette integration test [1];

I've updated the tests, I did not get to run myself so I'm relaying on Treeherder for that. I'll ask for your feedback again when they finish.

> * mms label and char counter look misaligned (too close to the left side) -
> I think it's not related to your patch, but we'll need to fix that anyway
> once we get UX spec.

This was already happening before my patch. Let's wait for the spec but I would want to ask for not blocking on this as it's an unrelated bug.

[1] https://github.com/mozilla-b2g/gaia/pull/25574/files#diff-56289627d91906a9c3b0f834ef19f514R262
Flags: needinfo?(azasypkin)
(In reply to Salvador de la Puente González [:salva] from comment #8)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #7)
> > Comment on attachment 8512595 [details] [review]
> > Aligning the MMS label at the end of the subject line, over the send button.
> > 
> > Hey Salvador,
> > 
> > Codewise it looks code to me, except for the following points:
> > 
> > * multiline subject looks broken in Composer;
> 
> Sorry, I don't understand you. AFAIK, subject is not multiline as seen in [1]
> 

I meant that subject visually wraps, but it does not accept '\n' (the use of aria-multiline is to let the user know if it is a textbox where they can enter line breaks or not). The easiest way to see the issue is just enter a lot "W" chars - on flame it can wrap into 3 lines.
Flags: needinfo?(azasypkin)
Ok, I see. Now it is not wrapping properly. I will investigate about this. Thx.
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

I reviewed the code making the subject label to wrap properly and fixing the integration tests. Can you give me some feedback, please?
Attachment #8512595 - Flags: feedback?(azasypkin)
(In reply to Salvador de la Puente González [:salva] from comment #11)
> Comment on attachment 8512595 [details] [review]
> Aligning the MMS label at the end of the subject line, over the send button.
> 
> I reviewed the code making the subject label to wrap properly and fixing the
> integration tests. Can you give me some feedback, please?

Sorry for the delay, will definitely take a look tomorrow!
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Looks good to me, just two nits about relying on flex instead of "calc" and additional assert in integration test.

Also I see that Gij is still failing for your PR, it maybe be related to bug 1094076, but maybe not - please be sure that it's not because of your patch.

Thanks!
Attachment #8512595 - Flags: feedback?(azasypkin) → feedback+
Hi all, please refer to p.13 - p.16 for MMS marker behavior. thanks!
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Hi there, behavior wise looks good to me, also flagging Fang for ui-review. Thanks Fang!
Attachment #8512595 - Flags: ui-review?(jelee)
Attachment #8512595 - Flags: ui-review?(fshih)
Attachment #8512595 - Flags: ui-review+
Attached image MMS_bug.png
The only issue here is the “MMS” should be center aligned to the send button.  See attached for correct screen. Thanks!
Attachment #8512595 - Flags: ui-review?(fshih) → ui-review-
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Hi guys. I updated the PR. Could you mind to review, please?
Attachment #8512595 - Flags: ui-review?(fshih)
Attachment #8512595 - Flags: ui-review-
Attachment #8512595 - Flags: review?(azasypkin)
s/Could/Would/
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Works good, but I still have some doubts regarding to some CSS changes left from previous review. Please, see at GitHub.

Thanks!
Attachment #8512595 - Flags: review?(azasypkin)
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

It looks good to me now! Thanks! : )
Attachment #8512595 - Flags: ui-review?(fshih) → ui-review+
Ok. I'm going to try my best in order to explain whats happening here:

This is the current layout:

.mms-label-line
+---------------------------------------+
|+------------------------++-----------+|
||.subject-composer-input ||.mms-label ||
|+------------------------++-----------+|
+---------------------------------------+
|.content-composer                      |
|                                       |

In addition, you have another box .subject-composer-placeholder for the subject placeholder with position absolute so it's out of the flex flow.

We need to make MMS to appear always to the right so .subject-line is flex and `justify-content: flex-end`. This forces .mms-label to stay at right even if .subject-composer-input is not displayed (`display: none`).

The problem is, when you add subject but don't write, if you make `display: none` for .mms-label, then .subject-composer-input is moved to the right as this is the value in `justify-content`. This is because .subject-composer-input is the only item being displayed and the flex container is set to move its items to the right. With `visibility: hidden` we make .mms-label to be not displayed but considered as a visible item (its box is not removed from the layout). Furthermore, this increases performance by avoiding a reflow.

Setting `justify-content: flex-start` does not solve the problem as now, if we attach an image without adding the subject, the .mms-label will appear at the beginning of .subject-line. You could think `align-self` is there to override this behavior but 'align-self` refers only to the cross axis. There is no similar way of overriding justification for main axis. The `align-self` property is there to keep its content to the bottom.

What do you think?
Attachment #8512595 - Flags: review?(azasypkin)
(In reply to Salvador de la Puente González [:salva] from comment #21)
> Ok. I'm going to try my best in order to explain whats happening here:
> 
> The problem is, when you add subject but don't write, if you make `display:
> none` for .mms-label, then .subject-composer-input is moved to the right as
> this is the value in `justify-content`. This is because
> .subject-composer-input is the only item being displayed and the flex
> container is set to move its items to the right. 

Mmm, is it still the case when you replace "width: calc...." with "flex-grow: 1; min-width: 0"? In this case subject placeholder will occupy all space and should be correctly aligned?
(In reply to Oleg Zasypkin [:azasypkin] from comment #22)
> (In reply to Salvador de la Puente González [:salva] from comment #21)
> > Ok. I'm going to try my best in order to explain whats happening here:
> > 
> > The problem is, when you add subject but don't write, if you make `display:
> > none` for .mms-label, then .subject-composer-input is moved to the right as
> > this is the value in `justify-content`. This is because
> > .subject-composer-input is the only item being displayed and the flex
> > container is set to move its items to the right. 
> 
> Mmm, is it still the case when you replace "width: calc...." with
> "flex-grow: 1; min-width: 0"? In this case subject placeholder will occupy
> all space and should be correctly aligned?

Notice the placeholder is absolutely positioned so it's not affected by flex. In case you're referring to .subject-composer-input, changing `width: calc(...)` by `flex-grow: 1; min-width: 0` but keeping `visibility: hidden;` for .mms-label won't change anything as the layout inside the flex container remains unaltered. Although, I think your approach of no using `width: calc(...);` is more elegant indeed so I changed mine.
Oh. I got you. Sorry. Once you add `min-width:0` and `flex-grow:1;` the .subject-composer-input extends itself to cover all the available space so it keeps the alignment correct even if there is no .mms-label being displayed. This allow us to completely hide the .subject-line recovering the extra space for when there is no subject.
Comment on attachment 8512595 [details] [review]
Aligning the MMS label at the end of the subject line, over the send button.

Okay, now it looks good to me! I've left just a few last nits on GitHub and restarted busted Gij job on Treeherder.

Thanks for taking care of this!
Attachment #8512595 - Flags: review?(azasypkin) → review+
I updated the PR. I'm waiting until Treeherder finishes to land the patch.
master: 7fb4ddfb74a0481d813bbd554a91c1e009e16412
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.