968483 - [Building Blocks] Update header design to accommodate larger back-button

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Casey Yee]

For 1.4 we will be modifying the header design to accommodate a larger back-button.

To support the larger back button and hit-state (increased to 5rem), we will be making a few additional changes:
- Center aligned header text.
- Increasing font weight to font-weight: 400
- Reduced header text sizing font-size: 2.2rem

Comment 1

[Casey Yee]

Attachment: bug-968483.jpg

Just want to get your OK on these screenshots so Pavel can proceed.

Comment 2

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8371057 [details]
bug-968483.jpg

ok.

Comment 3

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

Attachment: Shot

Hey Przemek,
what about subheaders? do we need to change the left space from 3rem to 5rem on them? I think that they look strange now :)

Comment 4

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

Attachment: One more shot

Actually it's not big deal and in the most cases we don't use the subheaders directly after the header but I just want to raise a flag (I'm pretty sure that you guys already have this in mind :)

Comment 5

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

Attachment: patch for Gaia/master

Comment 6

[Casey Yee]

Comment on attachment 8371883 [details] [review]
patch for Gaia/master

Sorry, have to minus this one.  Has lots of bugs still.

Comment 7

[Casey Yee]

Attachment: bug-968483-1.jpg

Attached are some issues that I experienced with the patch.

This one is tricky since the left navigation buttons are not always present.    The markup layout may need to change to make this work.

Comment 8

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

Comment on attachment 8371883 [details] [review]
patch for Gaia/master

I understand what is the problem ... I think it's fixed now :)

Comment 9

[Przemek Abratowski [:przemek] UX]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #3)
> Created attachment 8371272 [details]
> Shot
> 
> Hey Przemek,
> what about subheaders? do we need to change the left space from 3rem to 5rem
> on them? I think that they look strange now :)


We decided not to center the subheaders for 1.4.

Comment 10

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8371272 [details]
Shot

We decided not to center the subheaders for 1.4.

Comment 11

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8371272 [details]
Shot

Make sure subheaders are left aligned with the rest of the settings text other than the headers.

Comment 12

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

Hey Przemek,
does that mean that this but is invalid?

Comment 13

[Casey Yee]

Pavel, the comment was regarding sub-header.   We'll want to continue to proceed with centering the main header text.

Comment 14

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

Hey Casey,

did you have time to review the patch?

Comment 15

[Casey Yee]

Pavel,
I'm still seeing all the issues that I'm seeing with my comments 7.

Comment 16

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

Hey Casey,

are you sure that you flash your phone with my branch?

Comment 17

[Casey Yee]

Attachment: bug-968483.zip

Comment 18

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

Attachment: Shots

Hey Casey,

I find that the [BB]Edit have some custom styles for [BB]Headers I think that this was fixed with the refresh of [BB]Headers ... I will open a bug for this and I will prepare a patch for it.

The other apps looks OK to me

Comment 19

[Casey Yee]

Comment on attachment 8376093 [details]
Shots

Yup, this would do it :)

Comment 20

[Casey Yee]

Pavel, I'm still seeing alignment issues as per my screenshots in comment 17.

Comment 21

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

maybe you need the patch from Bug 972782 because we have [BB]Edit who has its own styles for headers

Comment 22

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

Attachment: patch for Gaia/master - The CSS Version

Have in mind that this patch not affect the [BB]Edit for now ... so all Apps in Edit Mode will not look like other Headers

Comment 23

[Casey Yee]

Comment on attachment 8380008 [details] [review]
patch for Gaia/master - The CSS Version

The solution is still not complete.   What we need is to have the header text center with the screen rather than the available space.

Pavel is continuing to look at CSS options but it seems likely that we will need some kind of JS to support this change.

Comment 24

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

the link of the new proposal is http://jsbin.com/bitebova/8/

Comment 25

[Anthony Ricaud (:rik)]

I'm not sure what is needed here. None of the screenshots are consistent with the descriptions I'm reading.
In Pavel's link from comment 24, examples 3 and 6 are not centered.

My understanding is that the text should be centered to the screen. When it's bigger than the space, it should be centered with an ellipsis. And when it's in between, it should be left or right aligned (depending on which side has more buttons) without ellipses. Right?

Are we sure all buttons will have the same width? (That can help design a "hacky" solution)

Comment 26

