908241 - [SMS] Add attachment highlight state doesn't extend to the top edge

Status: RESOLVED INVALID

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

Description

[Amy Lee [:amylee] UX]

Attachment: Email-Add-Attachment.png

Hi, 

The blue highlight state doesn't extend to the top edge of the button.

Thanks

Comment 1

[Wayne Chang [:wchang]]

Amy, is this happening on HD only?

Comment 2

[Amy Lee [:amylee] UX]

(In reply to Wayne Chang [:wchang] from comment #1)
> Amy, is this happening on HD only?

Hi, 

This is HD and v1.1. What flag should this be changed to?

Comment 3

[Eric Pang [:epang] UX]

Hey Pavel, can you check the highlight state for this?  The height probably just needs to be increased. Thanks!

Comment 4

[Eric Pang [:epang] UX]

(In reply to Amy from comment #2)
> (In reply to Wayne Chang [:wchang] from comment #1)
> > Amy, is this happening on HD only?
> 
> Hi, 
> 
> This is HD and v1.1. What flag should this be changed to?

Hey Wayne, just wanted to make sure you saw Amy's response :)

Comment 5

[Julien Wajsberg [:julienw]]

Be careful as the CSS in that part is not so easy. :)

Could have regressed because of bug 902390.

Comment 6

[Wayne Chang [:wchang]]

I'd recommend we leave this for 1.1 and 1.1hd altogether, have it corrected on master/1.2 ?
:)

Comment 7

[Eric Pang [:epang] UX]

(In reply to Wayne Chang [:wchang] from comment #6)
> I'd recommend we leave this for 1.1 and 1.1hd altogether, have it corrected
> on master/1.2 ?
> :)

Yes, makes sense to me.  Thanks Wayne!  Change the whiteboard tags to reflect this.

Comment 8

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Comment 9

[Sam Joch [:samjoch]]

Comment on attachment 805965 [details]
patch for Gaia/master

I am not sure about the js part (apps/sms/js/thread_ui.js)
Redirect the review to a peer.

Comment 10

[Steve Chung [:steveck]]

Attachment: 2013-09-24-01-45-02.png

Hi Amy, the patch pivanov provided will move the button to the top. Is that the result you desired for? Thanks.

Comment 11

[Julien Wajsberg [:julienw]]

Steve, Victoria should be our only Visual Design contact for the Messages app.

Comment 12

[Amy Lee [:amylee] UX]

Hi Steve, 

Yes if you want to have Victoria confirm that would be great. The original bug was filed because the highlight state (blue overlay) wasn't reaching the top edge of the button. This isn't shown in the screenshot you provided so I can't provide feedback.

Comment 13

[Steve Chung [:steveck]]

Attachment: sample.png

Hi Victoria/Amy, this patch moves both buttons to the top of the container for keep the highlight background position. Is this result fit to your design? Thanks.

Comment 14

[Amy Lee [:amylee] UX]

Hi Steve, 

I'm happy with this. Victoria are you okay with this?

Comment 15

[Julien Wajsberg [:julienw]]

I'm quite sure Victoria won't like it, as she already rejected a similar proposal I made ;)

Comment 16

[Amy Lee [:amylee] UX]

The bug was filed for alignment issues of the highlight state and not for design revisions. Any design related feedback please consult with Victoria. Thanks

Comment 17

[Julien Wajsberg [:julienw]]

The alignment issue of the highlight state is due to fact that the button is not aligned to the top anymore. The cause is bug 902390 which increased the height by one pixel, but the real cause was the change to the new font which introduces that pixel as soon as we were typing one character. Bug 902390 was made to make that 1px change persistant, so that we don't have a reflow when we type the first character.

Victoria, maybe bug 902390 was a wrong solution to a good problem, and we should have decreased the font-size or the line-height instead ? Or maybe the final font has not the same height difference with the previous font ?

Comment 18

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

triage: not blocking v1.2, minus

Comment 19

[Victoria Gerchinhoren [:vicky]]

Thanks Amy for bringing this up, this looks OK to me. But one question, is this a problem commming from the Building Block design or was it a mistake made in App? I'll needinfo Arnau to take a look at this in the original BB.

Thanks!
Victoria

