Closed Bug 983812 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.0)

RESOLVED FIXED
2.0 S3 (6june)
feature-b2g 2.0

People

(Reporter: nkot, Assigned: gmarty)

References

Details

(Keywords: regression, Whiteboard: [systemsfe][p=5])

Attachments

(5 files, 7 obsolete files)

99.11 KB, image/png
Details
738.05 KB, image/jpeg
Details
6.85 KB, application/json
Details
6.77 KB, application/json
Details
46 bytes, text/x-github-pull-request
mikehenrty
: review+
Details | Review
Attached image 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
This was a change done via bug 968483.
Blocks: 968483
Keywords: regression
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.
Keywords: qawanted
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
Keywords: qawanted
QA Contact: pbylenga
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Resolution: WORKSFORME → FIXED
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).
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(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).
Flags: needinfo?(jsmith)
Flagging Jaime and Patryk. This is not a regression: this is a design decision.
Flags: needinfo?(padamczyk)
Flags: needinfo?(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.
Flags: needinfo?(jachen)
(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.
Thanks for the reply Jason- lets discuss offline. I'll follow up on Monday.
Resolving this as WONTFIX for 1.4. 

We will follow up on the rest on Monday.
Flags: needinfo?(padamczyk)
Flags: needinfo?(nhirata.bugzilla)
Resolution: FIXED → WONTFIX
Blocks: 965889
Reopening for 1.5/2.0. Will fix with JS header solution.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Depends on: 908309
No longer depends on: 908309
blocking-b2g: --- → 2.0?
Who is going to take this? Arnau, Pavel?
Flags: needinfo?(pivanov)
Flags: needinfo?(arnau)
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?
Flags: needinfo?(arnau)
Whiteboard: [systemsfe]
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.
Status: REOPENED → RESOLVED
blocking-b2g: 2.0? → ---
Closed: 6 years ago6 years ago
Resolution: --- → WORKSFORME
Assignee: nobody → sjochimek
Target Milestone: --- → 2.0 S1 (9may)
Blocks: SysFE
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.
No longer blocks: 968483
Status: RESOLVED → REOPENED
Depends on: 968483
Resolution: WORKSFORME → ---
Assignee: sjochimek → mhenretty
(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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WORKSFORME
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 qa-b2g@mozilla.org when this lands to make sure that QA is aware of the known fallout.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Whiteboard: [systemsfe] → [systemsfe][p=5]
feature-b2g: --- → 2.0
Clearing the ni? on Pavel, as my question was "Who will do this work?" which is now Michael.
Flags: needinfo?(pivanov)
Adding the latest spec here for reference.
Guillaume is going to help out with this one. Please see attachment 8423480 [details] for details.
Assignee: mhenretty → gmarty
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
Attached file Github PR (obsolete) —
Attachment #8425535 - Flags: review?(kevingrandon)
Attachment #8425535 - Flags: review?(21)
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.
Attachment #8425535 - Flags: review?(kevingrandon) → feedback?(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.
Attachment #8425535 - Flags: feedback?(kgrandon)
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 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 :)
Attachment #8425535 - Flags: feedback?(yor)
Attachment #8425535 - Flags: feedback?(wilsonpage)
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.
Attachment #8425535 - Flags: feedback?(yor)
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?
I suppose I was more interested inWC with and without the resize logic. Those numbers are useful though, so thank you for taking them!
Attachment #8425535 - Flags: feedback?(wilsonpage) → feedback+
Comment on attachment 8425535 [details] [review]
Github PR

Looks good overall. But I would like to see a new version and have some answers.
Attachment #8425535 - Flags: review?(21) → feedback+
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
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).
Attached file PR, for info only, don't merge (obsolete) —
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 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.
Flags: needinfo?(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.
Flags: needinfo?(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
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.
Attachment #8425535 - Attachment is obsolete: true
Attachment #8432290 - Attachment is obsolete: true
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.
Attached file Perf for master
Attached file Perf for bug 983812
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?
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 :)
Attachment #8433675 - Attachment is obsolete: true
Attached file Final Github PR (obsolete) —
Attachment #8434833 - Attachment is obsolete: true
Attachment #8434932 - Flags: review?(21)
I updated the PR, the code has been clean and all test are green! Ready for review.
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.
Attachment #8434932 - Flags: review?(21)
Attached file [Pull Request] Center with tests. (obsolete) —
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.
Attachment #8434932 - Attachment is obsolete: true
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)
(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 ?
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 :)
Attached file Final Github PR (obsolete) —
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?
Attachment #8435571 - Attachment is obsolete: true
Attachment #8435825 - Flags: review?(21)
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.
Attachment #8435825 - Flags: review?(21) → review+
YAY! Thank you so much Vivien.
I updated the PR to address your remarks. Now waiting for Travis.
Unit tests were failing for camera. Had to include FontSizeUtils with the test config in that app.
Attachment #8435825 - Attachment is obsolete: true
Attachment #8436152 - Flags: review+
master: https://github.com/mozilla-b2g/gaia/commit/86bb40777dea46bcd1eeb7d542e1988bf871d86e

Good job Guillaume!!
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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.
(In reply to Marcus Cavanaugh [:mcav] <mcav@mozilla.com> 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.
You need to log in before you can comment on or make changes to this bug.