Closed Bug 1073807 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.0M verified, b2g-v2.1 unaffected, b2g-v2.2 unaffected)

VERIFIED FIXED
2.1 S7 (24Oct)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- unaffected
b2g-v2.2 --- unaffected

People

(Reporter: sasikala.paruchuri8, Assigned: etienne)

References

Details

(Whiteboard: [LibGLA,TD100601,QE2, A][systemsfe])

Attachments

(5 files, 1 obsolete file)

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
Whiteboard: [LibGLA,TD100601,QE2, A]
Dear Vance Chen,

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

Thanks
Flags: needinfo?(vchen)
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.
Flags: needinfo?(sasikala.paruchuri8)
[Blocking Requested - why for this release]:
partner requested as blocker. nominate to 2.0 ?
blocking-b2g: --- → 2.0?
Attached video 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%
Flags: needinfo?(sasikala.paruchuri8)
Hi Luke, Could you please kindly look into the issue ?
Thank you very much!
Flags: needinfo?(vchen) → needinfo?(lchang)
Hi Rachelle,

Sorry for the late reply. I'll take a look soon.
Flags: needinfo?(lchang)
Thanks,Luke. Since this is requested by partner as blocker. 
Thanks for your time and efforts.
Triage group: Blocking as partner blocker.
blocking-b2g: 2.0? → 2.0+
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.
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.
Flags: needinfo?(ryang)
We may also need graphic team's help on profiling.
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!
Flags: needinfo?(ryang) → needinfo?(fdjabri)
I feel that this is the same issue as the one tracked by bug 978588, where this has been pushed to backlog.
Adapting the patch from bug 978588 makes the situation much better here.
Assignee: nobody → lissyx+mozillians
Attached file Gaia PR (obsolete) —
Tests are green locally with this patch.
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.
Attachment #8502480 - Flags: review?(etienne)
Sasikala, would you mind giving a try to the proposed PR and see if it also improves the situation on your side?
Flags: needinfo?(sasikala.paruchuri8)
See Also: → 978588
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?
Flags: needinfo?(sasikala.paruchuri8)
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
(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.
Flags: needinfo?(sasikala.paruchuri8)
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.
Flags: needinfo?(sasikala.paruchuri8)
We need to set a limit, it's already a workaround for some bug that has not been considered blocking in the past.
Flags: needinfo?(ryang)
Flags: needinfo?(hochang)
Flags: needinfo?(cserran)
Flags: needinfo?(anygregor)
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
Attachment #8502480 - Flags: review?(etienne)
Flags: needinfo?(cserran)
Whiteboard: [LibGLA,TD100601,QE2, A] → [LibGLA,TD100601,QE2, A][systemsfe]
Target Milestone: --- → 2.1 S6 (10oct)
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.
Flags: needinfo?(fdjabri) → needinfo?(rmacdonald)
(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.
Flags: needinfo?(anygregor)
(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.
Flags: needinfo?(rmacdonald)
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (Oct24)
Too much busy for now.
Assignee: lissyx+mozillians → nobody
Flags: needinfo?(hochang)
Hi Alexandre, thanks for your work and comments.
Agree with you to set a limit. Thanks!
Flags: needinfo?(ryang)
Assigning to Etienne who has a patch for this
Assignee: nobody → etienne
Attached file 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?
Attachment #8502480 - Attachment is obsolete: true
Attachment #8504882 - Flags: feedback?(sasikala.paruchuri8)
Attachment #8504882 - Flags: feedback?(chrislord.net)
Comment on attachment 8504882 [details] [review]
Gaia PR

Figured out my perf issue from yesterday and added tests so moving on to review :)
Attachment #8504882 - Flags: feedback?(chrislord.net) → review?(chrislord.net)
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!
Attachment #8504882 - Flags: feedback?(sasikala.paruchuri8)
Attached image Issue_overlap.png
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.
Attachment #8504882 - Flags: review?(chrislord.net)
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.
(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.
(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 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.
Attachment #8504882 - Flags: review?(chrislord.net)
Attached image no-overlap
Could not reproduce the issue from Comment 34. Can you check again with the latest version of the patch?
Flags: needinfo?(sasikala.paruchuri8)
Comment on attachment 8504882 [details] [review]
Gaia PR

Looks good! I've added one nit, but it's nothing that should stop landing.
Attachment #8504882 - Flags: review?(chrislord.net) → review+
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?
Flags: needinfo?(sasikala.paruchuri8)
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]: ø
Attachment #8504882 - Flags: approval-gaia-v2.0?
Flags: needinfo?(bbajaj)
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!
Flags: needinfo?(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 :)
Flags: needinfo?(etienne)
Attachment #8504882 - Flags: review+ → review?(chrislord.net)
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 :)
Attachment #8504882 - Flags: review?(chrislord.net)
Comment on attachment 8504882 [details] [review]
Gaia PR

Good catch, patch updated! (and re-tested on device)
Attachment #8504882 - Flags: review?(chrislord.net)
Comment on attachment 8504882 [details] [review]
Gaia PR

LGTM, pending tests passing.
Attachment #8504882 - Flags: review?(chrislord.net) → review+
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.
Flags: needinfo?(bbajaj)
(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?
Flags: needinfo?(bbajaj)
(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.
Flags: needinfo?(bbajaj)
(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?
Keywords: verifyme
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.
Attachment #8504882 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Etienne,

Could we also do a try run before landing on the branch?
Flags: needinfo?(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
Flags: needinfo?(etienne)
Not sure if this is on someones radar for landing. Etienne, can you merge?
Flags: needinfo?(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.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attached video video
Thanks Shine!
Mark as "VERIFIED".
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: Woodduck
You need to log in before you can comment on or make changes to this bug.