897874 - [Notification] horizontally scrollable with many CB messages

Status: RESOLVED FIXED

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

Description

[Leo]

Attachment: 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

[hanj.kim25]

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

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

It's a bug in notification. Not only CB.

Comment 3

[Wayne Chang [:wchang]]

Hi Yuren,
Can you see if you can help out here?

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/11164

Don't transition before handler is set.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Hmm, I mad the patch yesterday and clicked the submit button but I didn't check the status and was attracted by something else...

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

clear ni.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

master

https://github.com/mozilla-b2g/gaia/commit/a3d7268219f6ff92d296a3305bdfef14209885dc

Comment 8

[hanj.kim25]

(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...

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

What's the action? Do you wait for a while that transition is ended?

Comment 10

[hanj.kim25]

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

[hanj.kim25]

(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

[hanj.kim25]

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);
    }
  },

Comment 13

[Leo]

Attachment: Pull request url

Comment 14

[(inactive after 6/18) Alive Kuo [: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.

Comment 15

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

Uplifted a3d7268219f6ff92d296a3305bdfef14209885dc to:
v1-train: 288276c9762502211ab5432bf8db6dd0cb9faa6c

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

File a new bug to track #12

Comment 17

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

v1.1.0hd: 288276c9762502211ab5432bf8db6dd0cb9faa6c