[Casey Yee]

As per email thread:

Hey everyone,
Pavel has been working hard on trying to get this to work in just pure CSS.   This is the best that it can get without a Js solution:
http://jsbin.com/bitebova/20/

It's not exactly what we wanted, but I think it'll have to do.   I personally think that its important to get a more useable back button and that it will be worth the miss-alignment for now.   We can work on a follow-up bug and js based solution which gets us exactly what we want after we land the css solution.  Pavel expects that the CSS solution as prototyped in the link above will take 1 week to land the patch + fixes that will be required in the apps to support the change.   

Thoughts?

I've also included Eteinne and Anthony since I've asked them to provide suggestions on what we might be able to do in terms of a potential Js solution in Gaia.   To get you up to speed on the discussion, here is the bug that is being discussed:
https://bugzilla.mozilla.org/show_bug.cgi?id=968483

Here is Pavels attempt at a Javascript solution:
http://pivanov.github.io/gaia/headers/index.html

The text on a few lines elllipses too early, but I do believe the fix shouldn't be too difficult of a problem to solve.    I think where I would really like to get your help is how to best integrate this header styling logic into Gaia.

Comment 27

[Casey Yee]

(In reply to Anthony Ricaud (:rik) from comment #25)
> I'm not sure what is needed here. None of the screenshots are consistent
> with the descriptions I'm reading.
> In Pavel's link from comment 24, examples 3 and 6 are not centered.

That's right.   None of the solutions so far fit the requirement.
> 
> My understanding is that the text should be centered to the screen. When
> it's bigger than the space, it should be centered with an ellipsis. And when
> it's in between, it should be left or right aligned (depending on which side
> has more buttons) without ellipses. Right?

The text should be centered to the screen width (not available space) as much as possible and ellipse when the string is too long.

> 
> Are we sure all buttons will have the same width? (That can help design a
> "hacky" solution)

The left navigation buttons (back, close, menu) will be always 50px, but the right buttons can be of any width.

Comment 28

[Kevin Grandon :kgrandon]

I'm coming into this late, and not entirely clear, but I can't see a reason for CSS from any of the mockups. Can you show me an example you're having trouble with a CSS implementation and I will see if I can help out?

Comment 29

[Patryk Adamczyk [:patryk] UX]

Attachment: In most cases there will be a button on the left or right side of the header... causing the header label to be off center. Its VERY noticable on device.

The header should be center aligned, since most labels are shorter, it going to look like a bug to users... especially when each screen the header's center point jumps left or right. 

The header label should be smart enough to know that if its over a certain number of characters and it may start to create an ellipsis than it will best align itself in the space provided (likely with a center point that is off center on the screen)

Comment 30

[Kevin Grandon :kgrandon]

Wouldn't one simple fix be to add a negative margin-left or negative left attribute to the centered text that matches the button width? I'm only just speculating here and have not really dug into the code yet (though I am more than happy to take a deeper look in the next few days).

Comment 31

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

Hey Kevin,

I think that the negative margin is not an option ... but I will send you (maybe tomorrow) a patch to see what is the problem

Comment 32

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

Comment on attachment 8380008 [details] [review]
patch for Gaia/master - The CSS Version

the known issues are:

1. when we have button/s with text who expand the "width" of the button	
2. when we have 3 buttons and a looooong title (small space for the title but is not so bad I think)
3. worst case ... when you have point 1 and point 2 together (usually - [BB]Edit Mode)

Comment 33

[Kevin Grandon :kgrandon]

Comment on attachment 8380008 [details] [review]
patch for Gaia/master - The CSS Version

I didn't try it on a device or anything, but seems like it should be fine. I would recommend using a class name instead of data-role, those are indexed and should perform better for selector matching iirc. (Faster css is generally better). I would be fine with button.navigation, or button.action selectors. Though most of BB is not too performant, so it's not a deal-breaker for me.

Comment 34

[Casey Yee]

Comment on attachment 8380008 [details] [review]
patch for Gaia/master - The CSS Version

As per IRC conversation.  Ellipsing before available space runs out.

Comment 35

[Casey Yee]

Attachment: Screen Shot 2014-02-06 at 15.23.49.png

Screen shot of issue as per comment 34

Comment 36

[Patryk Adamczyk [:patryk] UX]

