1059967 - [SHB] Update SHB styling according to v1.0 UX specification

Status: RESOLVED FIXED

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

Description

[:Eli Perelman]

In the updated UX spec for the Software Home Button, the height of the bar which contains the SHB was defined as having a height of 4.5rem. However, the height of the SHB as it's currently implemented is 5rem. We need to adjust the height of the SHB and its dependent UI elements to reflect this updated height.

Comment 1

[:Eli Perelman]

Obsoleting the original description.

This bug now encompasses making all the required CSS changes to the SHB to reflect the updated UX specification attached in bug 1036339.

Comment 2

[:Eli Perelman]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23646

Etienne, my current change correctly fixes the 1px on a Flame, but obviously wreaks havoc on the tests in system/test/unit/layout_manager_test.js [1]. Would you mind looking over my current change to layout_manager [2] and providing some feedback as to what steps you recommend I take, whether it be modifying the tests or going with something different in layout manager? Thanks!

[1] https://tbpl.mozilla.org/?rev=0bb3dce4e4d5eaf53d34ff4f1c7d49644b4bdea4&tree=Gaia-Try
[2] https://github.com/mozilla-b2g/gaia/pull/23646/files

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 8483118 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23646

I still think we should tweak the size of the software home button and not the layoutManager.

That said if we get the go from Benoit about this approach not regressing bug 1027231 we'll make it work.

Other notes:
- we still get a gap when opening the utility tray
- have you talk with Markus about using CSS variables like we said?
- if we keep the getBoundingClientRect we'll need to cache it, we don't want to reflow at every layoutManager call.

Comment 4

[:Eli Perelman]

Etienne:

1. I was able to successfully implement CSS variables for this part, so yay!
2. Shifting the height of the SHB from 4.5rem to 4.4rem fixes the 1px gap issue. While this fixes the issue on the Flame in the here and now, I'm not sure this gap will not re-manifest itself on devices with different device densities like Madai with 2.25dpx.
3. I can change this if we come to the decision of 4.4rem vs. 4.5rem.

So from here, Peter, could you please let us know on the necessity of 4.5rem? Moving to 4.4rem wasn't noticeable on my Flame seeing as it's a 1px difference visually. Also something for everyone to keep in mind scope-wise is that changing to 4.4rem is a single character change and fix, while making changes to Layout Manager is larger and much riskier to 2.1.

Comment 5

[:Eli Perelman]

ni? for comment 4.

Comment 6

[Peter La]

Hi Eli,

Re: 4.4 rem vs 4.5 rem.  I think for this release, your 4.4 rem solution probably strikes the best balance between our goal (aligning with toolbar height) and practicality (reducing scope of changes and hence risk).  So I'm fine with 4.4 rem for this release.  In the future it would be good to figure out a way to fix it.  I am noticing right now that our toolbars are actually also about .1 rem off in places, so maybe at some point we can align them all properly to 4.5 for all devices and solve the 1 px gap problem.

Comment 7

[:Eli Perelman]

Great, thank you Peter! Like I said, in order to fix some of those things for other devices, I foresee having to make a change to Layout Manager, but Etienne would know best. Maybe we can make that a goal for 2.2?

Comment 8

[:Eli Perelman]

Holding off on the review until bug 1061962 lands so I can include its changes to be included in the CSS variables.

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8483118 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23646

Impressive work, 2 comments

* I'm afraid that creating a shared/style/global.css is opening pandora's box. I'd like to start with a system/style/definitions.css for now and see how it evolves.
* The fullscreen software home button is not centered properly

PS: not related to the review but looks like we might need to go with your getBoundingClientRect in the end for bug 1059683

Comment 10

[:Eli Perelman]

Etienne, I was able to move all system-related style changes back to the system app which let me move the variable there as well.

Could you post a screenshot of the fullscreen SHB not being centered. I am unable to replicate.

Comment 11

[Etienne Segonzac (:etienne)]

(In reply to :Eli Perelman from comment #10)
> Etienne, I was able to move all system-related style changes back to the
> system app which let me move the variable there as well.
> 
> Could you post a screenshot of the fullscreen SHB not being centered. I am
> unable to replicate.

So many different fullscreens :) I meant the one when you mozRequestFullScreen() I use the UITest app to test it (there's a fullscreen test page inside)

Comment 12

[Jason Smith [:jsmith]]

Removing from meta as this is not a blocker for 2.1.

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8483118 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23646

Awesome, thanks for taking the time to do this the right way!

Comment 14

[:Eli Perelman]

My pleasure!

Comment 15

[Kevin Grandon :kgrandon]

Since this has an R+ and green gaia-try, I'll go ahead and land this if that's ok as it's blocking one of my bugs :)

In master: https://github.com/mozilla-b2g/gaia/commit/8fa931b1c2b72e459a78d8b3e49effc6959d9476

Comment 16

[:Eli Perelman]

Comment on attachment 8483118 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23646

Requesting uplift to v2.1 for feature polish.

[Bug caused by] (feature/regressing bug #): bug 1036339
[User impact] if declined: legacy style SHB
[Testing completed]: yes, manual
[Risk to taking this patch] (and alternatives if risky): moderately low, only style changes, but across several files and entry points
[String changes made]: n/a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/4b5c09c778d309900e0c62186ed811f61a505527