Opened on 08/29/2017 at 04:05:33 PM

Closed on 10/10/2017 at 05:44:38 PM

Last modified on 03/29/2018 at 07:58:31 AM

#5593 closed defect (fixed)

[webextension] options page doesn't open in a Firefox private browsing window

Reported by: hfiguiere Assignee: mjethani
Priority: P2 Milestone: Adblock-Plus-3.0-for-Firefox
Module: Platform Keywords:
Cc: kzar, sebastian, mjethani, trev, greiner, jeen Blocked By: #5835, #5847
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

​https://codereview.adblockplus.org/29532763/
​https://codereview.adblockplus.org/29532767/
​https://codereview.adblockplus.org/29533613/
​https://codereview.adblockplus.org/29567749/
​https://codereview.adblockplus.org/29567782/
​https://codereview.adblockplus.org/29567749/
​https://codereview.adblockplus.org/29570774/

Description (last modified by mjethani)

Environment

Firefox Nightly (57.0a from today)
Adblock Plus Web Extension from master.

How to reproduce

  1. Install Adblock Plus Web Extension from master. Older version might be able to reproduce too.
  2. Open a private browser window.
  3. Click on the ABP icon and select "Options"

Observed behaviour

Nothing happens.

Expected behaviour

The Options page should open.

Notes

  • works fine from a non private windows
  • on Chrome from an icognito window, the options page is opened in a non-incognito window.

What to change

  1. Instead of using the background page directly from the popup use messaging to communicate with it.
  2. Update the adblockpluscore dependency to hg:b21bddce2678 git:2b57122. No other changes will be included.
  3. Update the adblockplusui dependency to hg:87265b2d6f0b git:6defd73. No other unrelated changes will be included.

Hints for testers

In short, everything in the popup should be tested, as pretty much every single part of the implementation has changed in a significant way, yet we expect no changes in behavior (except as noted below).

When the user interacts with a UI element, there may be a slightly noticeable delay between the interaction and until the respective change takes effect. For example, when the user toggles the "Show number in icon" preference, it may take up to a second or even slightly longer to update the number widget in the toolbar icon. This is expected. If the delay is too distracting and unacceptable from a user experience point of view, it should be filed as a separate issue.

To test the notification logic in the popup do the following:

  1. Paste this into the background console: 
require("notification").Notification.addNotification({
  id: new Date() | 0,
  type: "critical",
  title: "Notification title",
  message: "<a>Open contribution page</a>",
  links: ["contribute"],
  urlFilters: ["||example.com^$document"]
});
  1. Open a new tab, browse to ​http://example.com and make sure the notification pops up.
  1. Click the "ABP" icon make sure the notification is listed in the popup window too, click the notification in the popup and make sure the contribution page opens successfully.

