Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#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 19 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 19 months ago by Ross

  • Description modified (diff)

Changed 19 months ago by Ross

comment:2 Changed 19 months ago by Ross

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

comment:3 Changed 19 months ago by Ross

  • Description modified (diff)

comment:4 Changed 19 months ago by Ross

  • Description modified (diff)

comment:5 Changed 19 months ago by kzar

  • Cc saroyanm greiner added

comment:6 Changed 19 months ago by saroyanm

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

comment:7 Changed 19 months ago by saroyanm

  • Cc agiammarchi wspee added

comment:8 Changed 19 months ago by saroyanm

  • Owner set to saroyanm

comment:9 Changed 19 months ago 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 19 months ago by saroyanm

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

comment:11 follow-up: Changed 19 months ago 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 19 months ago by sebastian

  • Blocking 6476 added

comment:13 in reply to: ↑ 11 Changed 19 months ago 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 19 months ago by kzar

  • Blocking 6476 removed

comment:15 Changed 19 months ago by abpbot

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

comment:16 Changed 19 months ago by saroyanm

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

comment:17 Changed 19 months ago by sebastian

  • Blocking 6511 added

comment:18 Changed 19 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed.

ABP 3.0.2.1998
Chrome 49 / 65 / Windows 7

Note: See TracTickets for help on using tickets.