890443 - [SMS] the "fixed header" style and transitions should be adjusted so that it's not jumping when changing header

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Leo]

1. Title: They are display errors in message thread view
2. Precondition: Have several threads from at least three days in Messaging app
3. Tester's Action:Go to Messaging app and scroll through the threads view, slow, fast, etc. Observe the 'Day' bar: 'Today', 'Yesterday' etc.
4. Detailed Symptom (ENG.) : There are two problems when scrolling :the top bar below 'Today' sometimes is doubled, sometimes no, sometimes the space between two bars are different etc. Moreover sometimes the 'Today' bars are doubled 
5. Expected:  There shouls not be any problem with displaying bars in messaging application
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on v1-train
8. Gaia Revision:  613e69ee0a009399130ad2cbb8b9d0462cd1fc70
9. Personal email id: [e-mail]

Comment 1

[Borja Salguero [:borjasalguero]]

This is a regression about 'fixed-header'. I will take a look!

Comment 2

[(Inactive after June) George Duan [:gduan] [:喬智]]

It seems a duplicate of bug 885278 .

Comment 3

[Julien Wajsberg [:julienw]]

Some of the bug is bug 885278 however I've also noticed the subtle display problems, like double horizontal bar, etc. I think we may have a slightly different style between the fixed header and the normal headers.

Also, the transition used when changing the header should probably be adjusted, because we can see that the header is "jumping" when the transition is finished.

This bug could be used to fix this.

Comment 4

[Julien Wajsberg [:julienw]]

Renominating, because I think the subtle display problems should not be leo+. Leo+ Bug 885278 is already handling most (if not all) of the functional problems.

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

hi Julien, does it make sense to mark this one as a dup of bug 885278? if bug 885278 covers this bug. Thanks

Comment 6

[Borja Salguero [:borjasalguero]]

Julien, as you know currently the process for getting a leo+ it's quite hard, so we should avoid to renominate without taking into consideration that this bug *probably* is considered in a different way by our partners (for example in this case LG). Please mark as DUP if you think this is the same, or ask the reporter for reconsidering the leo+ status. Thanks!

Comment 7

[Julien Wajsberg [:julienw]]

Bug 885278 covers the "functional" aspects, I mean, the header text that is not refreshed and stuff like this.

I'd like this bug to cover all the "cosmetic" aspects, I mean, when it's jumping a little while you scroll, when there is double bars, when there is sometimes a shifted header behind another header.

To me, the "cosmetic" aspects are not a blocker, and that's why I've renominated. But I understand if our partners judge that this is blocking. I wanted to make sure we were not trying to block for 2 different things here.

Comment 8

[Wayne Chang [:wchang]]

Leo considers this blocking although cosmetic. Leo+ per request.

Leo, please test with patch from bug 885278 to see if this problem is covered.

Borja, will you track this bug if not fixed by 885278? Thanks

Comment 9

[Julien Wajsberg [:julienw]]

Borja, then I can take this bug as well if you want, since my eyes are on this code these days :)

Comment 10

[Borja Salguero [:borjasalguero]]

I thinks is a really good idea Julien. Let me know when the patch will be ready and I'll take a look. Thanks!

Comment 11

[Leo]

Hi Wayne,
We have tested the patch from bug 885278,but the issue is still reproducible.

(In reply to Wayne Chang [:wchang] from comment #8)
> Leo considers this blocking although cosmetic. Leo+ per request.
> 
> Leo, please test with patch from bug 885278 to see if this problem is
> covered.
> 
> Borja, will you track this bug if not fixed by 885278? Thanks

Comment 12

[Julien Wajsberg [:julienw]]

I have a good idea on how to fix this, will provide a patch during the week-end.

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: patch v1

PR is https://github.com/mozilla-b2g/gaia/pull/10963

Unit tests are passing, and I didn't need to change the existing one, which is satisfying :)
I did some heavy tests on the device in lots of cases but please test it as well :)

Now the "hiding effect" when scrolling should work as expected.

Important changes
* changes how the fixed header is found. Now we're iterating starting with the
  top instead of the bottom. In the SMS app, the most common cases is when the
  list is not scrolled so much, and therefore starting at the top will likely be
  way faster once the list is long.
* we don't position the header in the same loop, now we position the header
  depending on the next header, if it exists.
* we use a specific hiding class instead of specifying the 100% transform
  translation in the js code. Also, using a 100% transform translation is used
  instead of display none, so that we can get some necessary measurements in the
  init method. Most notably, we need to take into account the view's padding
  top.

Less important changes:
* changed the desktop mock so that we have more headers when loading in Firefox
* check that we have mozSetMessageHandler before using it so that we can launch
  the app in main Firefox (Corey and I wanted to do that for a long time)
* moved the first header's margin top to the view's padding top. BTW this
  removes a building blocks override.
---
 apps/sms/index.html                     |    2 +-
 apps/sms/js/activity_handler.js         |    3 +
 apps/sms/js/desktop_sms_mock.js         |   14 ++---
 apps/sms/js/fixed_header.js             |  105 +++++++++++++++++++++----------
 apps/sms/style/sms.css                  |   25 ++++----
 apps/sms/test/unit/fixed_header_test.js |    5 ++
 6 files changed, 101 insertions(+), 53 deletions(-)

Comment 14

[Julien Wajsberg [:julienw]]

Borja, since I'm in PTO, feel free to land if you r+, or fix the nits yourself and ask for another review from fcampo or one of the Sms peers.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v2

fixed the small regression where the added padding was also in the messages view.

Github PR is still https://github.com/mozilla-b2g/gaia/pull/10963

Comment 16

[Borja Salguero [:borjasalguero]]

