1073807 - [System] User cannot pull down the notification bar when there are several cancelled pairing requests

Status: VERIFIED FIXED

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

Description

[sasikala]

1. Title: User cannot pull down the notification bar when there are several cancelled pairing requests
2. Precondition: Should have 160 to 200 images in Gallery app.
                 Should have bluetooth paired device
3. Tester's Action: 1.Select all the images(160) from Gallery
                    2.Share via Bluetooth Transfer
                    3.When the other device gets a request with Accept/Deny 
                    4.Deny the request -> Now we will get notifications for all the requests 
4. Actual Symptom (ENG.) : User cannot pull down the notification bar
5. Expected: Should be able to view and pull down the notification bar

Comment 1

[Sharaf]

Dear Vance Chen,

Can you please help in assigning this to the right person?

Thanks

Comment 2

[Rachelle Yang [:ryang][[e-mail]]]

Hi Sasikala,

please provide the detailed build information (below)and video STR otherwise we won't proceed the issue any further.
including 
gaia revision
gecko revision
build-ID 
gecko version
firmware version
occurrence rate 

thank you very much.

Comment 3

[Rachelle Yang [:ryang][[e-mail]]]

[Blocking Requested - why for this release]:
partner requested as blocker. nominate to 2.0 ?

Comment 4

[sasikala]

Attachment: Issue_Video.3gp

Hi, Please find the attached video(Issue_Video.3gp)for the issue.

STR:
1.Should have 160 images in Gallery and have Bluetooth paired device
2.Select all images from gallery -> share via Bluetooth
3.Deny the request from other device -> Now we will get notifications for all the cancelled requests
4.Pull down the notification bar

Build Information:
1.Git commit info - ddb7567c
2.Platform version - 32.0
3.Build Identifier - 20140903133524
4.occurrence rate - 75%

Comment 5

[Rachelle Yang [:ryang][[e-mail]]]

Hi Luke, Could you please kindly look into the issue ?
Thank you very much!

Comment 6

[Luke Chang [:lchang] (inactive)]

Hi Rachelle,

Sorry for the late reply. I'll take a look soon.

Comment 7

[Rachelle Yang [:ryang][[e-mail]]]

Thanks,Luke. Since this is requested by partner as blocker. 
Thanks for your time and efforts.

Comment 8

[howie [:howie]]

Triage group: Blocking as partner blocker.

Comment 9

[Luke Chang [:lchang] (inactive)]

This issue seems to be caused by too many notifications triggered at the same time. I notice that the graphic performance will degrade if there are more than 200 notifications in the utility tray.

After discussing with Ian, one notification per one file is by design so we can't just reduce these error messages in this case. I'll look into performance issue next.

Comment 10

[Luke Chang [:lchang] (inactive)]

Hi Rachelle,

After looking into system app, I think it's a generic performance issue of the utility tray that happens whenever there are too many notifications in it.

For the long term, to implement infinite scrolling mechanism in the utility tray may be an option but it will be a new feature.

For this bug only, I suggest to consult UX if we can drop some notifications in this case.

Comment 11

[Luke Chang [:lchang] (inactive)]

We may also need graphic team's help on profiling.

Comment 12

[Rachelle Yang [:ryang][[e-mail]]]

Hi Francis, per comment 9 and comment10, 

May you please kindly look into the issue, and share your ideas if we could do something to
the utility tray that happens whenever there are too many notifications in it ?

Thank you very much!

Comment 13

[:gerard-majax]

I feel that this is the same issue as the one tracked by bug 978588, where this has been pushed to backlog.

Comment 14

[:gerard-majax]

Adapting the patch from bug 978588 makes the situation much better here.

Comment 15

[:gerard-majax]

Attachment: Gaia PR

Comment 16

[:gerard-majax]

Tests are green locally with this patch.

Comment 17

[:gerard-majax]

Comment on attachment 8502480 [details] [review]
Gaia PR

Etienne, I know you are as frightened as I am on this, but as far as I could test this was okay. Locally all unit tests are fine.

Comment 18

[:gerard-majax]

Sasikala, would you mind giving a try to the proposed PR and see if it also improves the situation on your side?

Comment 19

[sasikala]

Hi gerard-majax,

Thank you for the patch. I have done testing with this patch having 350 images in gallery and sharing via Bluetooth but this is not working fine.

Thanks!

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> Sasikala, would you mind giving a try to the proposed PR and see if it also
> improves the situation on your side?

Comment 20

[sasikala]

The following scenarios are not working
1. Not able to pull down the notification channel every time
2. Once notification channel is pulled down -> not able to move the notification up again

Comment 21

[:gerard-majax]

(In reply to sasikala from comment #19)
> Hi gerard-majax,
> 
> Thank you for the patch. I have done testing with this patch having 350
> images in gallery and sharing via Bluetooth but this is not working fine.
> 
> Thanks!
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #18)
> > Sasikala, would you mind giving a try to the proposed PR and see if it also
> > improves the situation on your side?

Sorry but in your first reports, you were testing with 160-200 images. Now you are evaluating 350. This is not consistent.

Can you please confirm whether the patch improves the situation or not on your first case ? As I documented, if it's improved by this, then it's likely to be bug 978588, which has been considered not a blocker for some time.

Thanks.

Comment 22

[sasikala]

Hi gerard-majax,

Thank you for your response.This patch is working fine for 160-200 images. 
(The below case for 350 images is tested to verify whether same patch works well for more images and given the information).

Thanks!

(In reply to Alexandre LISSY :gerard-majax from comment #21)
> (In reply to sasikala from comment #19)
> > Hi gerard-majax,
> > 
> > Thank you for the patch. I have done testing with this patch having 350
> > images in gallery and sharing via Bluetooth but this is not working fine.
> > 
> > Thanks!
> > 
> > (In reply to Alexandre LISSY :gerard-majax from comment #18)
> > > Sasikala, would you mind giving a try to the proposed PR and see if it also
> > > improves the situation on your side?
> 
> Sorry but in your first reports, you were testing with 160-200 images. Now
> you are evaluating 350. This is not consistent.
> 
> Can you please confirm whether the patch improves the situation or not on
> your first case ? As I documented, if it's improved by this, then it's
> likely to be bug 978588, which has been considered not a blocker for some
> time.
> 
> Thanks.

Comment 23

[:gerard-majax]

We need to set a limit, it's already a workaround for some bug that has not been considered blocking in the past.

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 8502480 [details] [review]
Gaia PR

Given how late this is we should keep the patch focused. ie either just the moz-element hack or just the costcontrol change. I suspect the moz-element is the important part given the profile. So let's do just that (and remove the costcontrol widget/utilitytrayshown part).

Some notes:
* no need to toggle the class on "#screen", it's expensive and "static" isn't really an explicit name :/ let's just toggle classes on the 2 containers directly (.on-top / .below maybe).
* I'd like to rename `app-notifications-container` to `desktop-notifications-placeholder` to make it clear that we're not introducing a new concept but just moving something outside of the utility-tray element

Comment 25

[Francis Djabri [:djabber]]

Putting a limit on the number of notifications seems very reasonable to me. Passing this over to Rob for the final word, as the UX owner for Notifications.

Comment 26

[Gregor Wagner [:gwagner]]

(In reply to Alexandre LISSY :gerard-majax from comment #23)
> We need to set a limit, it's already a workaround for some bug that has not
> been considered blocking in the past.

Yes one issue at a time in a bug! Lets focus on the issue described in the description and file followups if needed.

Comment 27

[Rob MacDonald [:robmac]]

(In reply to Francis Djabri [:djabber] from comment #25)
> Putting a limit on the number of notifications seems very reasonable to me.
> Passing this over to Rob for the final word, as the UX owner for
> Notifications.

I agree. If we're talking about a limit of 200 notifications, it's unlikely someone would get that far down the list in any case.

Comment 28

[:gerard-majax]

Too much busy for now.

Comment 29

[Rachelle Yang [:ryang][[e-mail]]]

Hi Alexandre, thanks for your work and comments.
Agree with you to set a limit. Thanks!

Comment 30

[Candice Serran (:cserran)]

Assigning to Etienne who has a patch for this

Comment 31

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

I'm testing this on a flame-kk 2.0.
I have taken 200 screen captures, so I have at least 200 pictures in my gallery and 200 notifications.

It works a _lot_ better, but there's still one strange repaint of the notifications on touchend, making us loose part of the transition but not preventing us to open the utility tray.

This is a cleaned up version for Alex's patch which was Vivien's patch, so:
- Chris, flagging you early since you'll probably be the reviewer for this :)
- Sasikala, are we good for 200 notifications? or should I investigate this repaint more?

Comment 32

[Etienne Segonzac (:etienne)]

Comment on attachment 8504882 [details] [review]
Gaia PR

Figured out my perf issue from yesterday and added tests so moving on to review :)

Comment 33

[sasikala]

Comment on attachment 8504882 [details] [review]
Gaia PR

Hi etienne, 

Thank you for the patch. We have tested the patch and is working fine but there is an overlap with No SIM inserted Pop-up.Uploading a screenshot for the same. Please verify the scenario once.

Thanks!

Comment 34

[sasikala]

Attachment: Issue_overlap.png

Comment 35

[Chris Lord [:cwiiis]]

Comment on attachment 8504882 [details] [review]
Gaia PR

I have too many nits and questions to r+ this, but this isn't an r-. I think some more experimentation might yield a simpler fix for this, but either way the patch could do with another round or two.

Comment 36

[Etienne Segonzac (:etienne)]

While waiting for an updated patch, I should explain the approach a bit more.

Without bug 931668, setting the style.transform on the #utility-tray div becomes really expensive when it has a lot of descendents. So the more notifications we have the slower it gets.

The goal of the moz-element hack is to store the notifications outside of #utility-tray to keep the number of #utility-tray descendents low.

Comment 37

[Chris Lord [:cwiiis]]

(In reply to Etienne Segonzac (:etienne) from comment #36)
> While waiting for an updated patch, I should explain the approach a bit more.
> 
> Without bug 931668, setting the style.transform on the #utility-tray div
> becomes really expensive when it has a lot of descendents. So the more
> notifications we have the slower it gets.
> 
> The goal of the moz-element hack is to store the notifications outside of
> #utility-tray to keep the number of #utility-tray descendents low.

mm, I get what it's working around, but I think that expense may be negated if you hide the overflow - it's worth a shot.

Comment 38

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #37)
> (In reply to Etienne Segonzac (:etienne) from comment #36)
> > While waiting for an updated patch, I should explain the approach a bit more.
> > 
> > Without bug 931668, setting the style.transform on the #utility-tray div
> > becomes really expensive when it has a lot of descendents. So the more
> > notifications we have the slower it gets.
> > 
> > The goal of the moz-element hack is to store the notifications outside of
> > #utility-tray to keep the number of #utility-tray descendents low.
> 
> mm, I get what it's working around, but I think that expense may be negated
> if you hide the overflow - it's worth a shot.

if the overflow doesn't negate the expense, it's also worth trying a clip-rect to clip it to just the visible region.

Comment 39

[Etienne Segonzac (:etienne)]

Comment on attachment 8504882 [details] [review]
Gaia PR

I really wished the overscroll/pointer-events/clip would work but it doesn't :/
(even tried all of them together).

PR updated nonetheless.

Comment 40

[Etienne Segonzac (:etienne)]

Attachment: no-overlap

Could not reproduce the issue from Comment 34. Can you check again with the latest version of the patch?

Comment 41

[Chris Lord [:cwiiis]]

Comment on attachment 8504882 [details] [review]
Gaia PR

Looks good! I've added one nit, but it's nothing that should stop landing.

Comment 42

[sasikala]

Hi Etienne,
Tested the scenario once again with latest version of the patch and is working fine.
Thank you.

(In reply to Etienne Segonzac (:etienne) from comment #40)
> Created attachment 8505574 [details]
> no-overlap
> 
> Could not reproduce the issue from Comment 34. Can you check again with the
> latest version of the patch?

Comment 43

[Etienne Segonzac (:etienne)]

Comment on attachment 8504882 [details] [review]
Gaia PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): always had it

[User impact] if declined: a user with more than 50 notifications (read or unread) will start experiencing lag when opening the utility tray. it gets worse an worse as the number of notifications grows until it barely works.

[Testing completed]: opening/closing the tray on various apps (fullscreen or not), with multiple types of notifications. the utility tray is also covered by various marionette tests.

[Risk to taking this patch] (and alternatives if risky): moderate. the change is well tested but it's definitely not "just a tweak".

[String changes made]: ø

Comment 44

[sasikala]

Hi Etienne,

While doing testing -> found one scenario failing. Please find the STR for the issue
1. Should have some notifications in notifications bar
2. While in call -> slide down the notification bar
3. The notification bar is shown with notifications when pulled down and then the notifications aren't shown once it is down. But if we move notification bar a little bit up, the notifications are shown again.
Will you please check this scenario.

Thanks!

Comment 45

[Etienne Segonzac (:etienne)]

Comment on attachment 8504882 [details] [review]
Gaia PR

Thanks! Patch updated.
I knew we shouldn't do z-index outside of zindex.css, fixed now :)

Comment 46

[Chris Lord [:cwiiis]]

Comment on attachment 8504882 [details] [review]
Gaia PR

Not an r+ because of what appears to be a mistake in the z-index css. Re-flag me when it's fixed and the commented problem is verified :)

Comment 47

[Etienne Segonzac (:etienne)]

Comment on attachment 8504882 [details] [review]
Gaia PR

Good catch, patch updated! (and re-tested on device)

Comment 48

[Chris Lord [:cwiiis]]

Comment on attachment 8504882 [details] [review]
Gaia PR

LGTM, pending tests passing.

Comment 49

[bhavana bajaj [:bajaj]]

needs to land on master here before we consider uplifts to branches, holding off on approval until then.
Does the 2.0 or master patch apply to 2.1 ? If its the 2.0, please set teh 2.1 approval request on the same so this can land there.

Comment 50

[Etienne Segonzac (:etienne)]

(In reply to bhavana bajaj [:bajaj] from comment #49)
> needs to land on master here before we consider uplifts to branches, holding
> off on approval until then.
> Does the 2.0 or master patch apply to 2.1 ? If its the 2.0, please set teh
> 2.1 approval request on the same so this can land there.

This is a 2.0 only patch.
The UtilityTray was refactored in 2.1. and 2.2.

How should we proceed?

Comment 51

[bhavana bajaj [:bajaj]]

(In reply to Etienne Segonzac (:etienne) from comment #50)
> (In reply to bhavana bajaj [:bajaj] from comment #49)
> > needs to land on master here before we consider uplifts to branches, holding
> > off on approval until then.
> > Does the 2.0 or master patch apply to 2.1 ? If its the 2.0, please set teh
> > 2.1 approval request on the same so this can land there.
> 
> This is a 2.0 only patch.
> The UtilityTray was refactored in 2.1. and 2.2.
> 
> How should we proceed?

I see, thanks ! I'll mark those branches unaffected then and we can continue the branch landing here. Will request explicit QA verification in this case as well.

Comment 52

[bhavana bajaj [:bajaj]]

(In reply to sasikala from comment #0)
> 1. Title: User cannot pull down the notification bar when there are several
> cancelled pairing requests
> 2. Precondition: Should have 160 to 200 images in Gallery app.
>                  Should have bluetooth paired device
> 3. Tester's Action: 1.Select all the images(160) from Gallery
>                     2.Share via Bluetooth Transfer
>                     3.When the other device gets a request with Accept/Deny 
>                     4.Deny the request -> Now we will get notifications for
> all the requests 
> 4. Actual Symptom (ENG.) : User cannot pull down the notification bar
> 5. Expected: Should be able to view and pull down the notification bar

Can you please help verify the issue is fixed for you in tomorrow's build?

Comment 53

[bhavana bajaj [:bajaj]]

Comment on attachment 8504882 [details] [review]
Gaia PR

Along with reporter verification, please help get QA verification once it lands on 2.0. Note other branches are unaffected.

Comment 54

[bhavana bajaj [:bajaj]]

Etienne,

Could we also do a try run before landing on the branch?

Comment 55

[Etienne Segonzac (:etienne)]

(In reply to bhavana bajaj [:bajaj] from comment #54)
> Etienne,
> 
> Could we also do a try run before landing on the branch?

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=4d664ff62463

Comment 56

[Gregor Wagner [:gwagner]]

Not sure if this is on someones radar for landing. Etienne, can you merge?

Comment 57

[Etienne Segonzac (:etienne)]

Rebased (on 2.0), here's the new try https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=3f696838b0b1 will land as soon as I get some green.

Comment 58

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/9f5b6f025e528fabfcc068782cb9b492cb51a7f9

Comment 59

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/9f5b6f025e528fabfcc068782cb9b492cb51a7f9

Comment 60

[Shine(Leave from Mozilla)]

This issue can't repro on woodduck 2.0 and Flame 2.0.
See attachment: Verify_video.MP4
Reproducing rate: 0/5

Woodduck 2.0 buid:
Gaia-Rev        ee5cf148b4c546beea9bfb799d2a3ee62074957d
Gecko-Rev       73601b71861cbc2f180c4d2653cec3e9fbb39db5
Build-ID        20141114050313
Version         32.0
Flame 2.0 versions:
Gaia-Rev        ab83632c92f9fc571b11d8468b6901cc4ed905c0
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e21bf45e6c44
Build-ID        20141113000201
Version         32.0
This issue can't repro on woodduck 2.0

Comment 61

[Shine(Leave from Mozilla)]

Attachment: video

Comment 62

[William Hsu [:whsu]]

Thanks Shine!
Mark as "VERIFIED".