Can we extend that ellipses over... visually it looks like we easily have about 50 more pixels on the right?

Comment 37

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

yep I think so ... we can try if you give me some numbers :)

Comment 38

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

Comment on attachment 8380008 [details] [review]
patch for Gaia/master - The CSS Version

This PR contains all "apps/" except "contacts" there are some js logic in headers and I will need help for it

P.S. as we talk on irc the "[BB]Headers" now have centered title to available space

Comment 39

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

and one more thing we use "class name" instead of "data-role" now. I agreed with Kevin comment :)

Comment 40

[Axel Hecht [:Pike]]

Did we look at the impact this will have on localized versions of Firefox OS?

Comment 41

[Casey Yee]

(In reply to Axel Hecht [:Pike] from comment #40)
> Did we look at the impact this will have on localized versions of Firefox OS?

Hi Axel,
We have been careful in making sure that we maintain the same amount of string space as we've always had by reducing the header text sizing.   RTL has also been considered in this change.

Comment 42

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

Attachment: patch for Gaia/master

Hey guys sore for the big noise of this till now. I will add explanation of what we want for 1.4 of [BB]headers, [BB]Edit_mode after few minutes so we can keep the discussion here. Sorry again and Thanks in advanced for the reviews

Comment 43

[Stephany Wilkes]

Kaze is out on PTO all week and, due to the Bugzilla attachment issues, I cannot flag Vivien or someone else on the superreview? Will ping people in IRC to find someone else to take this.

Comment 44

[Stephany Wilkes]

Vivien, I am flagging you on needinfo? as I cannot change the review? flag due to Bugzilla attachment issues today. Kaze is out on PTO this week and this patch needs to land ASAP so it can be tested in advance of Friday. Can you help or is there someone else you can suggest? Thanks!

Comment 45

[Stephany Wilkes]

Vivien, I am flagging you on needinfo? as I cannot change the superreview? flag due to Bugzilla attachment issues today. Kaze is out on PTO this week and this patch needs to land ASAP so it can be tested in advance of Friday. Can you help or is there someone else you can suggest? Thanks for any help you can provide here.

Comment 46

[Kevin Grandon :kgrandon]

Comment on attachment 8388444 [details] [review]
patch for Gaia/master

Ok, let some comments on the pull request. Overall I feel better about this patch than before - especially if we don't have to update app code. We do have to be *super* careful about this though, as I'm sure there are probably some regressions lurking about. (There always is when changing shared/ code). I wonder if we should loop in QA for some pre-emptive testing this week.

Comment 47

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

Attachment: [BB]Headers spec for v1.4 and v1.5

Spec for [BB]Headers

Comment 48

[Casey Yee]

Comment on attachment 8388444 [details] [review]
patch for Gaia/master

Pavel, just make sure email is fixed.
r+

Comment 49

[Stephany Wilkes]

Hi Pavel - I am not familiar with the superreview? flag. Why do we need it here? Just curious. Thanks!

Comment 50

[Stephany Wilkes]

Comment on attachment 8388444 [details] [review]
patch for Gaia/master

Based on this list, our superreview? flag should be set to David Baron. 
https://www.mozilla.org/hacking/reviewers.html

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8388444 [details] [review]
patch for Gaia/master

This patch does not require superreview; it's possible, however, that its author wanted review from a particular person for a particular reason, and used the superreview? flag rather than setting an additional review? flag.

I also don't see how https://www.mozilla.org/hacking/reviewers.html leads to me for a patch of this sort.

Comment 52

[Stephany Wilkes]

David, that is exactly what I was hoping you'd say. :)

Comment 53

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

Sorry for the superreview It was a mistake

Comment 54

[Stephany Wilkes]

It's totally fine Pavel - we are all learning. I wasn't sure until David cleared it up for me. :)

Comment 55

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

Landed to master:

https://github.com/mozilla-b2g/gaia/commit/32b5a2c8986332d50edde79517fe4fc83ae8519c

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/884bdee3d631623f05d01646b3843ce8768b90f2 because it broke gaia-unit tests on tbpl: https://tbpl.mozilla.org/php/getParsedLog.php?id=35961023&tree=B2g-Inbound

Comment 57

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

Attachment: patch for Gaia/master

Hey guys,

Armau and I found some problems with [BB]Edit_mode with pointer-events.
I fixed the patch and I made a new PR

