983812 - [B2G] UI: Headers in multiple apps are misaligned - shifted to the right

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Natalya Kot [:nkot]]

Attachment: screenshot

Description:
The headers in multiple apps appear misaligned - shifted to the right - see attached screenshot.

Repro Steps:
1) Updated Buri to BuildID: 20140314093423
2) Open different apps - Settings, Contacts

Actual:
The headers seems to be misaligned - shifted to the right 

Expected:
The headers display properly

Environmental Variables:
Device: Buri master (v1.4), Mozilla RIL
BuildID: 20140314093423
Gaia: 1cef557f9e9a865d1bf49d99a8f1cca1f0f4f5c4
Gecko: 142911d6d987
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Notes:

Repro frequency: 100%
See attached screenshot

Comment 1

[Jason Smith [:jsmith]]

This was a change done via bug 968483.

Comment 2

[Jason Smith [:jsmith]]

Should be fixed per the backout of the above bug - I've asked someone to verify that the issue no longer reproduces with the latest gaia changesets.

Comment 3

[Peter Bylenga [:PBylenga]]

I can verify that the issue no longer reproduces with the latest gaia changesets.

v1.4 Environmental Variables:
Device: Buri v1.4 MOZ
BuildID: 20140313131233
Gaia: d058b4851157dededea27157983087c1920d08da
Gecko: fe40387eba1a
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Comment 4

[Stephany Wilkes]

Guys, there is some confusion here. I know they look misaligned, but this is actually what the headers are supposed to look like right now. Bug #968483 is not the problem; we expected this as temporary horror until a better fix (another aspect of the visual refresh) in 1.5. Though it looks off center, it is centered to available space - i.e. between the buttons. We will "center to screen" in a subsequent release (per comment #90 over in bug #968483).

Comment 5

[Jason Smith [:jsmith]]

