Closed Bug 968483 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.0, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S2 (23may)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: caseyyee.ca, Assigned: mikehenrty)

References

Details

(Whiteboard: [systemsfe][p=8])

Attachments

(4 files, 14 obsolete files)

46 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
738.05 KB, image/jpeg
Details
46 bytes, text/x-github-pull-request
pivanov
: review+
Details | Review
217.59 KB, image/png
pabratowski
: ui-review+
Details
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
Attached image bug-968483.jpg (obsolete) —
Just want to get your OK on these screenshots so Pavel can proceed.
Attachment #8371057 - Flags: ui-review?(pabratowski)
Comment on attachment 8371057 [details]
bug-968483.jpg

ok.
Attachment #8371057 - Flags: ui-review?(pabratowski) → ui-review+
Assignee: nobody → pivanov
Attached image Shot (obsolete) —
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 :)
Attachment #8371272 - Flags: feedback?(pabratowski)
Attached image One more shot (obsolete) —
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 :)
Attached file patch for Gaia/master (obsolete) —
Attachment #8371883 - Flags: review?(kyee)
Comment on attachment 8371883 [details] [review]
patch for Gaia/master

Sorry, have to minus this one.  Has lots of bugs still.
Attachment #8371883 - Flags: review?(kyee) → review-
Attached image bug-968483-1.jpg (obsolete) —
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 on attachment 8371883 [details] [review]
patch for Gaia/master

I understand what is the problem ... I think it's fixed now :)
Attachment #8371883 - Flags: review- → review+
(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 on attachment 8371272 [details]
Shot

We decided not to center the subheaders for 1.4.
Comment on attachment 8371272 [details]
Shot

Make sure subheaders are left aligned with the rest of the settings text other than the headers.
Attachment #8371272 - Flags: feedback?(pabratowski) → feedback-
Hey Przemek,
does that mean that this but is invalid?
Flags: needinfo?(pabratowski)
Pavel, the comment was regarding sub-header.   We'll want to continue to proceed with centering the main header text.
Flags: needinfo?(pabratowski)
Hey Casey,

did you have time to review the patch?
Flags: needinfo?(kyee)
Pavel,
I'm still seeing all the issues that I'm seeing with my comments 7.
Flags: needinfo?(kyee)
Hey Casey,

are you sure that you flash your phone with my branch?
Flags: needinfo?(kyee)
Attached file bug-968483.zip (obsolete) —
Attachment #8372615 - Attachment is obsolete: true
Flags: needinfo?(kyee)
Attached image Shots (obsolete) —
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
Attachment #8376093 - Flags: feedback?(kyee)
Comment on attachment 8376093 [details]
Shots

Yup, this would do it :)
Attachment #8376093 - Flags: feedback?(kyee) → feedback+
Pavel, I'm still seeing alignment issues as per my screenshots in comment 17.
maybe you need the patch from Bug 972782 because we have [BB]Edit who has its own styles for headers
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
Attachment #8380008 - Flags: review?(kyee)
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.
Attachment #8380008 - Flags: review?(kyee) → review-
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)
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.
(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.
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?
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)
Flags: needinfo?(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).
Flags: needinfo?(kgrandon) → needinfo?(pivanov)
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
Flags: needinfo?(pivanov)
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)
Attachment #8380008 - Flags: review?(kyee)
Attachment #8380008 - Flags: review-
Attachment #8380008 - Flags: feedback?(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.
Attachment #8380008 - Flags: feedback?(kgrandon) → feedback+
Comment on attachment 8380008 [details] [review]
patch for Gaia/master - The CSS Version

