Cannot re-enable site after disabling

dhowe commented 4 months ago

I tried to reproduce the issue when...

Description

It seems impossible to un-disable a site after disabling it: see also #2462

Steps to Reproduce

Disable a site, then try to re-able it

mneunomne commented 4 months ago

@dhowe how did you disable the page in the first place?

The only case in which I was able to reproduce this error was when a root page of a domain is directly added to the trusted-sites list:

https://www.google.com/

instead of

www.google.com

this causes this makes the "re-enable" fail since it is only the second format is removed, but not the first. It seems that uBlock doesn't have this same issue, will check why and resolve asap.

dhowe commented 4 months ago

Here is an example:

Visit https://www.carousell.com.hk/
Click 'Disable' on the menu and refresh page
Try to click 'Disable' again to un-disable, nothing happens [primary bug]

"www.carousell.com.hk" is added to TRUSTED SITES

What should happen:

'Disable' should change to 'Enable' when disabled=true
You should be able to re-enable a site by pressing 'Enable'
It should be more clear what is happening when you press 'Disable' the first time, and whether you have to press again to select domain or page

mneunomne commented 4 months ago

@dhowe I just implemented the ability to switch the scope of the disable status, without having to chick again on the enable button.

The UI issue that appears from that, is that within the uBlock structure, we can only access true/false if a page is disabled, but we can't know if the scope of this disable is domain or page-specific. Which makes the UI confusing since when u click on the popup, it doesn't properly shows what is the current scope by which the page is disabled.

For that I see two possible solutions:

A) Leave it as it is, but hide the popup-arrow while the page is alreadhy disable, to generate less confusion, and make clear that the scope can't be changed while the page is already disabled.
B) Implement a value in the popupData object to check if the page is disabled by page-specific or domain rule.

dhowe commented 4 months ago

I remember some discussions about why it wasn't practical to store whether the user had chosen page/domain for every site. So how about this flow: (it may be your A):

In 'enabled' state, when 'Disable' icon is pressed it acts as a proper menu, and the user must choose page or domain
In 'disabled' state, user sees only 'Enable' - pressing this removes the entry (page or domain) from trusted sites
To change from domain to page or vice-versa, user must enable then disable again with the different setting

mneunomne commented 4 months ago

So I guess this mean that the "popup arrow" needs to dissapear when the website is currently disabled? In order not to confuse the user that it is possible to change the scope of "disable" the website while it is already disabled.

dhowe commented 4 months ago

yes, it needs to disappear when the text changes to 'Enable'

mneunomne commented 4 months ago

Ok, I finally understood why this issue with the disable was happening in cases page-specific disable where the last char of a url ended with a '/':

https://github.com/dhowe/AdNauseam/blob/8c861d2ce2f6abc5bbaafd1d9d9b19ea9e543acc/src/js/adn/core.js#L2378

We are, for some reason, trimming the last char of the url request, which was causing the url saved in trusted sites to be faulty. I'll try to will understand why we do this, and make sure this doesn't happen in such case.

mneunomne commented 4 months ago

Created an exception for the trimming character to avoid it happening when toggling the disable button. Also set to hide the arrow when the disable is active AND the popup is not yet closed.

dhowe commented 4 months ago

We are, for some reason, trimming the last char of the url request, which was causing the url saved in trusted sites to be faulty. I'll try to will understand why we do this, and make sure this doesn't happen in such case.

I don't remember, but this may have been an attempt to ensure that URL versions (https://site.com and https://site.com/) are treated as the same site/domain. Please verify in your fix that this is still the case

mneunomne commented 4 months ago

In relation to the disable/enable, yes I have tested and it is working as expected. both https://site.com/ and https://site.com/ are treated as https://site.com/ when disabling as page-specific, and site.com when treating it as domain.

mneunomne commented 3 months ago

Fixed, closing

dhowe / AdNauseam