Opened 8 months ago

Closed 8 months ago

Last modified 3 months ago

#6948 closed defect (fixed)

"block element" bug in ABP 3.3.2 (only chrome)

Reported by: mapx Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, mjethani, kzar, greiner, Ross Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29894584/

Description (last modified by kzar)

Environment

windows 10
chrome 69
ABP 3.3.2

How to reproduce

  1. go to a page with images, for example here:

https://www.purelypoultry.com/pekin-ducklings-p-621.html

  1. right click an image, block element, confirm, the image is blocked
  2. right click another image on the same page

Observed behaviour

The "block element" option / feature disappeared

Note

  • It's ok in ABP for firefox
  • to see again the "block element" option just click another tab and then go back to the images page

Reported here:
https://adblockplus.org/forum/viewtopic.php?f=10&t=59376&start=0

Hints for testers

With this fix I rewrote the context menu logic, so we'll need to test our "Block element" right-click context menu button quite thoroughly.

On Firefox for Android the context menu should not be displayed, since the filter composer isn't designed for mobile and some of the required APIs aren't available. Please test that general adblocking functionality works OK there, since this change has modified the "If not Firefox for Android" checks.

On other platforms, the "Block element" context menu is supposed to show up when the user right-clicks on image, video and audio elements. But only if the page isn't whitelisted, and if the page was loaded after the extension was last installed/reloaded. When the "Block element" button in the context menu is clicked, the filter composer window should pop up and the element which was right clicked should be highlighted red.

Behind the scenes, the browser actually only lets us add or remove our context menu item for _all_ tabs in all windows at once. So, we actually watch to see when the user changes tab, to check if we should display the context menu for the current tab. Supposing the user switches from one whitelisted tab to another, we don't have to do anything, but then if they switch to a non-whitelisted page we might have to then add our context menu item.

So, test things like switching between tabs and windows and check that the context menu is displayed when it should be.

Finally, some logic relating to if the "Block Element" button in the popup was touched with this change. So, test that still works too.

Change History (12)

comment:1 Changed 8 months ago by mapx

  • Description modified (diff)

comment:2 Changed 8 months ago by mapx

  • Description modified (diff)

comment:3 Changed 8 months ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

I could reproduce it on Chrome 69 with Adblock Plus 3.2, 3.3.2 and on the "next" branch as of revision 811116e9757e, but sometimes it only happened after using "Block element" twice. I couldn't reproduce it at all on Chrome 68.

Last edited 8 months ago by sebastian (previous) (diff)

comment:4 Changed 8 months ago by mapx

ABP console
Unchecked runtime.lastError while running browserAction.setIcon: No tab with id: 817543195.

comment:5 Changed 8 months ago by sebastian

This issue has been reported before, see #6490. But it might be unrelated of the behavior reported here, as this error already showed up before Chrome 69.

comment:6 Changed 8 months ago by kzar

  • Owner set to kzar

I can reproduce this on Chrome 68, investigating.

comment:7 Changed 8 months ago by kzar

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

comment:8 Changed 8 months ago by kzar

  • Cc Ross added
  • Description modified (diff)

comment:9 Changed 8 months ago by kzar

  • Description modified (diff)

comment:10 Changed 8 months ago by abpbot

comment:11 Changed 8 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:12 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. The context menu entry does not disappear after first use, is hidden when the page is whitelisted and is kept in sync with changing tab/whitelist state.

This still does occur on Edge however, so I've opened #7299 separately.

ABP 3.4.3.2253
Firefox 65.0.1 / 51 / Windows 10
Firefox Mobile 65.0.1 / Android 7.1.1
Chrome 72.0.3626.109 / 49.0.2623.75 / Windows 10
Opera 58.0.3135.65 / 36.0.2130.80 / Windows 10
Edge 44.17763.1.0 / Windows 10

Note: See TracTickets for help on using tickets.