Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

fix various noscript issues #8410

Merged
merged 1 commit into from
Apr 20, 2017
Merged

fix various noscript issues #8410

merged 1 commit into from
Apr 20, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Apr 20, 2017

fix #8403

  1. change noscript icon from button to span to fix CSP error
  2. fix repeated toggling of noScriptInfo dialog
  3. refactor to remove deprecated code and combine windowActions
  4. increase noscript click area
  5. fix security icon disappearing on twitter.com

test plan:

  1. disable scripts on twitter.com. the lock icon should not disappear
  2. click noscript icon
  3. open browser console. you should not see any CSP errors.
  4. now click the noscript icon repeatedly. you should see the popup come up and disappear repeatedly.
  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

fix #8403

1. change noscript icon from button to span to fix CSP error
2. fix repeated toggling of noScriptInfo dialog
3. refactor to remove deprecated code and combine windowActions
4. increase noscript click area
5. fix security icon disappearing on twitter.com

test plan:
1. disable scripts on twitter.com. the lock icon should not disappear
2. click noscript icon
3. open browser console. you should not see any CSP errors.
4. now click the noscript icon repeatedly. you should see the popup come up and disappear repeatedly.
@diracdeltas diracdeltas added this to the 0.15.0 milestone Apr 20, 2017
@diracdeltas diracdeltas self-assigned this Apr 20, 2017
@diracdeltas diracdeltas requested a review from bsclifton April 20, 2017 08:22
@@ -119,7 +120,7 @@ module.exports.siteHacks = {
onBeforeSendHeaders: function(details) {
if (details.requestHeaders.Referer &&
details.requestHeaders.Referer.startsWith('https://twitter.com/') &&
details.url.startsWith('https://mobile.twitter.com/')) {
details.url.startsWith(appConfig.noScript.twitterRedirectUrl)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice cleanup 👍

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes work great! 👍

@bsclifton bsclifton merged commit 428d0c6 into master Apr 20, 2017
@bsclifton bsclifton deleted the fix/8403 branch April 20, 2017 15:30
syuan100 pushed a commit to syuan100/browser-laptop that referenced this pull request Nov 9, 2017
by replacing form with div. Input element does not have to have form as a parent.

Closes brave#8457
Follow-up to brave#8410

Auditors: @cezaraugusto

Test Plan:
1. Run `npm run test -- --grep='can selectively allow scripts'`
2. disable scripts on twitter.com. the lock icon should not disappear
3. click noscript icon
4. open browser console. you should not see any CSP errors.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

various issues with noscript icon
2 participants