Unable to close filter suggestion window in Block Element

Environment

• OS version: macOS 10.15
• Browser version: Opera (older and latest versions)
• Extension version: Reproducible in ABP 3.8 and dev builds for ABP 3.9
• Last working version: NA

Steps to reproduce

1. On Opera, open any website and Click "Block Element" in the Pop-up UI
2. Try blocking an element on the page
3. Click the "Add" or "Cancel" buttons in the filter suggestion window

Observed behavior

The filter suggestion window remains open after clicking the "Add" or "Cancel" button

Expected behavior

The filter suggestion window should be closed after clicking the "Add" or "Cancel" button

Further information

• This only happens in Opera. Other browsers show the expected behavior.
• With the latest changes in 2019-4: https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/-/issues/761 and https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/-/issues/638, clicking the Unselect in the filter's suggestion window has the same issue, causing the composer windows to pile up with each user's selection

added 1 deleted label

changed milestone to %Release 2019-3.1

added 1 deleted label

While this is not a regression when compared to ABP 3.8, I'd rather get it fixed sooner than later, especially if it turns out it's an easy fix, so I added 2019-3.1 milestone to it.

assigned to @ThomasGreiner

I was able to trace this error back to browser.tabs.remove() which throws the following error:

Uncaught (in promise) Error: Tabs cannot be edited right now (user may be dragging a tab).
  (anonymous) @ polyfill.js:131
  value @ polyfill.js:121
  (anonymous) @ composer.js:64
  Promise.then (async)
  closeMe @ composer.js:64
  closeDialog @ composer.js:79

Therefore it looks like Opera, unlike other browsers, doesn't allow us to close tabs in standalone windows. There are a couple of ways how we could resolve this by calling browser.windows.remove() instead of browser.tabs.remove():

• adblockpluschrome: Extend "app.get:senderId" message to also return window ID.
• adblockpluschrome: Provide new message that allows UI to close itself.
• adblockpluschrome: Handle error in polyfill.
• adblockplusui: Make additional browser API call to determine window ID using tab ID from "app.get:senderId".

@snoack As outlined above, this could be fixed in adblockpluschrome which I'd consider to make the most sense as it's a platform-specific extension API problem. Otherwise, we could add the described workaround in adblockplusui, if you prefer that.

Makes sense. Feel free to file an issue in adblockpluschrome. FWIW, this doesn't seem to be a new regression, hence it's not critical for the upcoming release, right?

moved to eyeo/adblockplus/adblockpluschrome#204 (closed)

Closing this issue since it was moved to adblockpluschrome: eyeo/adblockplus/adblockpluschrome#204 (closed)

removed 1 deleted label

removed milestone %Release 2019-3.1

unassigned @ThomasGreiner

should we re-open this, as we can handle the exception on our side?

eyeo/adblockplus/adblockpluschrome#204 (comment 347767330)

Let's wait for the discussion to conclude before we do anything here. I'm fine with reopening it and keeping it open until we're confident that no changes to adblockplusui will be necessary.

@agiammarchi @ThomasGreiner Since eyeo/adblockplus/adblockpluschrome#204 (closed) has been rejected, should we go ahead and implement the workaround in abpui?

We plan to ship the new UI/UX for the block element (https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/-/issues/638) in 2019-4 and also promote it in the updates page, so I though it might be a good idea to resolve this issue by then.

@ursakacar Thanks for bringing this up again. I agree that it makes sense to fix it together with the release of #638 (closed) so I'll assign it to release-2019-4, as suggested.

Fortunately, based on this comment, we should be able to replace the existing workaround with window.close(), which sounds reasonable to do in adblockplusui.

@ThomasGreiner from what I understood, we don't have to first implement the workaround, and then later replace it with window.close(), but we could go ahead and use window.close() in 2019-4 already right?

The only instance of browser.tabs.remove() I could find was this: https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/-/blob/353cf5f375053bd7439107a22bb7f434bbcb25ea/ext/content.js#L230, would this be replaced with window.close()?

Would such change affect anything else than closing the filter composer window (trying to think of the testing scope)?

@ursakacar We're talking about two workarounds here:

• A: The existing workaround for Firefox <52 is to use browser.tabs.remove() instead of window.close().
• B: The new workaround for older Opera versions is to use window.close() instead of browser.tabs.remove().

Therefore what I'm saying is that we could remove (A), since we're going to drop support for Firefox 51, and instead use window.close(), which is expected to work across all versions of Firefox and Opera.

That being said, we should verify the assumption that window.close() works in Firefox 52-57, since this comment suggests otherwise.

The only instance of browser.tabs.remove() I could find was this: https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/-/blob/353cf5f375053bd7439107a22bb7f434bbcb25ea/ext/content.js#L230, would this be replaced with window.close()?

No, that's part of our mocks for the test environment. The code we'd need to replace is located in adblockplusui/composer.js.

Would such change affect anything else than closing the filter composer window (trying to think of the testing scope)?

No, this change is not expected to affect anything other than the filter composer dialog window. Only if we were to fix it in adblockpluschrome, we'd end up affecting other parts of the extension.

@ThomasGreiner thanks a lot for your elaborate reply, I understand this much better now.

@ThomasGreiner

That being said, we should verify the assumption that window.close() works in Firefox 52-57, since this comment suggests otherwise.

I run some tests and it does work on Firefox 52-57. Unfortunately, after testing the next versions I found out that it doesn't work on versions 63, 63.0.1 and 63.0.3 (Linux & Windows): neither "Cancel" nor "Block" will close the popup window if we use windows.close(). As a curious note, only 63.x has the issue; 64-70 (stopped tests there) works as well.

So, should I go then with the suggested solution where the error is catched and close the windows only then?

We discussed this yesterday so it might make sense to summarize here, for reference:

• A workaround seems to be necessary after all.
• It's still unclear whether WebExt is supposed to provide us with a platform-independent implementation of browser.windows.remove().
• We can go ahead and implement the workaround on our end already to avoid further delays, while the above question remains unresolved.

@2alin Anything I missed or misunderstood?

@ThomasGreiner All looks good, I just reignited the discussion in the other thread (see comment).

@2alin Thanks and feel free to implement the workaround for now. Have you also been able to find out whether this issue is related to #808 (closed)?

Done and issue not related to #808 as explained here.

reopened

changed the description

changed milestone to %Release 2019-4

added 1 deleted label

assigned to @2alin

mentioned in issue eyeo/adblockplus/adblockpluschrome#204 (closed)

mentioned in merge request !536 (merged)

added 1 deleted label and removed 1 deleted label