Comment 58

[Kevin Grandon :kgrandon]

I wonder if that could've caused the TBPL failures? Looking at them, it doesn't mention edit mode specifically, but if something was causing pointer-events to not work it could've possibly caused the failure.

Comment 59

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

Kevin,
Is there a way we could do this same tests without merging the patch?

Comment 60

[Kevin Grandon :kgrandon]

(In reply to Arnau March  [:arnau] from comment #59)
> Kevin,
> Is there a way we could do this same tests without merging the patch?

You would have to use the gaia-try: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_Gaia

Comment 61

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

Comment on attachment 8389703 [details] [review]
patch for Gaia/master

My concern regarding edit mode has been solved in this patch, let's see if that is what was crashing the gaia-unit tests.

Comment 62

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

Pavel, can you try Kevin's link?

Comment 63

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! We have been trying to reproduce the issue using our forked repositories (in fact Arnau's one) and running Travis on them :-)

As you can see at https://travis-ci.org/rnowm/gaia/jobs/20612483 and more concretely at https://travis-ci.org/rnowm/gaia/jobs/20612483#L1827 the issue is related to expecting the element with id 'contact-name-title' (see https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/contacts/regions/contact_details.py#L11 and https://github.com/mozilla-b2g/gaia/blob/3b84e007c352f0afbe9d79ae21badbbf05984654/apps/communications/contacts/elements/details.html#L8) to have an x-location of 0. Since, as I have read throughout the bug is now centered, this condition may be no longer valid. So it is an issue with the test and not with the patch :-) Anyhow, since I am not familiarised with these tests (BTW, the error is always the same) I will need-info Dave Hunt who I saw has been providing part of the code for these tests.

I will prepare a JS integration test to check if this could be the issue and to have more information about it but I would say Dave will be able to provide us with much valuable information once he reads this comment :-)

Thanks!

Comment 64

[Germán Toro del Valle (:gtorodelvalle)]

I just confirmed that the x-location of the element with id 'contact-name-title' is now (after applying Pavel's patch) 50 and not 0 and consequently that condition is never met and, as a matter of fact, the element searching timeout fires :-)

No need to say, that I am not suggesting to substitute the 0 in the test by 50 (since this may change in the future and we would have the same problem) but a condition as general as possible (probably not dependant on elements locations :p) to validate that the conditions needed by the test to run and to pass are the correct ones ;-)

I'm sure Dave will know what I am talking about and also what to do :-)

Comment 65

[Germán Toro del Valle (:gtorodelvalle)]

In fact, the 50 I am getting is for the case when the form is shown to add a new contact, the language is English and the text shown at the header title is "Add contact" :-)

Comment 66

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

Thanks German. I guess 50 is the distance from the left to where the title starts. As in this patch we are centering all titles, that distance will change with the length of the word.
Is it possible to disable this test as it seems is no longer valid for our UI?

Comment 67

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

And can we land this patch in the mean time?

Comment 68

[Stephany Wilkes]

Who is the person who needs to disable the test? Is Dave Hunt the right person or do we need to ni? someone else.

Casey, how is the ui-review coming? This needs to move.

Comment 69

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

Landed to master:

https://github.com/mozilla-b2g/gaia/commit/5024a949f5a63dac3abf769264077191d9436010

Comment 70

[Stephany Wilkes]

Flagging Kevin to see if he can comment on test changes, and Jason.

Comment 71

[Stephany Wilkes]

Flagging Fabrice also since I asked Pavel to land in the meantime, while test issues are tracked down, since we are racing the clock here.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

Wait, what? We re-landed this *BEFORE* addressing the test issues? That's not acceptable.

Comment 73

[[:fabrice] Fabrice Desré]

if we landed with expected bustage, please backout

Comment 74

[Kevin Grandon :kgrandon]

Commits need to be atomic in nature, so we do need to land with a working test. This probably needs to be backed out and re-landed with a test when it fails. I would recommend Zac or someone from the A-team to update the test. I am fairly slammed with things, but if this is critical I could also try to find time this week.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Reverted.
https://github.com/mozilla-b2g/gaia/commit/642957d1b314d019a16729e272554c825338b2e4

Comment 76

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And it did indeed still fail: https://tbpl.mozilla.org/php/getParsedLog.php?id=36018564&tree=B2g-Inbound

