fix popup width in Edge

[Hsn723]

Should fix #95 and similar issues
Not entirely sure, but I am assuming best practice here is to not touch anything under src/ for platform-specific issues so I placed my changes in platform/edge and reflected them in make-edge.sh.

[nicole-ashley]

Thanks @Hsn723!

This appears to fix it when enabling advanced mode, but it causes another issue when disabling it. For some reason the firewall is not hidden in the pop-up UI when you disable it from settings, even though it appears that the .advancedMode class is removed. This behaviour is also present in Chrome (though I'm not sure about the class). I wonder if this is a bug in the core?

Possibly a fix at the moment that still works with the above bug is to remove the .advancedMode class from your change:

body {
  width: 100%;
}

This at least seems to works in Edge in all cases, including when the .advancedMode class is removed but the firewall is still showing.

@gorhill, is the above CSS change something that would be ok in the core repo? Or will it cause issues with other browsers?

[gorhill]

will it cause issues with other browsers?

I will have to test to find out. I will do so when I get the time and will notify you if I add this to the core.

[nicole-ashley]

Thanks @gorhill!

[Hsn723]

@nikrolls Whether width: 100% is applied to .advancedUser or simply body should not matter. I figured .advancedUser would be less broad but can change that to body instead if you prefer. I used .advancedUser since I could make that fix Edge specific with minimal effort. To apply this fix on body without applying it on other platforms the build command sed -i 's/body {/body {\n\twidth:100%;/' $DES/css/popup.css needs to be added to make-edge.sh instead which is less elegant.

Also, the additional issue you found is actually not directly related to this fix, since the code for this fix only compiled for the Edge target. That you could repro it in Chrome indicates that it is a separate issue.

Upon investigation, the behavior you describe is actually related to popupData.dfEnabled not being toggled properly when switching from Advanced to Basic.

As the fix for that needs to be done in the ublock.js file this would also affect all platforms. Namely, around line 339:

case 'advancedUserEnabled':
        if ( value === true ) {
            us.dynamicFilteringEnabled = true;
        }
        break;

Needs to be

case 'advancedUserEnabled':
        us.dynamicFilteringEnabled = value;
        break;

In order for the behavior you described not to occur. As this is out of the scope of the current PR and affects all browsers, I could send another PR to the upstream uBO repo for that additional issue.

[nicole-ashley]

@Hsn723 What I meant was that because of this issue, applying the CSS fix to only .advancedUser solved one issue but caused another: when you disable advanced mode, the popup goes back to its usual width, but because of the other bug you can only see part of the firewall in it. Therefore a fix that makes Edge work like Chrome currently does is to remove the .advancedUser class from your PR.

Regardless, if it can be made in the core I'm happier with it because I'd prefer not to add more workarounds to the build script.

[Hsn723]

@nikrolls I see your point. I've removed the .advancedUser class and also included the fix for the other bug since it was a relatively small fix. Let me know if you'd rather keep them separate.

[gorhill]

Unfortunately using width: 100%; for body causes an issue in Chromium (and possibly other platforms) when the popup is brought up from within the logger:

This also causes a regression for the Firefox/legacy version:

I did not test the Firefox for Android version, but I fear this could cause issue as well.

One thing I could do to avoid the sed is to add <link rel="stylesheet" href="css/vapi-popup.css" type="text/css"> to popup.html, and the Edge version could put the fix in there.

[Hsn723]

@gorhill Thank you, that sounds like a more elegant and future-proof solution than the sed approach. I'll update this PR accordingly once that's available in the upstream.

[nicole-ashley]

Thanks @gorhill, I'm happy with that solution if it fits in with your usual strategy for platform-specific overrides.

[nicole-ashley]

@Hsn723, we could actually add this ahead of time based on gorhill@f6afd8c. They will probably go out in the same release anyway.

[Hsn723]

@nikrolls Cool, I've gone ahead and moved things accordingly.

[nicole-ashley]

Looks good, just a couple of notes.

[nicole-ashley]

We can't make these changes here, they need to be made in the core repository.

[nicole-ashley]

Unnecessary blank line

[nicole-ashley]

For consistency with the JS line could this be:

cp -R platform/edge/*.css $DES/css

And then move the vapi-popup.css file to the edge folder.

[nicole-ashley]

Missing new line at the end of the file 😊

[Hsn723]

@nikrolls Thanks for the quick review. I've made the requested changes. For the part that affects the core repository I'll submit a separate PR over there.

[lychichem]

Tested by sideload 1.15.4 plus this fix into Edge. I have to say, the fix do solve the problem, but just partly. The popup will show contents properly. But the popup sometimes still just show the right side with proper content. However, if mouse is hovered on it, the popup will be just okay. So I think this fix is just a workaroud tweak, the true fix to this issue maybe rely on Microsoft.

[nicole-ashley]

Thanks @Hsn723! Sorry to be a bother, but do you mind squashing these commits? Especially since there have been a couple of different approaches throughout. Makes the commit history nice and clean. Then I can merge right away!

[nicole-ashley]

Thanks @lychichem for giving it a go. I'll run it for a while and see what I come up with as well. The more feedback the better. Which release branch of Windows are you running?

[Hsn723]

@nikrolls No problem, as requested I've squashed them all into one clean commit.