To test the stats logic in the popup:

  1. Enable EasyList, disable AA.
  2. Browse to ​https://reddit.com
  3. Click the ABP logo, in the popup window make sure the stats section is expanded.
  4. Ensure that the blocked counts are correct when the popup first opens, but that they are not updated in real-time as more adverts are blocked. (In other words if an advert is blocked whilst the popup is open the numbers won't update, but after the popup is re-opened the numbers should be correct.)

To test that the popup waits for the page to finish loading before it shows any UI:

  1. Create a page containing the following code: 
<body>
  <h1 id="result"></h1>
  <script>
    let r = 0;
    for (let i = 0; i < 100000; i++)
      r = Math.random();
    document.getElementById("result").innerHTML = r;
  </script>
</body>
  1. Load the page over http:// or https:// and immediately open the popup before the page has finished loading
  1. Ensure that the popup shows no relevant UI elements until and only until the page has finished loading

Also test that the popup shows none of the usual options when a page is loaded off disk using file://

Test that when the user enables or disables ad blocking on a page the preference takes effect immediately and stays in effect if the tab is closed and the page is opened again in a different tab.

Attachments (0)

Change History (51)

comment:1 Changed on 08/30/2017 at 01:37:47 PM by hfiguiere

  • Description modified (diff)

comment:2 Changed on 08/30/2017 at 01:48:11 PM by hfiguiere

Upon further investigation, it seems that Firefox doesn't support chrome.extension.getBackgrounPage() in private browsing

​https://bugzilla.mozilla.org/show_bug.cgi?id=1329304

This is used in ext/popup.js

Last edited on 08/30/2017 at 01:52:28 PM by hfiguiere

comment:3 Changed on 08/30/2017 at 03:35:16 PM by hfiguiere

  • Cc kzar sebastian mjethani trev added
  • Summary changed from options page doesn't open in a Firefox private browsing window to [webextension] options page doesn't open in a Firefox private browsing window

comment:4 Changed on 08/30/2017 at 07:11:46 PM by sebastian

IMO, the proper solution would be to remove any usage of ext.* (from the browser action popup), and use the chrome.* API directly. Eventually, we want to get rid of ext anyway, as there is no point of such an abstraction layer anymore, since we no longer support Safari with this code, and all other platforms target the same API.

comment:5 Changed on 08/31/2017 at 09:21:46 AM by trev

In fact, we just shouldn't use getBackgroundPage(). In this particular case, it seems to be unnecessary - we can load ext/background.js in the pop-up instead of extending the one from the background page. We have another (indirect) use of this API in stats.js however, it should really be using proper messaging.

comment:6 follow-up: Changed on 08/31/2017 at 01:23:32 PM by mjethani

popup.js is also going to need stuff from lib/adblockplus.js. Should we load that too? I don't know if it's going to be very useful, since it seems to rely on data structures maintained by ext/background.js, but those will be empty in this instance.

Probably a better idea is to hide the menu entirely and just show the options button.

comment:7 in reply to: ↑ 6 Changed on 08/31/2017 at 01:34:42 PM by trev

Replying to mjethani:

popup.js is also going to need stuff from lib/adblockplus.js. Should we load that too?

No, it should be using messaging and asking the background page for whatever info it needs.

Last edited on 08/31/2017 at 01:34:59 PM by trev

comment:8 Changed on 08/31/2017 at 03:50:25 PM by mjethani

  • Cc greiner added
  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:9 Changed on 08/31/2017 at 03:50:44 PM by mjethani

  • Status changed from new to reviewing

comment:10 follow-up: Changed on 08/31/2017 at 04:03:39 PM by mjethani

Unless we want to ignore the user's preferences entirely in the private window, we'll need the Prefs object. It's fairly lightweight and we can load it as a module in the popup by itself.

But before we do that we need to make a few changes to the popup code. I've submitted ​29532763 for this.

In addition to popup.js, we need to worry about stats.js and notification.js as well. We could probably handle most of it by messaging between the popup and the background page, but sending messages to the background page just to get or set a preference value is probably a bit of an overkill.

comment:11 Changed on 08/31/2017 at 04:28:40 PM by sebastian

One the reason why we use extension.getBackgroundPage() to access functionality of ext/background.js and lib/stats.js rather than loading these scripts directly into the popup, is so that global state (e.g. the number of blocked ads) is shared. Another problem is that when we'd include ext/background.js in the popup, some initialization (like registering event handlers) is redundantly performed when the popup is loaded. However, instead it might be possible to include ext/content.js and use messaging, just like the options page does.

comment:12 Changed on 08/31/2017 at 05:12:45 PM by mjethani

  • Review URL(s) modified (diff)

comment:13 in reply to: ↑ 10 Changed on 08/31/2017 at 05:14:46 PM by mjethani

Replying to mjethani:

We could probably handle most of it by messaging between the popup and the background page, but sending messages to the background page just to get or set a preference value is probably a bit of an overkill.

Well, the options page does it, so I was wrong. I've uploaded ​29532767 now.

comment:14 Changed on 09/01/2017 at 12:09:21 PM by mjethani

  • Review URL(s) modified (diff)

comment:15 Changed on 09/04/2017 at 02:40:54 PM by abpbot

A commit referencing this issue has landed:
​Issue 5593 - Return new value from prefs.toggle...

comment:16 Changed on 09/18/2017 at 11:10:25 AM by kzar

  • Blocking 5080 added

For reference this is blocking #5080 now since the way that popup.js makes use of the require function from the background page won't work once we're using Webpack. In my opinion we should replace all uses of require with messaging here.

comment:17 Changed on 09/18/2017 at 01:28:11 PM by kzar

  • Blocking 5708 added

comment:18 Changed on 09/18/2017 at 01:29:27 PM by kzar

  • Blocking 5080 removed

(I've created a separate issue (#5708) for that change now.)

comment:19 follow-up: Changed on 09/18/2017 at 06:34:03 PM by sebastian

I think what Wladimir meant on the review is that he prefers to have this effort split up into several reviews. However, in order to fix the popup on Firefox (i.e. this issue) we have to remove all use of require() from popup.js eventually, hence #5708 seems redundant.

But how exactly is accessing modules through chrome.extension.getBackgroundPage().require() from popup.js causing problems with WebPack?

comment:20 Changed on 09/19/2017 at 08:46:45 AM by trev

require is no longer a global symbol with WebPack. We could export it, but it will accept numerical module IDs rather than strings then (WebPack replaces invocations accordingly during module compilation). So reusing require from other contexts is not possible.

comment:21 Changed on 09/19/2017 at 01:44:14 PM by kzar

  • Blocking 5708 removed

comment:22 in reply to: ↑ 19 Changed on 09/19/2017 at 01:45:15 PM by kzar

  • Blocking 5080 added

Replying to sebastian:

I think what Wladimir meant on the review is that he prefers to have this effort split up into several reviews. However, in order to fix the popup on Firefox (i.e. this issue) we have to remove all use of require() from popup.js eventually, hence #5708 seems redundant.

OK, I've closed it as a duplicate.

comment:23 Changed on 09/25/2017 at 06:01:39 PM by kzar

  • Blocking 5535 added

comment:24 Changed on 09/27/2017 at 10:04:07 PM by abpbot

A commit referencing this issue has landed:
​Issue 5593 - Use messaging in popup for prefs, whitelisting, and stats

comment:25 Changed on 09/30/2017 at 02:58:24 AM by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:26 Changed on 10/05/2017 at 12:14:02 PM by kzar

(I think there's a regression with the above commit, see #5832.)

comment:27 follow-up: Changed on 10/05/2017 at 04:27:26 PM by kzar

So I spent some time today attempting to remove the last require in stats.js, which was FilterNotifier used for keeping the "Ads blocked" counts up to date in real time.

That turned out to be a pain, since the popup does not get a tabId the messageResponder can't send it the messages when the count is updated. Sebastian, Manish and I discussed it in IRC and we're now questioning if the complexity of possibly solutions is worth it at all.

What do you think Thomas? Would you be OK if we removed the real-time block count from the popup, so that the numbers stayed static until the popup was next opened?

comment:28 in reply to: ↑ 27 Changed on 10/05/2017 at 04:41:20 PM by greiner

  • Cc jeen added

Replying to kzar:

What do you think Thomas? Would you be OK if we removed the real-time block count from the popup, so that the numbers stayed static until the popup was next opened?

I think that's a question for Product because they may know better how important this aspect is to the overall user experience - especially since they've already been working on a redesign of the popup.

@Jeen How important do you think it is to update the counter in the popup in real time? Any ideas on what we could do to avoid a bad experience due to the icon counter and the popup counter being out-of-sync?

comment:29 follow-up: Changed on 10/05/2017 at 06:10:39 PM by kzar

  • Blocked By 5835 added

comment:30 in reply to: ↑ 29 ; follow-up: Changed on 10/06/2017 at 10:04:38 AM by jeen

Replying to kzar:

What do you think Thomas? Would you be OK if we removed the real-time block count from the popup, so that the numbers stayed static until the popup was next opened?

When you say the number stay static until the popup is next opened - does that mean that if for example I am on a page which contains 6 blocked ads which is reflected in the bubble UI icon, when I open the popup - it will display 6 ads blocked upon opening? Or would it show #old count upon opening, I close the popup, open it again and then it displays 6 ads blocked? And if the ad count stayed static, what count will it display?

The ad count is an important number to be displayed within the popup, but I would like to better understand how it works.

comment:31 Changed on 10/06/2017 at 01:09:38 PM by kzar

  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:32 Changed on 10/06/2017 at 01:10:04 PM by kzar

  • Description modified (diff)

comment:33 in reply to: ↑ 30 Changed on 10/06/2017 at 01:22:37 PM by kzar

Replying to jeen:

Replying to kzar:

What do you think Thomas? Would you be OK if we removed the real-time block count from the popup, so that the numbers stayed static until the popup was next opened?

When you say the number stay static until the popup is next opened - does that mean that if for example I am on a page which contains 6 blocked ads which is reflected in the bubble UI icon, when I open the popup - it will display 6 ads blocked upon opening? Or would it show #old count upon opening, I close the popup, open it again and then it displays 6 ads blocked? And if the ad count stayed static, what count will it display?

The ad count is an important number to be displayed within the popup, but I would like to better understand how it works.

Currently when you open the popup the number of blocked adverts is listed. If more adverts are blocked while you still have the popup open the blocked advert numbers increase there in real-time.

With the change we're suggesting the number of blocked adverts would still be listed, but if more adverts were blocked while the popup was still open the number there wouldn't be updated. If the user then closed and re-opened the popup the number would be up to date again.

We ask because updating those in real-time is problematic for both Firefox web-ext support and the switch to webpack for our build system. There _is_ a way to make it work, but it's a fairly complicated and will be quite expensive to implement.

comment:34 Changed on 10/06/2017 at 01:45:46 PM by jeen

I'm happy to keep the number static until the popup is opened. As long as the ad count is accurately reflected on first opening the bubble UI then it's fine.

Most users don't keep the bubble UI open for long - and would only open it to perform a specific task, so having the ad count display in real time is not so important.

comment:35 Changed on 10/06/2017 at 01:49:49 PM by kzar

Awesome, I'll go ahead with that then thanks.

comment:36 Changed on 10/06/2017 at 01:58:43 PM by kzar

  • Review URL(s) modified (diff)

comment:37 Changed on 10/06/2017 at 08:18:20 PM by abpbot

A commit referencing this issue has landed:
​Issue 5593 - Stop updating the stats in the popup in real-time

comment:38 Changed on 10/09/2017 at 02:21:48 PM by kzar

  • Blocked By 5847 added

comment:39 Changed on 10/09/2017 at 04:01:27 PM by kzar

  • Description modified (diff)

comment:40 Changed on 10/09/2017 at 04:26:01 PM by kzar

  • Description modified (diff)

comment:41 Changed on 10/09/2017 at 04:30:28 PM by kzar

  • Description modified (diff)

comment:42 Changed on 10/09/2017 at 04:48:50 PM by abpbot

A commit referencing this issue has landed:
​Issue 5593 - Use messaging for the popup's notification code

comment:43 Changed on 10/09/2017 at 05:02:09 PM by kzar

  • Blocking 5080, 5535 removed

Since we no longer have any use of require in these files this issue no longer blocks the webpack ones.

comment:44 Changed on 10/10/2017 at 05:32:47 PM by abpbot

A commit referencing this issue has landed:
​Issue 5593 - Merge notification.js and stats.js into popup.js

comment:45 Changed on 10/10/2017 at 05:38:56 PM by mjethani

  • Review URL(s) modified (diff)

comment:46 Changed on 10/10/2017 at 05:40:02 PM by mjethani

  • Review URL(s) modified (diff)

comment:47 Changed on 10/10/2017 at 05:44:38 PM by mjethani

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

comment:48 Changed on 10/10/2017 at 05:45:03 PM by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:49 Changed on 10/11/2017 at 02:49:52 PM by kzar

  • Description modified (diff)

Could you add some testing instructions for the parts of the you touched Manish?

comment:50 Changed on 10/17/2017 at 11:22:45 AM by mjethani

  • Description modified (diff)

comment:51 Changed on 03/28/2018 at 07:38:28 PM by abpbot

Edit: Removed wrong commit reference

Last edited on 03/29/2018 at 07:58:31 AM by wspee

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 mjethani.
Attachments ↑ Description ↑
 
Note: See TracTickets for help on using tickets.