Opened on 03/22/2018 at 08:42:02 AM

Closed on 03/23/2018 at 12:03:52 PM

Last modified on 03/28/2018 at 07:43:03 AM

#6510 closed defect (fixed)

Configure notification opens Options page incorrectly

Reported by: Ross Assignee: saroyanm
Priority: P1 Milestone:
Module: User-Interface Keywords:
Cc: sebastian, kzar, saroyanm, greiner, agiammarchi, wspee Blocked By:
Blocking: #6511 Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29730617/

Description (last modified by saroyanm)

Environment

ABP 3.0.2.1983
Chrome 49 / 65 / Windows 7

Not reproduced in Opera as it does not currently display buttons on notifications.

How to reproduce

  1. Save the notification below into a directory.
  2. Serve it `python -m SimpleHTTPServer
  3. Install ABP.
  4. [ABP Icon] > [Right Click Popup] > [Inspect].
  5. In the popup console, set the notification url: browser.runtime.sendMessage({type: "prefs.set", key: "notificationurl", value: "<serveurl>"})
  6. Wait for ABP to request notifications.
  7. Select [Configure notification settings] in the popup.

Test data

{
  "notifications": [
    {
      "id": "1",
      "message": {
        "en-US": "This is a normal notification."
      },
      "severity": "normal",
      "title": {
        "en-US": "Test Normal Notification"
      }
    }
  ],
  "version": "201707281538"
}

Observed behaviour

The new options page is opened, but the content area is collapsed and contains no content. See screenshot.

Expected behaviour

In the 3.0.2 release, selecting [Configure notification settings] opens the options page to the advanced tab.
Preferably with the Show useful notifications option highlighted.

Attachments (1)

6510-OptionsFromNotification.jpg (66.5 KB) - added by Ross on 03/22/2018 at 08:45:00 AM.

Download all attachments as: .zip

Change History (19)

comment:1 Changed on 03/22/2018 at 08:43:46 AM by Ross

  • Description modified (diff)

Changed on 03/22/2018 at 08:45:00 AM by Ross

comment:2 Changed on 03/22/2018 at 08:45:23 AM by Ross

This is a regression if compared to using the old options page in 3.0.2.

comment:3 Changed on 03/22/2018 at 09:05:12 AM by Ross

  • Description modified (diff)

comment:4 Changed on 03/22/2018 at 09:06:13 AM by Ross

  • Description modified (diff)

comment:5 Changed on 03/22/2018 at 09:50:33 AM by kzar

  • Cc saroyanm greiner added

comment:6 Changed on 03/22/2018 at 11:53:46 AM by saroyanm

  • Description modified (diff)
  • Priority changed from Unknown to P1
  • Ready set

comment:7 Changed on 03/22/2018 at 11:56:13 AM by saroyanm

  • Cc agiammarchi wspee added

comment:8 Changed on 03/22/2018 at 11:56:20 AM by saroyanm

  • Owner set to saroyanm

comment:9 Changed on 03/22/2018 at 01:18:49 PM by saroyanm

The issue is that we do set the tab view to the notification (the argument that's sent from here).

The way it should behave, we should focus the Advanced tab instead of the "notification" (tab that doesn't exist), make sure the Show useful notifications option is visible and highlight it.

comment:10 Changed on 03/22/2018 at 04:16:35 PM by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:11 follow-up: Changed on 03/22/2018 at 05:37:01 PM by saroyanm

With the latest ABPUI dependency update the option page doesn't receive current response message at all, might be a race condition, still investigating. Might be related to reference implementation, but even with that patch applied, the issue exist. [still investigating]

comment:12 Changed on 03/22/2018 at 05:51:19 PM by sebastian

  • Blocking 6476 added

comment:13 in reply to: ↑ 11 Changed on 03/22/2018 at 09:21:23 PM by saroyanm

Replying to saroyanm:

With the latest ABPUI dependency update the option page doesn't receive current response message at all, might be a race condition, still investigating. Might be related to reference implementation, but even with that patch applied, the issue exist. [still investigating]

This was because we were using page.sendMessage instead of port.postMessage after switching to long-lived connections.

comment:14 Changed on 03/23/2018 at 11:01:11 AM by kzar

  • Blocking 6476 removed

comment:15 Changed on 03/23/2018 at 11:40:45 AM by abpbot

A commit referencing this issue has landed:
Issue 6510 - Configure notification opens options page incorrectly

comment:16 Changed on 03/23/2018 at 12:03:52 PM by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:17 Changed on 03/23/2018 at 03:45:51 PM by sebastian

  • Blocking 6511 added

comment:18 Changed on 03/28/2018 at 07:43:03 AM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed.

ABP 3.0.2.1998
Chrome 49 / 65 / Windows 7

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from saroyanm.
 
Note: See TracTickets for help on using tickets.