(In reply to Amy from comment #14)
> Hi Steve, 
> 
> I'm happy with this. Victoria are you okay with this?

Comment 20

[Julien Wajsberg [:julienw]]

Ayman, please have a look at attachment 808965 [details], as I thought you were opposed to this change.

Comment 21

[Victoria Gerchinhoren [:vicky]]

Hey Julien, we're mixing issues here. Nobody agreed on this change, but why is it in this bug? Can you raise a new one?

Thanks!

(In reply to Julien Wajsberg [:julienw] from comment #20)
> Ayman, please have a look at attachment 808965 [details], as I thought you
> were opposed to this change.

Comment 22

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Victoria, this is not a standard BB input field, the CSS for this component relies too much in js, adding inline styles to fix positions.

I would propose using flex-box for this component.

Comment 23

[Julien Wajsberg [:julienw]]

Isabel, there is a misunderstanding here: the attachment is exactly the result of the proposed patch in this bug.

Arnau, yes, good idea and completely agree, we didn't want to change the whole layout for 1.2 when it was a blocker, but could be a good idea to do this now that it's not a blocker anymore.

Comment 24

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> Ayman, please have a look at attachment 808965 [details], as I thought you
> were opposed to this change.

Thanks for flagging Julien.

Indeed I am oppose to this change. This is not the solution we are looking for. I have stated that the send CTA should not drift upwards as the message field extends. I have given clear direction as such, so we need an alternative solution.

Comment 25

[ayman maat :maat]

To be clear. both the bottom edge of the attach CTA and the bottom edge of the Send CTA need to remain anchored to the top edge of the top edge of the keyboard and must not drift upwards as the message field expands.

ni? me if you require further input

Comment 26

[Victoria Gerchinhoren [:vicky]]

OK, then it is treated as custom.

Thanks
(In reply to Arnau March from comment #22)
> Victoria, this is not a standard BB input field, the CSS for this component
> relies too much in js, adding inline styles to fix positions.
> 
> I would propose using flex-box for this component.

Comment 27

[Victoria Gerchinhoren [:vicky]]

Hey, I htink there's a big misunderstangding here, and what Amy is proposing to fix is just an offset higlight shape and not talking about any other solution. PLease, if you have a different bug to discuss open a new one, and treat it there. Let this one be fixed.

Thanks

(In reply to ayman maat :maat from comment #24)
> (In reply to Julien Wajsberg [:julienw] from comment #20)
> > Ayman, please have a look at attachment 808965 [details], as I thought you
> > were opposed to this change.
> 
> Thanks for flagging Julien.
> 
> Indeed I am oppose to this change. This is not the solution we are looking
> for. I have stated that the send CTA should not drift upwards as the message
> field extends. I have given clear direction as such, so we need an
> alternative solution.

Comment 28

[Victoria Gerchinhoren [:vicky]]

No, this is not what should happen here, the fix is just to fix this (https://bug908241.bugzilla.mozilla.org/attachment.cgi?id=794082) gap between the button shape/gradient ant the top divider for that field. Theres a white gap there that is more evident when the higlight color is on, just a gap. And this: https://bug908241.bugzilla.mozilla.org/attachment.cgi?id=808965 should not happen at all.

Thanks!

(In reply to Steve Chung [:steveck](PTO 10/1~10/8) from comment #10)
> Created attachment 808692 [details]
> 2013-09-24-01-45-02.png
> 
> Hi Amy, the patch pivanov provided will move the button to the top. Is that
> the result you desired for? Thanks.

Comment 29

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 808965 [details]
sample.png

This should not happen. The input elements should remain on the bottom part of the component while the input area expand upwards.

Comment 30

[Julien Wajsberg [:julienw]]

Victoria> I also agree with what Amy asked to do here, but since everybody said "yeah this patch is ok" while it was not, I had to raise a flag here ;)

Comment 31

[Amy Lee [:amylee] UX]

Hi Julien, 

Thanks for raising the flag. I was looking at the highlight state of the button and wasn't aware that changes were also made to layout of the text field.

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 805965 [details]
patch for Gaia/master

Hi pivanov, based on the Vicky's feedback, we still need to stick the button's position to the bottom. Please adjust the messages-input's margin and keep the buttonOffset logic in this patch, thanks.

Comment 33

[Julien Wajsberg [:julienw]]

I think this.INPUT_MARGIN is wrong and we need to check why.

Comment 34

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: After patch screenshot

Like this?

Comment 35

[Amy Lee [:amylee] UX]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #34)
> Created attachment 815327 [details]
> After patch screenshot
> 
> Like this?

The alignment of the buttons at the bottom look good to me. Has the highlight state been fixed so it reaches the top edge of the button? This was the original request. Thanks!

Comment 36

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: active state after patch

Sure :)

Comment 37

[Amy Lee [:amylee] UX]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #36)
> Created attachment 815382 [details]
> active state after patch
> 
> Sure :)

Looks great! Thanks Pavel

Comment 38

[Victoria Gerchinhoren [:vicky]]

Hey Pavel, this solves the confusion around how the input behaves (expanding upwards while the buttons stay sticked to the bottom) I guess the highlight issue is just fixed, right? Also, did something happen to the divider line at the top, the one that separates the recipients area from the body? Because it looks super dark and it used to be in a lighter (medium) grey. If that's something you did not tpuch, i'll just file a different bug for it.

Thanks!

Comment 39

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Victoria,

the color of the divider is #4d4d4d. I think there are some difference because of screenshots ... but if you want to change something on it we can do this in different, because it not related to this one (just assign it to me :))

Comment 40

[Steve Chung [:steveck]]

Comment on attachment 805965 [details]
patch for Gaia/master

Hi Victoria, please create another bug with color code if you feel the current divider color is wrong, thanks.

Hi Pavel, this css fixing with removing the unnecessary js logic is good. We should close with this patch and stress other UI issues in different bugs, thnaks.

Comment 41

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Thanks Steve :)

Landed on master:
https://github.com/mozilla-b2g/gaia/commit/400d5fec2f099397575ded3ef8544150a9b472c5

Comment 42

[Eric Pang [:epang] UX]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #41)
> Thanks Steve :)
> 
> Landed on master:
> https://github.com/mozilla-b2g/gaia/commit/
> 400d5fec2f099397575ded3ef8544150a9b472c5

Hi Pavel, because this is landed on master will it make it to HD?  Or does it need to land on HD as well?

Comment 43

[Julien Wajsberg [:julienw]]

This won't go to hd if this is not hd+.

Right now this won't even go to 1.2.

Comment 44

[Julien Wajsberg [:julienw]]

This patch doesn't work with the french locale, the input zone goes below the "send" (now "Envoyer") button (see attachment).

There is a reason this part uses Javascript, you know. This is not easy.

We may be able to do this with flex-box, as we didn't have this tool back when we first implemented this.

Backing out for now, sorry...

Comment 45

[Julien Wajsberg [:julienw]]

reverted: c07e3f25f51392391d57f1018ea2409e5695ef53

Comment 46

[Julien Wajsberg [:julienw]]

Attachment: problem with the patch with the french locale

Comment 47

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 805965 [details]
patch for Gaia/master

><html>
>  <head>
>    <meta http-equiv="Refresh" content="2; url=https://github.com/mozilla-b2g/gaia/pull/12267">
>  </head>
>  <body>
>    Redirect to pull request 12267
>  </body>
></html>

Comment 48

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

oh putain ... :) can you review it? I made it with flexbox :)

Comment 49

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #44)
> This patch doesn't work with the french locale, the input zone goes below
> the "send" (now "Envoyer") button (see attachment).
> 
> There is a reason this part uses Javascript, you know. This is not easy.
> 
> We may be able to do this with flex-box, as we didn't have this tool back
> when we first implemented this.
> 
> Backing out for now, sorry...

Sorry I miss the locale issue... my bad :(
It remind me that we did apply flex-box for adjusting compose input size automatically long time ago, but it's removed since whole sturcture changed.

Comment 50

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Steve,
is my bad ... I miss that too ... but Thanks to Julien we catch it on time :) and I think it's ready for review again

Comment 51

[Julien Wajsberg [:julienw]]

Comment on attachment 816726 [details]
patch for Gaia/master

This regresses the "MMS" and the counter indicators (which are in the same elements iirc).

The easiest way to show it is to attach an image so that the "MMS" indicator is displayed.

Please request review again once you're ready !

Comment 52

[Julien Wajsberg [:julienw]]

+ comments on github !

Comment 53

[Julien Wajsberg [:julienw]]

I think this is obsolete now, right?

Comment 54

[Amy Lee [:amylee] UX]

Yes I think so