As Julien is on PTO, I will take this and I will apply latest suggestions to Julien's patch. Steve will be the final reviewer! :)

Comment 17

[Borja Salguero [:borjasalguero]]

Taking! I'll start working on this issue this evening.

Comment 18

[Wayne Chang [:wchang]]

Hi Borja, did you make any progress with this here?

Comment 19

[Borja Salguero [:borjasalguero]]

Hi Wayne! Yep I was taking a look to this, and I hope to have it working soon (because there are a lot of scenarios to test in this case). Thanks!

Comment 20

[Borja Salguero [:borjasalguero]]

Attachment: Small fix

This is a quite small fix for the issue. Julien's patch includes a deep refactor (and the idea behind is good), but after testing on the device I've realized that some scenarios are not working as expected. I think that this refactor could be done in a followup, and apply here the patch for fixing the transition. Steve could you take a look? Thanks!

Comment 21

[Steve Chung [:steveck]]

(In reply to Borja Salguero [:borjasalguero] from comment #20)
> Created attachment 779685 [details]
> Small fix
> 
> This is a quite small fix for the issue. Julien's patch includes a deep
> refactor (and the idea behind is good), but after testing on the device I've
> realized that some scenarios are not working as expected. I think that this
> refactor could be done in a followup, and apply here the patch for fixing
> the transition. Steve could you take a look? Thanks!

Hi Borja, I just left some comments on github, thanks.

Comment 22

[Steve Chung [:steveck]]

Attachment: Fix header does not update properly after scroll to top quickly

I can still reproduce this problem after testing... Please check the attachment image and description:
1) Scroll down the view and make 2 header displayed at the same time on the top of the view(the former header must scroll up to middle).
2) Scroll up quickly. the former header will not update as expected. It will still stay as half-scrolled like step(1).

Comment 23

[Borja Salguero [:borjasalguero]]

After testing again it's true that there are scenarios where this is not working, and I've realized that the same happens in the Dialer... I would like to apply the same solution in both, and review and add tests to 'FixedHeader' for ensuring that it's working as expected in all apps. Julien, Wdyt?

Comment 24

[Borja Salguero [:borjasalguero]]

Attachment: Same issue in the dialer

Comment 25

[Borja Salguero [:borjasalguero]]

Actually I would like to see something like `display: sticky`... Do you know if there is any work related for adding this to Gecko?

Comment 26

[Borja Salguero [:borjasalguero]]

Something like https://bugzilla.mozilla.org/show_bug.cgi?id=886646 :)

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

need a non-PTO assignee here to address this timely... Steve, do you available to work on this? If not, could you find a peer to work on this?

Comment 28

[Steve Chung [:steveck]]

Since Julien already submitted the patch and the problem is not fixed completely in Borja's patch , I'll review Julien's patch first.

Comment 29

[Julien Wajsberg [:julienw]]

I can take it from here if you want :)

There is a very small bug left in my patch, I can fix it during the week end so that you can review it monday first thing, Steve :)

I've seen some (smaller) bugs in the other apps as well, might make sense to see if my fixes here fix them too. I'll file new bugs for this. The bug in SMS is bigger because of the padding that is at the top of the list, and that this patch takes into account.

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 775396 [details] [diff] [review]
patch v2

Remove the review flag for now, there is a small bug to fix first.

Comment 31

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Fixed the last bug I discovered while I was in holiday: when the last thread group was high enough, and we were scrolling it, the fixed header was not set. I wrote unit tests to check this as well.

The PR is still at https://github.com/mozilla-b2g/gaia/pull/10963 (there are some explanations there too !).

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 782314 [details] [diff] [review]
patch v3

Hi Julien,
This patch works great except one small issue for me. Here is the reproduce step:
1) Push some the reference test database.
2) Create some new conversation threads (2 or 3 is enough)
3) Verify the fix header. The fix header will become scrollable, but it will be fixed after restart the app.

The problem might be refreshing the fixHeader in thread_ui view. It will call FixedHeader.init
first while renderThreads, but it will make the header hide because currentHeader not exist. fixedContainer hide class will not be removed after that. Maybe we need to force the fixedContainer remove the hide if currentHeader exist or other better way to make it works perfectly. Please ping me after you fix this small issue, thanks!

Comment 33

[Julien Wajsberg [:julienw]]

Attachment: patch v4

Fixed the bug Steve has found during his review, and added 1 test for this.

The problem was that we didn't remove the hiding class when the header text didn't change. Now we remove it inconditionally (which should not add any perf problem because gecko checks for existence before trying to remove it, see [1]).

Added another test for another bug I introduced in the process ;) And I refactored a little the "list is empty" case.

[1] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMTokenList.cpp#226

PR is still at https://github.com/mozilla-b2g/gaia/pull/10963

Thanks Steve !

Comment 34

[Steve Chung [:steveck]]

Comment on attachment 782560 [details] [diff] [review]
patch v4

Thanks for the explanation and the latest patch looks great to me!

Comment 35

[Julien Wajsberg [:julienw]]

thanks !

master: 5032062fbc83356604a83ed7d6665caccfb36bf2

Comment 36

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 5032062fbc83356604a83ed7d6665caccfb36bf2 to:
v1-train: ddb922ed002e88d01f71199da32ff75911b455b2

Comment 37

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: ddb922ed002e88d01f71199da32ff75911b455b2

Comment 38

[isabelrios]

Checked with unagi device 08/07 v1-train build:
Gecko-70e8636
Gaia-60ca816
ref ril

Cannot to reproduce the problem.

Comment 39

[Julien Wajsberg [:julienw]]

Isabel> you need to put 'verified' instead of 'resolved'

('worksforme' is used when a reported bug is not reproducible by the tester)