979473 - [META] [BB]Headers - Update header design to accommodate larger back-button

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

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

Attachment: patch for Gaia/master

Comment 2

[Casey Yee]

Attachment: bug-979473.jpg

Need visual once-over to make sure it's all acceptable.

Comment 3

[Casey Yee]

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

Some comments in PR that should be addressed.  Looks good otherwise  r+

Comment 4

[Casey Yee]

Attachment: bug-979473-1.jpg

Adjusted to font-weight: 200

Comment 5

[Stephany Wilkes]

Candice, can you recommend a front-end developer from the Sys FE team who might be able to take the r? flag? Thanks for your consideration!

Comment 6

[Candice Serran (:cserran)]

Gregor would be more appropriate in knowing which dev could review this

Comment 7

[Gregor Wagner [:gwagner]]

What target version is this? 1.4 or >1.4?

Comment 8

[Stephany Wilkes]

Gregor, this is for 1.4. Thank you!

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Stephany Wilkes from comment #8)
> Gregor, this is for 1.4. Thank you!

This needs to be stamped by the release owners.
Peter, any comment from your side if we should take this change that late in the game?

Comment 10

[Théo Chevalier]

Please, be sure this patch does not land before bug 908300 is fixed, it will lead to bad l10n regressions. I see bug 908300 is targeting 1.4, so it should be okay.

Comment 11

[Gregor Wagner [:gwagner]]

Bug 908300 is not targeted for 1.4. We will land right after we branch.

Comment 12

[Stephany Wilkes]

Gregor, right now bug #908300 is marked as targeting 1.4 S3 (14mar)...

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Stephany Wilkes from comment #12)
> Gregor, right now bug #908300 is marked as targeting 1.4 S3 (14mar)...

Yes because Sam is working on it so we can land right after we branch. It's way to risky to land it before we branch though.

Comment 14

[jachen]

Please see below for context.

On Mar 4, 2014, at 10:14 AM, Fabrice Desré <[e-mail]> wrote:

Hi all,

On 03/04/2014 10:09 AM, Jaime Chen wrote:
https://bugzilla.mozilla.org/show_bug.cgi?id=925929

> Jaime 
Hi Fabrice-
Hope you are doing well!

We've been working on making an update to the header to give MUCH better
affordance to the back button. See the bug above. The code is only CSS and we'd like to put in a request to land this as
an exception to the rules for 1.4.

> Fabrice: I see no reasons to not land that for 1.4 - we are not taking only
blockers on trunk, anything that is good and doesn't break the world can
land.

> Jaime: Our hope is that if we can land this, that we can cherry pick this into
the 1.3 taroko and fugu branches so that those low end devices can also
benefit from this work. I have spoken informally to James and Joe about
this possibility.

> Jaime: I've cc:ed Casey and Pavel who are finalizing the code as we speak and
would like to throw up the r? flags for the various app owners today. We
wanted to consult you first before proceeding further.

> Fabrice: To get it on Tarako you need to set it to 1.3?. Usually we like to wait
for a few days of testing in master before deciding on uplifts to
prevent churn if we have to backout. So adding the 'verifyme' keyword to
the bug once it has landed helps to get QA attention.

> Fabrice: So overall I don't foresee big problems with that one, and it's a
welcome improvement!

Fabrice
-- 
Fabrice Desré
b2g team
Mozilla Corporation

Comment 15

[Gregor Wagner [:gwagner]]

(In reply to jachen from comment #14)
> Please see below for context.
> 
> On Mar 4, 2014, at 10:14 AM, Fabrice Desré <[e-mail]> wrote:
> 
> Hi all,
> 
> On 03/04/2014 10:09 AM, Jaime Chen wrote:
> https://bugzilla.mozilla.org/show_bug.cgi?id=925929
> 
> > Jaime 
> Hi Fabrice-
> Hope you are doing well!
> 
> We've been working on making an update to the header to give MUCH better
> affordance to the back button. See the bug above. The code is only CSS and
> we'd like to put in a request to land this as
> an exception to the rules for 1.4.
> 
> > Fabrice: I see no reasons to not land that for 1.4 - we are not taking only
> blockers on trunk, anything that is good and doesn't break the world can
> land.
> 

It's correct that anything can land but that doesn't mean that anything should land. As Theo said, the risk is that we will get a ton of localization regressions. One after another and nobody knows when it stops. These issues are usually filed by people that use localized versions and we don't have many of them. I keep missing the back button all the time and would love to see this in 1.4 but we shouldn't repeat our mistakes over and over again and land half baked features late in the game.
We need at least a switch here that we can turn off this feature if it breaks too many things.

Comment 16

[Stephany Wilkes]

Casey is looking into whether or not this bug will actually cause localization regressions. It is not clear that this bug will actually do so.

In addition, Casey is looking into this bug and bug #908300 (which this bug was not noted as depending on until today) together to see what changes, if any, might be required for bug #908300 to play nicely with this work. It is, right now, not yet clear that this bug "undoes" or otherwise puts bug #908300 at risk. 

FWIW, UX has been huge supporters of bug #908300 from the start for the myriad issues it fixes, which we otherwise had to make one-off UX fixes for, and the initial recommendation came from our team (Eric). UX worked very hard to make sure bug #908300 found a home in the Sys FE backlog for implementation. It is absolutely not our goal to jeopardize the improvements in bug #908300, but at present Casey does not have a strong indication that this bug will even do that.

Comment 17

[Casey Yee]

(In reply to Théo Chevalier [:tchevalier] from comment #10)
> Please, be sure this patch does not land before bug 908300 is fixed, it will
> lead to bad l10n regressions. I see bug 908300 is targeting 1.4, so it
> should be okay.

Theo, I would like to hear about what l10n issues you are concerned about specifically.   We have made adjustments to the font-size and font-weight with this patch to make sure that our string lengths are not effected as part of the change.   We have also tested to make sure that LTR renders correctly as well.

I also don't see too much of a conflict in what is being done in 908300.  The markup and css changes look to be fairly easy to incorporate into any of the changes in this patch.

Comment 18

[Gregor Wagner [:gwagner]]

Ok lets do a proper risk analysis here.
We are changing the back button for every single app.
Possible regressions are:
1) Localization: Titles wouldn't show correctly any more.
We would have to check every single app in every single localization to make sure there is no regression. Casey is saying this is not a problem.
2) The actual size for the app reduces. Is this the case? Some apps might have some fixed height and the bottom might not be visible after this change.
We also have to check every single app if it displays correctly.