Comment 77

[Stephany Wilkes]

Flagging Zac as he is on A-team. Also trying to find out who exactly is on A-team to see if someone in an earlier time zone can take this. If not, Zac, please let us know if you can help address our test change issues. We are under a lot of pressure to land this before Friday in good working order.

Comment 78

[Zac C (:zac)]

Here is the report from the failure on TBPL:
31aca2e57d9eee6b712c5de68463b1cf6c6f3bf302d44fbeb33d6e559de1667ae3acd726ccd9a2b97ec1e1559c6d8b3b8c96b528b0f02f26222285e297b7a4b9

Gtorodelvalle's debug sounds correct.
I can't debug this myself until tomorrow, but if you want to try again now you might have success with changing the element locator of the wait to: find_element(*self._back_button_locator) as it appears to be at ['x']==0 relative to the screen.

https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/contacts/regions/contact_details.py#L22

I'll keep my ni? in case this needs to be investigated tomorrow but if you fix it please clear it (and Dave's).

gtorodelvalle we have to wait very specifically like this often when there are slow transitions because there can be a race between Marionette calculating the location of the eleemnt (in order to perform and action like tap) and the final location of the element, it is very very sensitive!

Comment 79

[Germán Toro del Valle (:gtorodelvalle)]

I don't know if I get Zac's point, sorry :-) but yeah, yeah, we do these kind of checks in the Gaia integration tests (see https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/marionette/lib/contacts.js#L103 and also http://slid.es/gtorodelvalle/gaia-integration-tests#/30/0). The point is making these checks as independent as possible of the "environment" and the positioning of the elements, which BTW it is not always possible ;-)

I guess Zac is taking care of fixing the test. Is this right? :) I do not have the Marionette-Python environment set in my laptop to validate locally that they pass, I have to do it via Github + Travis :-(

Comment 80

[Zac C (:zac)]

I'm not fixing the test. It's your patch, you have to fix it. I just suggested a change that might work.

Gtorodelvalle, wait until you have to run these tests on a (slower) device too, I am sure lots of your waits will come crashing down!

Comment 81

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Zac is probably a better contact for this.

Comment 82

[Zac C (:zac)]

Hi guys I just debugged this locally and I confirmed that this line will work.

Put this on contact_details.py:22
self.wait_for_condition(lambda m: m.find_element(*self._back_button_locator).location['x'] == 0)

Cheers

Comment 83

[Stephany Wilkes]

Kevin, can you help to apply this change? If not Pavel can do it.

Comment 84

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

Attachment: patch for Gaia/master - with fixed test

Comment 85

[Kevin Grandon :kgrandon]

Comment on attachment 8390613 [details] [review]
patch for Gaia/master - with fixed test

Zac provided the fix and we have a green travis so I feel pretty good about this.

Comment 86

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

Thanks for the help guys :) Hope that this time everything will be OK

Landed to master: 

https://github.com/mozilla-b2g/gaia/commit/a71c4bf0a0e8ef949b6c03cac1e3be1d957f126e

Comment 87

[Julien Wajsberg [:julienw]]

Sorry, this regresses the header position in the Messages app :(
Just launch the app and you'll see it.

Comment 88

[Julien Wajsberg [:julienw]]

(or maybe that's expected ? :) )

Comment 89

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

can you shot it?

Comment 90

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

Julien, please check the attached VD specs in this patch: https://bug968483.bugzilla.mozilla.org/attachment.cgi?id=8388782
What you see in messages app is what you are supposed to see for 1.4 :)

Comment 91

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Zac C (:zac) from comment #80)
> I'm not fixing the test. It's your patch, you have to fix it. I just
> suggested a change that might work.
> 
> Gtorodelvalle, wait until you have to run these tests on a (slower) device
> too, I am sure lots of your waits will come crashing down!

No need to tell Zac that this was not my patch. I was the one who found the cause of the test not passing... ;-) You're welcome!

Comment 92

[Jason Smith [:jsmith]]

First - this should have been closed when it landed. Second - this caused a smoketest regression & a very ugly VD building blocks fallout. Naoki & I are in strong disagreement with comment 90 being a good idea, as the misalignment in the UX looks quite ugly on the phone.

Flagging checkin-needed for a backout. This also gets to wait until 1.5.

