Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid triggering popup blocker on some old versions of firefox #4166

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

gdh1995
Copy link
Contributor

@gdh1995 gdh1995 commented Nov 7, 2022

The old code in #4000 ignores Firefox versions which are older than 96 while don't belong to 91. So this PR adds it.

Should fix #4165 .

@tim-goto
Copy link

tim-goto commented Nov 21, 2022

To circumvent the issue that is being fixed by this PR I installed the latest version of the plugin locally and it does not have this issue besides the fact that this PR is not yet merged. Maybe I am running into something differently though..

Edit: sorry, I was wrong, it does not work on master. I checked out v1.67.1 now it works

lib/dom_utils.js Outdated
@@ -323,7 +323,7 @@ var DomUtils = {
// In firefox prior to 96, simulating a click on an element would be blocked. In 96+,
// extensions can trigger native click listeners on elements. See #3985.
const mayBeBlocked = event === "click" && Utils.isFirefox() && parseInt(Utils.firefoxVersion()) < 96
&& /^91\.[0-5](\.|$)/.test(Utils.firefoxVersion())
&& /^(?!91\.)|^91\.[0-5](\.|$)/.test(Utils.firefoxVersion())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regexp is now pretty complicated to look at, and the comment doesn't indicate why we're checking for Firefox 91.

I think this would be much easier to work with if 1) we added firefoxMajorVersion and firefoxMinorVersion to Utils and used those for comparisons, so we don't have to use parseInt on a string and regexps to parse those out here, and 2) updated the comment to describe why we're checking against Firefox 91 in this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Philc, I've updated the code to make it clear.

1. The old code ignores Firefox versions which are older than 96 while don't belong to 91.
2. On Firefox 91.6+, a plain `click` event on `<a target=_blank>` will still
   trigger the popup blocker, so this commit avoids it
@gdh1995 gdh1995 changed the title fix a typo when checking firefox version avoid triggering popup blocker on some old versions of firefox Nov 30, 2022
@gdh1995
Copy link
Contributor Author

gdh1995 commented Nov 30, 2022

And as I've updated in #3985 (comment) , Firefox 91.6+ still reports "popup-blocked" on a plain click event on <a target=_blank>, so I also add a check in this commit to fix it.

Just now I tested the commit cb78fa5 on Firefox 90, 91.0esr, 91.6esr and 96, and it works perfectly whether Firefox's Block pop-up windows (aka. dom.disable_open_during_load) is enabled or not.

@philc philc merged commit 4b5734f into philc:master Dec 1, 2022
@philc
Copy link
Owner

philc commented Dec 1, 2022

Much clearer, thanks @gdh1995

@gdh1995 gdh1995 deleted the fix-firefox-version-check branch December 2, 2022 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox can't open links in new windows
3 participants