I can see that a switch might not be possible here and the only way to find out is testing.
Casey, Pavel, does this make sense?

Comment 19

[Casey Yee]

(In reply to Gregor Wagner [:gwagner] from comment #18)
> Ok lets do a proper risk analysis here.
> We are changing the back button for every single app.
> Possible regressions are:
> 1) Localization: Titles wouldn't show correctly any more.
> We would have to check every single app in every single localization to make
> sure there is no regression. Casey is saying this is not a problem.

I never said that there were not going to be issues.   I need to know what the specific L10N issues we are concerned about.   "Titles wouldn't show up correctly" doesn't give me much to work on.   I already stated what steps we have taken to keep string length in check and RTL working as it should through the change.

> 2) The actual size for the app reduces. Is this the case? Some apps might
> have some fixed height and the bottom might not be visible after this change.

Sorry, I don't know what you are referring to here.   We have left all the peripheral markup as is and there is nothing here that should effect the layout outside of the header itself.

> We also have to check every single app if it displays correctly.

Yes, and this is something that we are working through with QA as well as testing extensively on our end.    We have made large BB changes in the past and fully understand that there will be some bug fall-out as a result.

> 
> I can see that a switch might not be possible here and the only way to find
> out is testing.
> Casey, Pavel, does this make sense?


We won't be able to pref this off.  Just need to test it.

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Casey Yee [:cyee] from comment #19)
> I already stated what steps we have taken to keep string length 
> in check and RTL working as it should through the change.

Definitely my limit in understanding CSS, but: by reducing padding/margin you know exactly how many pixels are available before and after the change, but how can you know that a string is going to fit with a different font-size/font-weight when the font is not monospaced?
 
If a string is currently sitting at the very limit before truncation happens, can you be mathematically sure that is going to fit as well with the new font weight/size?

Comment 21

[Stephany Wilkes]

Jason, Casey and I met yesterday evening to create a plan around this bug and 908300. We all agree both are high value, for the major issues they fix, and high risk given their broad impact. This bug is easy to back out but, due to the way Building Blocks are implemented, is not easy to pref off. (This reinforces why a BB refactor is important.) QA estimates that this deserves about 5-10 days of testing and we understand.

There is a lot of worry because people have heard that this bug is gunning for a 1.3 cherry pick, a challenging thought (to say the least) given what everyone has just gone through to get 1.3 out the door. To clarify, we will be very happy if these bugs make it into 1.4 and they are clearly noted as such in Bugzilla.

This is what we propose given the above:

1. I am flagging dev app owners on each patch to review the patches in 979473 and 979478.

2. We agree we need to land the changes here in 979473 and bug #979478 in order to fully understand possible VxD impact, papercuts and regressions (which we have no automated way to catch and usually occur).

3. Casey is going to put all of the BB headers work here in 979473 and bug #979478 into a separate branch for testing. He will work with Pavel on this. Harly, Patryk and Przemek should also review this.

4. After this is done and tested, we will add bug #908300 to Casey's separate branch to assess the impact there. Sam, Casey may need to work with you on this. Any issues (which we expect will be minimal; Casey estimate a few lines of change in a copy-paste manner) will be sorted afterward.

5. Even after all of this, QA will ensure that 979473 and bug #908300 never land together, and that each gets a separate push. This makes it easier to carve one of them out later if needed.

Please let me know if you have any concerns or questions on the above plan. We are grateful to Jason for his NATO-level negotiation skills.

Comment 22

[Fabien Cazenave [:kaze]]

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

(drive-by mode) Pavel, your patch looks good but it introduces a lot of noise: a big part of this patch is caused by changes in the CSS property order or by changing single quotes to double quotes.

Please minimize this noise as much as possible: keep the CSS property order, keep the double-quotes (I think they’re our default for CSS files anyway), etc.  Your patch would be easier to review, to uplift… and to back-out if necessary.  Besides, it would keep the git history much simpler.

As we’re worrying about stability and want to minimize the risk wherever possible, I think it would really make a lot of sense to try minimizing your patch.

Comment 23

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

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

I'm sorry Pavel, but you could achieve the same result in this patch by keeping the menu in the header, so you would only need to patch: edit mode, drawer and modals instead of patching all apps as you are proposing here.
If you need to change the HTML structure I would suggest to do this right, like we did it in the headers refactor:
https://github.com/mozilla-b2g/Gaia-UI-Building-Blocks/tree/v2/style/headers

Comment 24

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

Hey guys,

Thanks for the reviews. I just realize (from Arnau's comment) that we don't need to change the markup at this moment. I will prepare a patch who not touch the markup and I will provide the explanation what we need for 1.4 and what we will need for 1.5

Comment 25

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

we don't need this meta bug anymore because we don't need to change the markup in each app. And we will keep the changes for [BB]Headers and [BB]Edit_mode in single patch in to bug 968483

Comment 26

[Thomas Ho [:tho]]

Bug invalid.