Comment 93

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reverted in https://github.com/mozilla-b2g/gaia/commit/d058b4851157dededea27157983087c1920d08da as per jsmith's comment 92.

Comment 94

[Stephany Wilkes]

Hey guys, we'd like to take a look at the smoke test results for this. Is there any way to point us to them?

Comment 95

[Stephany Wilkes]

Likewise for the "very ugly VD building blocks fallout," please. Casey has been testing on device and we just can't suss out what that comment means. If anyone has a screenshot, it would be helpful to us. Thanks!

Comment 96

[Casey Yee]

Hi Jason,
As per Steph, I would like a clarification on what caused the smoke test failure.   

Regarding alignment in comment 90 -- Which alignment option are you and Naoki in disagreement about?  I would be more than happy to clarify the design direction with you guys  (can do this offline from bug).

Comment 97

[Jason Smith [:jsmith]]

The relevant bugs that motivated the backout are:

* [smoketest blocker] bug 983810
* [smoketest blocker] bug 983837
* [ugly VD fallout] bug 983812

Comment 98

[Jason Smith [:jsmith]]

(In reply to Casey Yee [:cyee] from comment #96)
> Hi Jason,
> As per Steph, I would like a clarification on what caused the smoke test
> failure.   
> 
> Regarding alignment in comment 90 -- Which alignment option are you and
> Naoki in disagreement about?  I would be more than happy to clarify the
> design direction with you guys  (can do this offline from bug).

We can discuss this more offline if you like.

Comment 99

[jachen]

(In reply to Jason Smith [:jsmith] from comment #92)
> First - this should have been closed when it landed. Second - this caused a
> smoketest regression & a very ugly VD building blocks fallout. Naoki & I are
> in strong disagreement with comment 90 being a good idea, as the
> misalignment in the UX looks quite ugly on the phone.
> 
> Flagging checkin-needed for a backout. This also gets to wait until 1.5.

Hi Jason- I appreciate your attention to detail. The design team for FxOS has reviewed this design very intensely.

As I commented, in https://bugzilla.mozilla.org/show_bug.cgi?id=983812 the usability of the back button is one of the top issues for the device. This is phase 1 of the header design which will be iterated on in the next release.

The "very ugly VD" comment is duly noted but we have decided to proceed with this change in 1.4. 

Pavel, Casey, please restart the process to land.

Comment 100

[Jason Smith [:jsmith]]

(In reply to jachen from comment #99)
> (In reply to Jason Smith [:jsmith] from comment #92)
> > First - this should have been closed when it landed. Second - this caused a
> > smoketest regression & a very ugly VD building blocks fallout. Naoki & I are
> > in strong disagreement with comment 90 being a good idea, as the
> > misalignment in the UX looks quite ugly on the phone.
> > 
> > Flagging checkin-needed for a backout. This also gets to wait until 1.5.
> 
> Hi Jason- I appreciate your attention to detail. The design team for FxOS
> has reviewed this design very intensely.
> 
> As I commented, in https://bugzilla.mozilla.org/show_bug.cgi?id=983812 the
> usability of the back button is one of the top issues for the device. This
> is phase 1 of the header design which will be iterated on in the next
> release.
> 
> The "very ugly VD" comment is duly noted but we have decided to proceed with
> this change in 1.4. 
> 
> Pavel, Casey, please restart the process to land.

I don't think so. Did you read the above comments about the two smoketest blockers that came out of this patch outside of that other VD issue? You aren't allowed to land for 1.4. We do not cause smoketest regressions on landings. Period.

Anyways, this is non-blocking work anyways that was only allowed to land if the regression rate was under control. The smoketest regressions clearly prove that's not true, so this gets to wait until 1.5.

Comment 101

[Kevin Grandon :kgrandon]

A *much* less risky patch would be to simply increase the sizing of the back button a bit. I would highly recommend opening a bug for only that, then we can worry about center alignment in another patch. It seems like this patch is far too ambitious, when we should do it in smaller chunks.

I do not think that this patch would be very risky - as the back button used to be larger in the past anyway. To be clear, I think there should be two bugs filed:

1 - Increase sizing of back button (this is low risk, and I don't see why it couldn't be in 1.4).
2 - Centering of headers (this is higher risk, and should not be in 1.4)

Comment 102

[Francesco Lodolo [:flod]]

(In reply to Kevin Grandon :kgrandon from comment #101)
> I do not think that this patch would be very risky - as the back button used
> to be larger in the past anyway. To be clear, I think there should be two
> bugs filed:

I disagree: impact on en-US could be minor, for localized builds they would be unknown until testing, most likely bad considering the amount of truncation problems we have. See also discussion in bug 979473.

Comment 103

[Kevin Grandon :kgrandon]

We've already had the larger back button in the past, so the truncation issues should already be known. There may be some risk, but it's going to be a lot less than doing everything at once.

Comment 104

[Francesco Lodolo [:flod]]

(In reply to Kevin Grandon :kgrandon from comment #103)
> We've already had the larger back button in the past
Can you add some details? Which version are you referring to?

> There may be some risk, but it's going to be a lot less than doing everything at once.
In bug 979473 we were assured that in the end the available space wasn't going to change. In this, at least from what I understand, we're just reducing it, unless the plan is different (e.g. larger back button but reduce paddings or font size). This sounds like taking unknown risks to me.

Comment 105

[Stephany Wilkes]

Resolving this as WONTFIX for 1.4. The UX team did not land in time, and did not follow the process and timeline for testing that we agreed with QA.

Comment 106

[Francesco Lodolo [:flod]]

I'm probably missing something, but how can a bug marked as wontfix for 1.4 be 1.3T?

Comment 107

[Jason Smith [:jsmith]]

(In reply to Francesco Lodolo [:flod] from comment #106)
> I'm probably missing something, but how can a bug marked as wontfix for 1.4
> be 1.3T?

Yup that's right - this should have not been nomed. Way too risky for 1.3T anyways.

Comment 108

[Stephany Wilkes]

Reopening since this was moved to 2.0. Flagging Candice to adjust my suggested 2.0 sprint placeholder to actually suit the Sys FE backlog, assigning to Sam as discussed during planning, and putting the blocking placeholder in as this blocks visual refresh and BB changes.

Comment 109

[Candice Serran (:cserran)]

Hey Stephany, 

Do we have the new identified tasks/stories, that should be opened for these so we can make sure they get into our sprint planning? Also needinfo'ing Peter in case he has more info

Comment 110

[Michael Henretty [:mikehenrty][:mhenretty]]

Just to reiterate what we need to do here:

Fix the following two problems *BEFORE* relanding (otherwise we will block QA, and so this would be immediately backed out again):
* [smoketest blocker] bug 983810
* [smoketest blocker] bug 983837


Based on comment 99, it seems UX is ok with an iterative approach to fixing the center alignment issues. So as long as we don't block smoketests or bust auto-tests, I think we can land this bug and then (quickly thereafter) fix the alignment issue from bug 983812. I will reopen that bug for those purposes.
* [ugly VD fallout] bug 983812

Comment 111

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #110)
> 
> Based on comment 99, it seems UX is ok with an iterative approach to fixing
> the center alignment issues. So as long as we don't block smoketests or bust
> auto-tests, I think we can land this bug and then (quickly thereafter) fix
> the alignment issue from bug 983812. I will reopen that bug for those
> purposes.
> * [ugly VD fallout] bug 983812

I don't think that makes sense to me. We shouldn't forcibly cause a user-facing regression - it's going to cause noise to daily testers. If we get to the point where we have to make a call on whether we can live with the issue or not, then we can decide if this should land without bug 983812.

Comment 112

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #111)
> (In reply to Michael Henretty [:mhenretty] from comment #110)
> > 
> > Based on comment 99, it seems UX is ok with an iterative approach to fixing
> > the center alignment issues. So as long as we don't block smoketests or bust
> > auto-tests, I think we can land this bug and then (quickly thereafter) fix
> > the alignment issue from bug 983812. I will reopen that bug for those
> > purposes.
> > * [ugly VD fallout] bug 983812
> 
> I don't think that makes sense to me. We shouldn't forcibly cause a
> user-facing regression - it's going to cause noise to daily testers. If we
> get to the point where we have to make a call on whether we can live with
> the issue or not, then we can decide if this should land without bug 983812.

FWIW - I'd prefer that we minimize the halfway done VD refresh situations as much as possible. Our contracting team is already getting confused by some of the landings that halfway done, which is creating noise in Bugzilla.

Comment 113

[Jason Smith [:jsmith]]

Mike & I talked in IRC about this - it will be safer to land this & fix the other issue as a followup, so I'll reopen bug 983812. He'll email [e-mail] when this lands to make sure that QA is aware of the known fallout.

Comment 114

[Jason Smith [:jsmith]]

Clearing the nom here since we're going to track features with the feature-b2g flag.

Comment 115

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [BB] Headers spec for 2.0

Uploading the latest spec and cleaning out outdated attachments.

Sadly attachment 8390613 [details] [review] is very out of date with master, and will have to be almost entirely redone :(

Comment 116

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Candice Serran (:cserran) from comment #109)
> Hey Stephany, 
> 
> Do we have the new identified tasks/stories, that should be opened for these
> so we can make sure they get into our sprint planning? Also needinfo'ing
> Peter in case he has more info

For this bug specifically we have dependent bug 983812, which we will need to fix soon after landing this one.

We also have the truncated headers bug 908300, which is related to the header refresh. Fortunately, this can be worked on separately. I've attached the latest spec, attachment 8419084 [details], so I think the bugs we have now are enough to complete that work. We can file more if we need them :)

Comment 117

[Michael Henretty [:mikehenrty][:mhenretty]]

Pavel, would you be able to help me rebase your PR attachment 8390613 [details] [review] against the latest master? You probably know this code and the changes since better than anyone, so it would be great to have your help :)

At that point I can help with making sure all the auto/smoke tests pass, and with the landing process.

Comment 118

[Michael Henretty [:mikehenrty][:mhenretty]]

Just for informational purposes, :kgrandon is trying to drive changes in the web standards for css to allow us to not need javascript for things like centering text based on viewport, and auto-resizing font. He filed bug 1005348 to track that.

Comment 119

[Kevin Grandon :kgrandon]

Thanks for linking that. It's something that would be super nice to have I think, but there's no guarantees on how long this would take or if it's even going to happen quite yet. My guess is that even if we were able to standardize something, it'd still be a year+ away. Whatever solution we do now needs to be performant and reusable.

Comment 120

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

Attachment: patch for Gaia/master

Hey Michael,
here is the patch with the large back button ... and some small CSS improvements ping me if I can help with something else :)

Comment 121

[Michael Henretty [:mikehenrty][:mhenretty]]

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

Thanks for the quick turnaround Pavel!

Looks like we have some tests to fix, I will be looking at that today.

Comment 122

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR, rebased back button patch

This is a work in progress from Arnau that has been rebased against the recent header web component work (with a fix from me).

Comment 123

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: spritesheet.png

Przemek, would you have a look at these screens? Keep in mind we are doing the proper centering in bug 983812, so this is just centering the text based on the container.

Comment 124

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8421367 [details] [review]
Github PR, rebased back button patch

Casey or Pavel, mind giving this patch a look?

Comment 125

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

Comment on attachment 8421367 [details] [review]
Github PR, rebased back button patch

LGTM :) Thanks :)

Comment 126

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8422709 [details]
spritesheet.png

What I'm seeing here looks good.

Comment 127

[Michael Henretty [:mikehenrty][:mhenretty]]

I had QA run through the smoke tests with me, we found a slight style bug in the camera app. That is now fixed. We have ui-review+, code review+, and travis looks green. Time to land.

Comment 128

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/c98e9aa5dfe414a307238951a47de4c703c333c8

Comment 129

[Stephany Wilkes]

OK Jason - but just a note that this DOES block visual refresh for 2.0 AND the completion of the header. :) Header isn't done without this, in other words. Just tossing my $.02 into the pool.

Comment 130

[Johan Lorenzo [:jlorenzo]]

(In reply to Michael Henretty [:mhenretty] from comment #127)
> I had QA run through the smoke tests with me, we found a slight style bug in
> the camera app.

I confirm. We found this only bug by running the smoketests.

Comment 131

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Stephany Wilkes from comment #129)
> OK Jason - but just a note that this DOES block visual refresh for 2.0 AND
> the completion of the header. :) Header isn't done without this, in other
> words. Just tossing my $.02 into the pool.

I'm not sure which comment you are replying to, but we also have bug 983812 to track the alignment issue, and that is indeed blocking 2.0.

Comment 132

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

we have tons of cases using the back button; it's pretty noticeable when doing these tests if they are not aligned properly.