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

Pop-up blocker warning alerts #13

Closed
ameshkov opened this issue Aug 10, 2017 · 10 comments
Closed

Pop-up blocker warning alerts #13

ameshkov opened this issue Aug 10, 2017 · 10 comments
Assignees
Milestone

Comments

@ameshkov
Copy link
Member

ameshkov commented Aug 10, 2017

Here is the task for the alerts layout:
#12

Some general notes on the task:

  1. Alerts should be shown inside of a dynamically created iframe (look at how it's done in Assistant) in order to not mess with the websites' own styles.
  2. Instead of generic blocking, we can now simply suppress all the pop-ups and show the alert. The only exception should be the obvious pop-ups.
  3. For every new blocked pop-up, you should show a new alert. Pop-up is new if the URL is different.
  4. The pop-up blocker text will be translated via oneskyapp, so you should not hardcode strings.

Alert state 1

popup blocker extension - page moqups 2017-08-10 12-43-49

Alert is shown when you block an attempt to open a window.

  • example.org: domain of the pop-up window. Clicking on this link opens the popup.
  • Always allow example.org: allows all the pop-up windows leading to example.org from the current website.
  • Allow all pop-ups on this website: that's simple.

Alert state 2

After some time alert collapses into state 2:
popup blocker extension - page c2 b7 moqups f0 9f 94 8a 2017-08-10 13-00-55

Click on the "pop-up" link expands it back to the state 1.

Also, if pop-up is detected by a "generic" algorithm from the first version of an extension, we should show alert in the collapsed state right away.

How alerts are stack

Basically, they are located one under one:
https://monosnap.com/file/PFeTq9dLBERo0DYPbBeadRIcuqg11U

There should be no more than 4 alerts. When fifth emerges, remove the oldest one.

Alert lifecycle

2 seconds in the state 1.
5 seconds in the state 2.
Disappears after that.

@ameshkov ameshkov added this to the 2.1 milestone Aug 10, 2017
@ameshkov
Copy link
Member Author

Regarding showing the stack trace, it won't help casual users, so there's no need to put it to the regular UI. However, it makes sense to add a "Stack trace" link to the dev and beta builds.

@ameshkov
Copy link
Member Author

Also, in light of this change, I am no more sure we need to merge the #11. It seems that we can easily cover it with this change in a more user-friendly manner and less false-positive blockings.

@theseanl
Copy link
Contributor

What made you think that #11 will cause more false-positives?

@ameshkov
Copy link
Member Author

More complicated -> more false-positives

@ameshkov
Copy link
Member Author

@seanl-adg added one more thing to the description:

Also, if pop-up is detected by a "generic" algorithm from the first version of an extension, we should show alert in the collapsed state right away.

@theseanl
Copy link
Contributor

I need a description of the following:

  • When an 'Always allow example.org' is clicked, how are we going to notify that a domain is added to whitelist?
  • How are we going to allow users to manage whitelisted domains, domains to use strict blocking mode, etc?
  • How can users enable strict blocking mode for a website?

@ameshkov
Copy link
Member Author

Tested on http://code.ptcong.com/demos/bjp/demo.html

Issues

  1. State1->State2 transition should not be happening when user's mouse is over alert window;
  2. Click on the alert window on that demo page triggers pop-up second time;
  3. We need to have a JS confirmation dialog when the user clicks "Allow..." buttons;
  4. Increase state 1 lifetime to 3 seconds plz

@ameshkov
Copy link
Member Author

Answers

When an 'Always allow example.org' is clicked, how are we going to notify that a domain is added to whitelist?

With a confirmation dialog it would be pretty clear. However, we should also reload the page (reload without cache).

How are we going to allow users to manage whitelisted domains, domains to use strict blocking mode, etc?

Will be discussed in another issue.

How can users enable strict blocking mode for a website?

This also should be discussed in another issue.

@theseanl
Copy link
Contributor

  1. Click on the alert window on that demo page triggers pop-up second time;

That's legit, that's how BetterJsPop works. It adds its own masks on each of the document's iframes, including dynamically generated ones, and trigger popup when a user clicks on it.

@ameshkov
Copy link
Member Author

Testing it on http://oploverz.in

  1. Click does not expand the alert back:
    https://monosnap.com/file/nraX5KMYZH0ZecC7GPvRt1hjYeunmI

  2. http:// prefix is redundant:
    https://monosnap.com/file/6gqO88vcxH77n4jbwd7bmZedTzZ7bd

@ameshkov ameshkov reopened this Aug 23, 2017
theseanl pushed a commit that referenced this issue Aug 28, 2017
…/27 to master

#30
#27
#21 (comment)
#13 (comment)

Squashed commit of the following:

commit e15852da761fecfb44d5b7ebf537bf814f50bbf6
Merge: 89a86ed 686f613
Author: seanl-adg <seanl@adguard.com>
Date:   Sun Aug 27 07:18:33 2017 +0900

    Merge branch 'feature/issues/27' of ssh://bit.adguard.com:7999/extensions/popup-blocker into feature/issues/27

commit 89a86ed4e34d47a8f8433d692657119716772fca
Author: seanl-adg <seanl@adguard.com>
Date:   Sun Aug 27 07:17:26 2017 +0900

    fix

commit 686f6139efd183df86128ec944863ae513440b75
Author: seanl-adg <seanl@adguard.com>
Date:   Sun Aug 27 07:15:19 2017 +0900

    Fix

commit 77ed64086c378892fae44a36895e5992b300cb38
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 17:59:59 2017 +0900

    Workaround for AG bug

commit 3d17f11a8869bb9d66b26967b4f15697f1ca8ac7
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 09:02:37 2017 +0900

    fix

commit e55eae86cd2f7db4e94ee29f16ba2c9a70f80ca7
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 08:30:51 2017 +0900

    fix

commit 3fa5572ec40ac07453c662417b646c91b6319337
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 08:19:41 2017 +0900

    revision

commit 470c5135496a98bf3ca63fbe3a9f4eab6df0855f
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 04:28:45 2017 +0900

    Add more comments

commit ab8b902e17db592c8374c8d0e0f4d8fb2b305c25
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 04:21:03 2017 +0900

    Update test

commit 5dfa698bebb09d1fffbb0b66d887f40f10573c51
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 02:38:02 2017 +0900

    Update declaration

commit e407346d87bb2af6acda3e1a57466446b7846638
Author: seanl-adg <seanl@adguard.com>
Date:   Sat Aug 26 02:33:18 2017 +0900

    revision according to feedback

commit 4bc97bcb5035e6b54a028c76af3db30d08ca97aa
Author: seanl-adg <seanl@adguard.com>
Date:   Fri Aug 25 23:43:53 2017 +0900

    Fix - need to establish messaging channel on empty iframes now

commit 53b3aebfd08e632cd6d5579bf28575959ea8ea4d
Author: seanl-adg <seanl@adguard.com>
Date:   Fri Aug 25 22:51:17 2017 +0900

    Fix test

commit 57e775f17364f9c07361700cb0354f8da5fedc88
Author: seanl-adg <seanl@adguard.com>
Date:   Fri Aug 25 13:47:30 2017 +0900

    workaround for popular frameworks; arrange directory structure

commit 3ff73ee
Author: seanl-adg <seanl@adguard.com>
Date:   Thu Aug 24 20:06:41 2017 +0900

    quick dirty fix

commit 3ee47da
Author: seanl-adg <seanl@adguard.com>
Date:   Tue Aug 22 23:22:04 2017 +0900

    Fix a minor bug, increment version
theseanl pushed a commit that referenced this issue Nov 10, 2017
…le-fix to master

Squashed commit of the following:

commit 827f9c9054316b486b3670ed1b81d75f1ea13c98
Author: seanl-adg <seanl@adguard.com>
Date:   Fri Nov 10 17:47:17 2017 +0900

    Fix tsickle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants