Opened 2 years ago

Closed 2 years ago

Last modified 20 months ago

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

Change History (51)

comment:1 Changed 2 years ago by hfiguiere

  • Description modified (diff)

comment:2 Changed 2 years ago 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 2 years ago by hfiguiere (previous) (diff)

comment:3 Changed 2 years ago 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 2 years ago 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 2 years ago 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 2 years ago 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 2 years ago 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 2 years ago by trev (previous) (diff)

comment:8 Changed 2 years ago by mjethani

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

comment:9 Changed 2 years ago by mjethani

  • Status changed from new to reviewing

comment:10 follow-up: Changed 2 years ago 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 2 years ago 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 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:13 in reply to: ↑ 10 Changed 2 years ago 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 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:15 Changed 2 years ago by abpbot

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

comment:16 Changed 2 years ago 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 2 years ago by kzar

  • Blocking 5708 added

comment:18 Changed 2 years ago by kzar

  • Blocking 5080 removed

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

comment:19 follow-up: Changed 2 years ago 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 2 years ago 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 2 years ago by kzar

  • Blocking 5708 removed

comment:22 in reply to: ↑ 19 Changed 2 years ago 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 2 years ago by kzar

  • Blocking 5535 added

comment:24 Changed 2 years ago by abpbot

comment:25 Changed 2 years ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:26 Changed 2 years ago by kzar

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

comment:27 follow-up: Changed 2 years ago 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 2 years ago 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 2 years ago by kzar

  • Blocked By 5835 added

comment:30 in reply to: ↑ 29 ; follow-up: Changed 2 years ago 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 2 years ago by kzar

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

comment:32 Changed 2 years ago by kzar

  • Description modified (diff)

comment:33 in reply to: ↑ 30 Changed 2 years ago 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 2 years ago 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 2 years ago by kzar

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

comment:36 Changed 2 years ago by kzar

  • Review URL(s) modified (diff)

comment:37 Changed 2 years ago by abpbot

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

comment:38 Changed 2 years ago by kzar

  • Blocked By 5847 added

comment:39 Changed 2 years ago by kzar

  • Description modified (diff)

comment:40 Changed 2 years ago by kzar

  • Description modified (diff)

comment:41 Changed 2 years ago by kzar

  • Description modified (diff)

comment:42 Changed 2 years ago by abpbot

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

comment:43 Changed 2 years ago 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 2 years ago by abpbot

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

comment:45 Changed 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:46 Changed 2 years ago by mjethani

  • Review URL(s) modified (diff)

comment:47 Changed 2 years ago by mjethani

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

comment:48 Changed 2 years ago by mjethani

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

comment:49 Changed 2 years ago by kzar

  • Description modified (diff)

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

comment:50 Changed 2 years ago by mjethani

  • Description modified (diff)

comment:51 Changed 20 months ago by abpbot

Edit: Removed wrong commit reference

Last edited 20 months ago by wspee (previous) (diff)
Note: See TracTickets for help on using tickets.