As per IRC conversation.  Ellipsing before available space runs out.
Attachment #8380008 - Flags: review?(kyee) → review-
Attached image Screen Shot 2014-02-06 at 15.23.49.png (obsolete) —
Screen shot of issue as per comment 34
Can we extend that ellipses over... visually it looks like we easily have about 50 more pixels on the right?
Flags: needinfo?(kyee)
yep I think so ... we can try if you give me some numbers :)
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
Attachment #8380008 - Flags: review- → review?(kyee)
and one more thing we use "class name" instead of "data-role" now. I agreed with Kevin comment :)
Did we look at the impact this will have on localized versions of Firefox OS?
No longer depends on: 979483
(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.
Flags: needinfo?(kyee)
Attachment #8371883 - Attachment is obsolete: true
Attachment #8380008 - Attachment is obsolete: true
Attachment #8380008 - Flags: review?(kyee)
Attached file patch for Gaia/master (obsolete) —
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
Attachment #8388444 - Flags: ui-review?(kyee)
Attachment #8388444 - Flags: superreview?(kaze)
Attachment #8388444 - Flags: review?(arnau)
Attachment #8388444 - Flags: feedback?(kgrandon)
Attachment #8383988 - Attachment is obsolete: true
Attachment #8376093 - Attachment is obsolete: true
Attachment #8375647 - Attachment is obsolete: true
Attachment #8371286 - Attachment is obsolete: true
Attachment #8371272 - Attachment is obsolete: true
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.
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!
Flags: needinfo?(21)
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 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.
Attachment #8388444 - Flags: feedback?(kgrandon) → feedback+
Attachment #8388444 - Flags: superreview?(kaze) → superreview?(21)
Attached image [BB]Headers spec for v1.4 and v1.5 (obsolete) —
Spec for [BB]Headers
Comment on attachment 8388444 [details] [review]
patch for Gaia/master

Pavel, just make sure email is fixed.
r+
Attachment #8388444 - Flags: ui-review?(kyee) → ui-review+
Hi Pavel - I am not familiar with the superreview? flag. Why do we need it here? Just curious. Thanks!
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
Attachment #8388444 - Flags: superreview?(21) → superreview?(dbaron)
Flags: needinfo?(21)
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.
Attachment #8388444 - Flags: superreview?(dbaron)
David, that is exactly what I was hoping you'd say. :)
Sorry for the superreview It was a mistake
It's totally fine Pavel - we are all learning. I wasn't sure until David cleared it up for me. :)
Flags: needinfo?(kgrandon)
Attachment #8388444 - Attachment is obsolete: true
Flags: needinfo?(pivanov)
Attached file patch for Gaia/master (obsolete) —
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
Attachment #8389703 - Flags: ui-review?(kyee)
Attachment #8389703 - Flags: review?(arnau)
Attachment #8389703 - Flags: feedback?(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.
Flags: needinfo?(kgrandon)
Kevin,
Is there a way we could do this same tests without merging the patch?
(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 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.
Attachment #8389703 - Flags: review?(arnau) → review+
Pavel, can you try Kevin's link?
Attachment #8389703 - Flags: feedback?(kgrandon)
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!
Flags: needinfo?(dave.hunt)
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 :-)
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" :-)
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?
And can we land this patch in the mean time?
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.
Flagging Kevin to see if he can comment on test changes, and Jason.
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.
Flags: needinfo?(fabrice)
Attachment #8389703 - Flags: ui-review?(kyee) → ui-review+
Wait, what? We re-landed this *BEFORE* addressing the test issues? That's not acceptable.
if we landed with expected bustage, please backout
Flags: needinfo?(fabrice)
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.
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.
Flags: needinfo?(zcampbell)
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!
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 :-(
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!
Flags: needinfo?(zcampbell)
Zac is probably a better contact for this.
Flags: needinfo?(dave.hunt)
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
Kevin, can you help to apply this change? If not Pavel can do it.
Flags: needinfo?(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.
Attachment #8390613 - Flags: review?(kgrandon) → review+
Flags: needinfo?(kgrandon)
Thanks for the help guys :) Hope that this time everything will be OK

Landed to master: 

https://github.com/mozilla-b2g/gaia/commit/a71c4bf0a0e8ef949b6c03cac1e3be1d957f126e
Sorry, this regresses the header position in the Messages app :(
Just launch the app and you'll see it.
(or maybe that's expected ? :) )
can you shot it?
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 :)
(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!
Depends on: 983812
Depends on: 983810
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.
Keywords: checkin-needed
Depends on: 983837
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?
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!
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).
The relevant bugs that motivated the backout are:

* [smoketest blocker] bug 983810
* [smoketest blocker] bug 983837
* [ugly VD fallout] bug 983812
(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.
Blocks: 983810
No longer depends on: 983810
(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.
Flags: needinfo?(pivanov)
Flags: needinfo?(kyee)
(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.
Flags: needinfo?(pivanov)
Flags: needinfo?(kyee)
No longer blocks: 983810
Depends on: 983810
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)
(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.
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.
(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.
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
blocking-b2g: --- → 1.3?
blocking-b2g: 1.3? → 1.3T?
I'm probably missing something, but how can a bug marked as wontfix for 1.4 be 1.3T?
(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.
blocking-b2g: 1.3T? → -
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.
Assignee: pivanov → sjochimek
Status: RESOLVED → REOPENED
blocking-b2g: - → 2.0?
Flags: needinfo?(cserran)
Resolution: WONTFIX → ---
Target Milestone: --- → 2.0 S1 (9may)
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
Flags: needinfo?(swilkes)
Flags: needinfo?(pdolanjski)
Flags: needinfo?(cserran)
Whiteboard: [systemfe]
Whiteboard: [systemfe] → [systemsfe]
Blocks: SysFE
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
Blocks: 983812
No longer depends on: 983812
(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.
(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.
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 qa-b2g@mozilla.org when this lands to make sure that QA is aware of the known fallout.
Whiteboard: [systemsfe] → [systemsfe][p=8]
Assignee: sjochimek → mhenretty
Clearing the nom here since we're going to track features with the feature-b2g flag.
blocking-b2g: 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 :(
Attachment #8371057 - Attachment is obsolete: true
Attachment #8381473 - Attachment is obsolete: true
Attachment #8388444 - Attachment is obsolete: true
Attachment #8388782 - Attachment is obsolete: true
Attachment #8389703 - Attachment is obsolete: true
(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 :)
Flags: needinfo?(swilkes)
Flags: needinfo?(pdolanjski)
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.
Flags: needinfo?(pivanov)
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.
See Also: → 1005348
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.
Attached file patch for Gaia/master (obsolete) —
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 :)
Attachment #8419290 - Flags: review?(mhenretty)
Flags: needinfo?(pivanov)
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.
Attachment #8419290 - Attachment mime type: text/plain → text/x-github-pull-request
Attachment #8419290 - Flags: review?(mhenretty) → feedback+
feature-b2g: --- → 2.0
This is a work in progress from Arnau that has been rebased against the recent header web component work (with a fix from me).
Attachment #8419290 - Attachment is obsolete: true
Attached image 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.
Attachment #8422709 - Flags: ui-review?(pabratowski)
Comment on attachment 8421367 [details] [review]
Github PR, rebased back button patch

Casey or Pavel, mind giving this patch a look?
Attachment #8421367 - Flags: review?(pivanov)
Attachment #8421367 - Flags: review?(kyee)
Comment on attachment 8421367 [details] [review]
Github PR, rebased back button patch

LGTM :) Thanks :)
Attachment #8421367 - Flags: review?(pivanov) → review+
Comment on attachment 8422709 [details]
spritesheet.png

What I'm seeing here looks good.
Attachment #8422709 - Flags: ui-review?(pabratowski) → ui-review+
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
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.
master: https://github.com/mozilla-b2g/gaia/commit/c98e9aa5dfe414a307238951a47de4c703c333c8
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #8421367 - Flags: review?(kyee)
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.
(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.
(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.
Flags: in-moztrap?(nhirata.bugzilla)
we have tons of cases using the back button; it's pretty noticeable when doing these tests if they are not aligned properly.
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap-
You need to log in before you can comment on or make changes to this bug.