(In reply to Stephany Wilkes from comment #4)
> Guys, there is some confusion here. I know they look misaligned, but this is
> actually what the headers are supposed to look like right now. Bug #968483
> is not the problem; we expected this as temporary horror until a better fix
> (another aspect of the visual refresh) in 1.5. Though it looks off center,
> it is centered to available space - i.e. between the buttons. We will
> "center to screen" in a subsequent release (per comment #90 over in bug
> #968483).

I don't think that gives a strong argument why it's acceptable to damage one release at the notion of saying "let's fix the horror later even though we're introducing this now for the benefit of something else." We need do our diligence to avoid shipping with regressions, so we shouldn't introduce them openly to fix something else. This is especially important in this bug's case, as this problem cuts across the entire phone, which ends up damaging the overall interpretation of the phone design to our users (i.e. bad perception of quality).

Comment 6

[Stephany Wilkes]

Flagging Jaime and Patryk. This is not a regression: this is a design decision.

Comment 7

[jachen]

Created attachment 8391453 [details]

The screens attached are according to spec in 968483. 968483 is a phase 1 iterative approach to addressing the header. I think to call it a "temporary horror" is an incorrect description. 

There is another phase of design coming to the header in the next release.

The main goal for 968483 is to increase the area for the back button which is probably the #1 usability issue with this device.

Comment 8

[Jason Smith [:jsmith]]

(In reply to jachen from comment #7)
> Created attachment 8391453 [details]
> 
> The screens attached are according to spec in 968483. 968483 is a phase 1
> iterative approach to addressing the header. I think to call it a "temporary
> horror" is an incorrect description. 
> 
> There is another phase of design coming to the header in the next release.
> 
> The main goal for 968483 is to increase the area for the back button which
> is probably the #1 usability issue with this device.

The reality is that you didn't discuss this up front with the QA lead & dev lead on this area. This was allowed to land under the caveat that the regression risk was under control. If this was discussed up front as a known fallout, then we could have discussed this to make a call on whether to allow the fallout or not. However, that didn't happen, which I honestly think shows a lack of respect for your team leads, as we represent stakeholders in this decision.

This discussion is done - this doesn't land for 1.4 given the fact that the implementation failed to meet the quality requirements it needed to meet to land in the release.

Comment 9

[jachen]

Thanks for the reply Jason- lets discuss offline. I'll follow up on Monday.

Comment 10

[Stephany Wilkes]

Resolving this as WONTFIX for 1.4. 

We will follow up on the rest on Monday.

Comment 11

[Stephany Wilkes]

Reopening for 1.5/2.0. Will fix with JS header solution.

Comment 12

[Stephany Wilkes]

Who is going to take this? Arnau, Pavel?

Comment 13

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

Stephany, I'm not sure what you mean.
This bug is not reproducing anymore as 968483 was baked out, you mean if we are reopening 968483 to work on centered headers again?

Comment 14

[Jason Smith [:jsmith]]

We don't need to keep this bug open - this only happens if the regressing patch lands. Let's track everything in the original bug here.

Comment 15

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

Reopening because UX wants to land the back-button width increase (bug 968483) while having less than desirable header-text alignment temporarily. So the plan will be to land bug 968483, and then fix the alignment issue (which will probably require javascript) here.

See bug 968483 comment 119.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Michael Henretty [:mhenretty] from comment #15)
> Reopening because UX wants to land the back-button width increase (bug
> 968483) while having less than desirable header-text alignment temporarily.
> So the plan will be to land bug 968483, and then fix the alignment issue
> (which will probably require javascript) here.
> 
> See bug 968483 comment 119.

I disagree with this. See my followup comment in the bug.

Comment 17

[Jason Smith [:jsmith]]

Mike & I talked in IRC about this - it will be safer to land bug 968483 & fix this 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 18

[Stephany Wilkes]

Clearing the ni? on Pavel, as my question was "Who will do this work?" which is now Michael.

Comment 19

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

Attachment: [2.0] Header Centering Spec

Adding the latest spec here for reference.

Comment 20

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

Guillaume is going to help out with this one. Please see attachment 8423480 [details] for details.

Comment 21

[Guillaume Marty [:gmarty]]

Attachment: Github PR

Comment 22

[Guillaume Marty [:gmarty]]

Vivien, Kevin, can you please have a look at this PR? The code is not ideal.

Basically I have worked on 2 different implementations. The one in this PR is the one that doesn't trigger reflow, it also has much less code than the other one. But:
- The width of the buttons is hardcoded (as we can't get clientWidth)
- The resizing uses the dichotomy method to get the best size faster, but can take up to 3 reflows if the text is very long.
- I used transform:scale() instead of changing the font size to allow all the elements of the title to be resized consistently (see in the examples.html the header that says 'Inbox (2)').

Solving this issue is very hard and I can't see any easy way. Please let me know if this PR is on the right track.

Comment 23

[Kevin Grandon :kgrandon]

Comment on attachment 8425535 [details] [review]
Github PR

This looks good to me, but I don't think I can really give it an R+ yet. I think we need to land this in some app first, or have a pull request open with implementation in some app. It would also be good to understand what kind of performance impact this will have on launch latency - as it will definitely impact it somehow.

Comment 24

[Guillaume Marty [:gmarty]]

Thanks Kevin for the review. I addressed your remarks in the most recent PR.

When are we starting using these components in apps? If someone is going to work on measuring performance, then we need to test the impact of the header styling too.

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8425535 [details] [review]
Github PR

So I'm not sure who should put the final review on this, but I guess it would be good to flag wilson/yan on it.

I still think it would be worth doing some performance measurements before landing this. We could maybe hack up the template app and test with the following:
1 - vanilla header today
2 - gaia-header component without this patch
3 - gaia-header component with this patch

Also with the initial implementation in gaia-header, we have no choice but to execute and land these everywhere for 2.0. I hope we are all ready for some pain :)

Comment 26

[Yan Or]

I would recommend we have an implementation outside of gaia-header, using existing building block, if possible.  I'm not sure we are ready to switch to gaia-header for all apps in 2.0.

Comment 27

[Guillaume Marty [:gmarty]]

I measure the impact of the web component in load time.
I build 2 apps consisting of a bunch of headers one with traditional HTML, the other one with web components (gaia-header). I deactivated the font-fit/centering mechanism to get meaningful results.

The web component delay DOMContentLoaded and load events of about 250~300 ms.

DOMContentLoaded:
HTML: 170ms
WC:   450ms (+ 280)

Load:
HTML: 280ms
WC:   530ms (+250)

For some reason, I wasn't able to measure memory consumption.

I can check in these test apps to Github if you're interested.
What do you think of that?

Comment 28

[Kevin Grandon :kgrandon]

I suppose I was more interested inWC with and without the resize logic. Those numbers are useful though, so thank you for taking them!

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8425535 [details] [review]
Github PR

Looks good overall. But I would like to see a new version and have some answers.

Comment 30

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

Comment on attachment 8425535 [details] [review]
Github PR

A couple of things:

* As Yan said, we are going to need a non-Web Component version of this code as well. Given the timeline of this bug, we can't afford to wait on header WC to be ready for all apps.

* You don't need to do the auto-font resizing stuff in this bug (auto-resize is bug 908300), you just need to worry about the centering of the text.

* We are going to need a generalized solution here rather than basing this off of a fixed button width. It's true, we need to avoid reflows. But if measuring width of these buttons is done in a smart way (ie. only when textContent of a header changes), and the reflow is cheap (ie. a few milliseconds) I think it is ok to have a reflow.

* Also, I don't see us handling the case where the text is too wide to be centered based on screen width, or so needs to center on the container (ie, second from bottom of Long Header cases in attachment #8423480 [details]).

* We need to make this depend on FontSizeUtils from bug 908300, since we will need to know the fontSize before deciding how to center. Also, there are some nice utilities in there for measuring font widths, and listening for header changes. This way, we can just focus on the actual centering logic for this bug (which will still be tricky).

Comment 31

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

Attachment: PR, for info only, don't merge

Guillaume, here is how to integrate your centering logic to the existing FontSizeUtils. I'll try to sync up with you over IRC about this Monday morning UK time.

https://github.com/mikehenrty/gaia/commit/ed9b618711794a11d496af34ff4cc8b3616086a4

Comment 32

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

Comment on attachment 8432290 [details] [review]
PR, for info only, don't merge

Guillaume,

Thanks for your patch. I pulled it into this PR and added some comments inline (see all XXX comments, even in the css).

I also added a Header Test application that allows you to configure header text on a bunch of different configurations. This should be good for testing, and getting screenshots for UI-review.

Please address my comments, and then add some unit tests for the new functionality we added in font_size_utils.js.

Comment 33

[Guillaume Marty [:gmarty]]

My branch has been updated to address your comment (some are still open).

I fixed a few bugs in the centering and unit tested most of the new stuff, including changes on old stuff. Some tests are still red for me though :-(

When things are stabilised I guess next step will be to measure and improve performance? The performance of the centering mechanism needs checking.

Comment 34

[Guillaume Marty [:gmarty]]

Oh, BTW, my work branch is there, Mike and I are rebasing on each other's branch, but only him has a PR pending.
https://github.com/gmarty/gaia/tree/bug-983812-shift

Comment 35

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

Attachment: Github PR, with gmarty's centering logic

Had trouble rebasing my old PR with Guillaume's changes, so I created a new PR here. Some tests are failing, so this is not yet ready for review.

Comment 36

[Guillaume Marty [:gmarty]]

I had troubles with git so my new branch is here:
https://github.com/gmarty/gaia/tree/bug-983812-shifted3

More tests are passing now, there's just a single one failing.

Comment 37

[Guillaume Marty [:gmarty]]

Attachment: Perf for master

Comment 38

[Guillaume Marty [:gmarty]]

Attachment: Perf for bug 983812

Comment 39

[Guillaume Marty [:gmarty]]

I attached 2 perf test run with the SMS app with heavy load (1000 messages) for both the master branch and the current branch for this bug.
It ran on a Flame.

Above the fold event is delayed of ~25ms with this code in.
How does it sound?

Comment 40

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

Attachment: [Pull Request] Guillaume's centering logic with updated tests.

Updated some tests from Guillaumes branch. Guillaume, from here I think you can clean it up and test the crap out of it, and then flag for review :)

Comment 41

[Guillaume Marty [:gmarty]]

Attachment: Final Github PR

Comment 42

[Guillaume Marty [:gmarty]]

I updated the PR, the code has been clean and all test are green! Ready for review.

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8434932 [details] [review]
Final Github PR

I'm pretty sure we will trigger multiples reflows per text change with this. It would be good to format the code in a way where it does only 1 reflow per text change and have a test (using ReflowHelper) to ensure we don't regress.

I add some commnents on github as well.

Comment 44

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

Attachment: [Pull Request] Center with tests.

Ok, heres the latest PR. There is a problem though. Several of the apps insert Headers with innerHTML, and so it becomes hard for us to listen for textChanges on these elements since we have no idea when they get inserted.

The apps I've found that do this are:
Gallery, Email and Clock.

There are a few hacky approaches I have been thinking about, but for now it is probably best to fix these apps individually individually, and have them dispatch a CustomEvent whenever they add DOM nodes that could contain headers. This logic would then be removed once gaia header web components land.

Comment 45

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

Comment on attachment 8435571 [details] [review]
[Pull Request] Center with tests.

Just to report my findings in terms of reflows, this patch does _not_ add more than 1 reflow per header, and sometimes reformatting a header does not cause a reflow. I profiles 2 apps, performing a few actions in each. Here is what I found:

Settings       |  Master    |    with Centering
-------------------------------------------------------------
 -- load          27 reflows     28 reflows (2 format header calls)
 -- wifi panel    12 reflows     17 reflows (10 format header calls)
 -- Developer     5 reflows      7 reflows  (2 format header calls)


Contacts-heavy-work |  Master    |    with Centering
-------------------------------------------------------------
 -- load               116 reflows     117 reflows (2 calls)
 -- Contact Details    11 reflows      11 reflows (2 calls)

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Michael Henretty [:mhenretty] from comment #45)
> Comment on attachment 8435571 [details] [review]
> [Pull Request] Center with tests.
> 
> Just to report my findings in terms of reflows, this patch does _not_ add
> more than 1 reflow per header, and sometimes reformatting a header does not
> cause a reflow. I profiles 2 apps, performing a few actions in each. Here is
> what I found:
> 
> Settings       |  Master    |    with Centering
> -------------------------------------------------------------
>  -- load          27 reflows     28 reflows (2 format header calls)
>  -- wifi panel    12 reflows     17 reflows (10 format header calls)
>  -- Developer     5 reflows      7 reflows  (2 format header calls)
> 
> 
> Contacts-heavy-work |  Master    |    with Centering
> -------------------------------------------------------------
>  -- load               116 reflows     117 reflows (2 calls)
>  -- Contact Details    11 reflows      11 reflows (2 calls)

What I would like to avoid is that one reflow is done by the code that resize text, and an other is done by the code that centers it. I would like to merge the possible multiple reflows of those 2 calls, if possible.

So when you profile on master you already have taken into account the code that resize the font-size right ?

Comment 47

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

In all cases making a test that ensure that there is not more than one reflow when the header text content changes (with multiple form factors) would be enough. Then you won't need to convince me if there are some tests that prove that multiple reflows can't happen :)

Comment 48

[Guillaume Marty [:gmarty]]

Attachment: Final Github PR

Here is the latest PR. It contains all of the above +
* Exactly 1 reflow per call
* Integration test to prove that!
* Manual subscription to header formatting on clock, camera and mail apps

Vivien, can you review it?

Comment 49

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8435825 [details] [review]
Final Github PR

r+ with comments on github addressed. Please fix the indents and the small nits and then you should be good to go.

Comment 50

[Guillaume Marty [:gmarty]]

YAY! Thank you so much Vivien.
I updated the PR to address your remarks. Now waiting for Travis.

Comment 51

[Gregor Wagner [:gwagner]]

\o/

Comment 52

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

Attachment: [Pull Request] centering

Unit tests were failing for camera. Had to include FontSizeUtils with the test config in that app.

Comment 53

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

My Travis fork build: https://travis-ci.org/mikehenrty/gaia/builds/26986605

Gaia-Try: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=90e9f630cf55

Comment 54

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

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

Good job Guillaume!!

Comment 55

[Marcus Cavanaugh [:mcav]]

Thanks for your hard work on this, guys. I have one request: In the future, could you add a comment in front of new code that lands without review from the module owners? I know it's not always feasible to ask owners for changes like this, and that's totally understandable. It would just be nice to see something like "This event is triggered in support of new headers, and will be phased out when support for Web Components lands. See Bug XXXXXX.", so that other developers don't have to run `git blame` to figure out why an unknown piece of code landed in a module.

Regardless, thanks for doing this, I know it's especially hard under deadlines.

Comment 56

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

(In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #55)
> Thanks for your hard work on this, guys. I have one request: In the future,
> could you add a comment in front of new code that lands without review from
> the module owners? I know it's not always feasible to ask owners for changes
> like this, and that's totally understandable. It would just be nice to see
> something like "This event is triggered in support of new headers, and will
> be phased out when support for Web Components lands. See Bug XXXXXX.", so
> that other developers don't have to run `git blame` to figure out why an
> unknown piece of code landed in a module.

Absolutely right Marcus, we should have had comments in each of the places we were dispatching the lazyload event. This is an easy fix, so I'll file a follow up bug.