[Notification] horizontally scrollable with many CB messages

RESOLVED FIXED in Firefox OS v1.1hd

Status

P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: leo.bugzilla.gaia, Assigned: alive)

Tracking

unspecified
1.1 QE5
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

Details

(Whiteboard: [TD-68830] [LeoVB+])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 780845 [details]
Repro video

When there are many CB messages on notification panel, 
the notification panel becomes horizontally scrollable.

The notification item was swiped away to get deleted, however 
the notification panel scrolled horizontally.

It was reported that this issue is not reproducible with SMS, but is reproducible with CB messages.

Comment 1

5 years ago
Hi Alive, 

Would you take a quick look of the video attachment and see if this is a know issue (hopefully with a patch)?

Thanks
Flags: needinfo?(alive)
(Reporter)

Updated

5 years ago
Whiteboard: [TD-68830]
(Reporter)

Updated

5 years ago
blocking-b2g: --- → leo+
Priority: -- → P1
Target Milestone: --- → 1.1 QE5
(Reporter)

Updated

5 years ago
Flags: needinfo?(dale)
It's a bug in notification. Not only CB.
Flags: needinfo?(alive)
Hi Yuren,
Can you see if you can help out here?
Flags: needinfo?(yurenju.mozilla)
Created attachment 781573 [details] [diff] [review]
https://github.com/mozilla-b2g/gaia/pull/11164

Don't transition before handler is set.
Assignee: nobody → alive
Attachment #781573 - Flags: review?(timdream)
Hmm, I mad the patch yesterday and clicked the submit button but I didn't check the status and was attracted by something else...
clear ni.
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(dale)
Attachment #781573 - Flags: review?(timdream) → review+
master

https://github.com/mozilla-b2g/gaia/commit/a3d7268219f6ff92d296a3305bdfef14209885dc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → affected
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Whiteboard: [TD-68830] → [TD-68830] [LeoVB+]

Comment 8

5 years ago
(In reply to Alive Kuo [:alive] from comment #7)
> master
> 
> https://github.com/mozilla-b2g/gaia/commit/
> a3d7268219f6ff92d296a3305bdfef14209885dc

Too bad.. this is still happening with the patch...
What's the action? Do you wait for a while that transition is ended?

Comment 10

5 years ago
I think I found the reason now. 

The problem is, in removeNotification function in notifications.js, 
it should use 'querySelectorAll' instead of 'querySelector' and loop each item.


    var notificationNodes = this.container.querySelectorAll(notifSelector);
    for (var i = notificationNodes.length - 1; i >= 0; i--) {
      console.log('nnnnn removeNotification checkpoint 1');
      notificationNodes[i].parentNode.removeChild(notificationNodes[i]);
    }

    var lockScreenNotificationNodes =
      this.lockScreenContainer.querySelectorAll(notifSelector);
    for (var i = lockScreenNotificationNodes.length - 1; i >= 0; i--) {
      console.log('nnnnn removeNotification checkpoint 2');
      lockScreenNotificationNodes[i].parentNode
        .removeChild(lockScreenNotificationNodes[i]);
    }

p.s. The notificationID for CB messages that were added by cell_broadcast_system.js is undefined. and there can be multiple of those notifications on notification panel at a time.

Comment 11

5 years ago
(In reply to hanj.kim25 from comment #10)

Hm.. it's supposed to remove only the one that was touched/swiped. Please ignore the 'querySelectorAll' solution above.

Comment 12

5 years ago
The problem was the notification items that were added by cell_broadcast_system.js have notificationID being undefined.

Introducing a new function, removeNotificationByNode, fixes the problem.
Let me create a pull request with this.
Thanks


  closeNotification: function ns_closeNotification(notificationNode) {
    var notificationID = notificationNode.dataset.notificationID;

    var event = document.createEvent('CustomEvent');
    event.initCustomEvent('mozContentEvent', true, true, {
      type: 'desktop-notification-close',
      id: notificationID
    });
    window.dispatchEvent(event);

    if (notificationNode.dataset.notificationID === 'undefined')
      this.removeNotificationByNode(notificationNode);
    else
      this.removeNotification(notificationNode.dataset.notificationID);
  },

  removeNotificationByNode: function ns_removeNotificationByNode(notificationNode) {
    if (notificationNode) {
      notificationNode.parentNode.removeChild(notificationNode);
    }
  },
(Reporter)

Comment 13

5 years ago
Created attachment 784159 [details]
Pull request url
Attachment #784159 - Flags: review?(alive)
Comment on attachment 784159 [details]
Pull request url

Thanks for finding out the root cause.
But I think we couldn't let notificationID to be undefined.
So we shall give an ID in cell broadcast module instead modifying the notification module.
See voicemail for reference.
Attachment #784159 - Flags: review?(alive) → review-
Uplifted a3d7268219f6ff92d296a3305bdfef14209885dc to:
v1-train: 288276c9762502211ab5432bf8db6dd0cb9faa6c
status-b2g18: affected → fixed
Blocks: 901801
File a new bug to track #12
v1.1.0hd: 288276c9762502211ab5432bf8db6dd0cb9faa6c
status-b2g-v1.1hd: --- → fixed
You need to log in before you can comment on or make changes to this bug.