Opened on 07/12/2018 at 10:57:17 AM
Closed on 07/16/2018 at 11:00:17 AM
Last modified on 07/30/2018 at 03:22:41 PM
#6786 closed change (fixed)
Display Safari app extension migration page
Reported by: | kzar | Assignee: | kzar |
---|---|---|---|
Priority: | P2 | Milestone: | Adblock-Plus-1.12.5-for-Safari |
Module: | Platform | Keywords: | |
Cc: | sebastian, mario, l.ursachi@eyeo.com, fhd, CraftyDeano, dzhang, Ross, rscott | Blocked By: | |
Blocking: | Platform: | Safari | |
Ready: | yes | Confidential: | no |
Tester: | Unknown | Verified working: | no |
Review URL(s): |
https://codereview.adblockplus.org/29829611/ |
Description (last modified by kzar)
Background
From Apple's recent announcement it seems that we will need to migrate our Adblock Plus for Safari users over to a Safari App extension as soon as possible:
# Safari Extensions - Deprecated .safariextz-style Safari Extensions - Support for .safariextz-style Safari Extensions installed from the Safari Extensions Gallery is deprecated with Safari 12 on macOS. Submissions to the Safari Extensions Gallery will no longer be accepted after December 2018. Developers are encouraged to transition to Safari App Extensions. - Disabled canLoad Safari Extensions on first launch - Safari Extensions that make use of the canLoad event will be disabled on first launch and users will be notified with a warning that their use is harmful to performance. These extensions can be enabled in Safari Extensions preferences. Developers are encouraged to move to Content Blocker Extensions. - Removed Support for Developer-signed .safariextz Safari Extensions - Support for developer-signed .safariextz Safari Extensions in Safari 12 on macOS has been removed. They no longer appear in Safari preferences and cannot be enabled. On first launch users will receive a warning notification and these extension will not load.
So, we'd like to create a "Please migrate to our Safari App extension" page and have the Safari extension open it when the extension starts. Since that page is not yet ready, we'd also like to first check that the page exists before opening it.
What to change
- For users on Safari 10 and above...
- When the extension starts, perform a request in the background to https://eyeo.to/adblockplus/safari-app-extension-migration.
- If the response is successful open a new tab displaying that page.
- If not, try again in 24 hours.
- When the extension starts, perform a request in the background to https://eyeo.to/adblockplus/safari-app-extension-migration.
- For users on Safari 12 and above...
- Avoid using the old canLoad/beforeLoad APIs at all.
- Enable the content blocking mode automatically.
Hints for testers
- Ensure that for versions of Safari >= 10 that:
- When the extension starts it performs a background request to https://eyeo.to/adblockplus/safari-app-extension-migration.
- If the response to that request is succesful, a new tab is opened with that page.
- Otherwise it retries in 24 hours.
- Ensure that for versions of Safari >= 12 that:
- The user is automatically switched to the content blocking mode, and cannot switch back.
- Safari does not automatically disable Adblock Plus with a warning message about performance.
- Ensure the extension still works on Safari 6.
Attachments (1)
Change History (34)
comment:3 in reply to: ↑ 1 ; follow-ups: ↓ 4 ↓ 5 Changed on 07/12/2018 at 12:42:26 PM by mario
- Should we display the page when the extension starts, and every 24 hours afterwards, or just once the extension starts?
By showing it every 24h we'd catch those users who hardly ever restart Safari – I'm in favor of this approach, even if more intrusive. However if it adds too much complexity I'd be fine with going for the suggestion to show it only after startup.
- Should we care to store a preference that the background request has succeeded in the past? That way we could avoid performing as many requests in the background to the page, but at the cost of adding complexity.
I can't comment on this and would leave it to Sebastian (?) to answer.
- Is the URL and are the other details OK?
Just double-checked with Ops whether there are any new limitations/requirements when it comes to defining the URL of landing pages within adblockplus.org. There aren't any and we're free to use a non-existing URL.
Thus I'm completely fine with the proposed URL. Luiza, are you fine with the URL as well?
I'm also fine with all the other stated details.
Thanks a lot!
comment:4 in reply to: ↑ 3 Changed on 07/12/2018 at 12:54:27 PM by lursachi
Replying to mario:
Thus I'm completely fine with the proposed URL. Luiza, are you fine with the URL as well?
I'm also fine with all the other stated details.
URL LGTM! :-)
Thank you, guys!
comment:5 in reply to: ↑ 3 ; follow-up: ↓ 7 Changed on 07/12/2018 at 01:27:23 PM by sebastian
Replying to mario:
- Should we display the page when the extension starts, and every 24 hours afterwards, or just once the extension starts?
By showing it every 24h we'd catch those users who hardly ever restart Safari – I'm in favor of this approach, even if more intrusive. However if it adds too much complexity I'd be fine with going for the suggestion to show it only after startup.
Well, even if we don't show the message repeatedly during the run time of the extension, it would still show up once the extension gets installed/updated (without requiring a restart of Safari). So if this is the only concern, it's probably not necessary to show the message repeatedly.
- Should we care to store a preference that the background request has succeeded in the past? That way we could avoid performing as many requests in the background to the page, but at the cost of adding complexity.
I can't comment on this and would leave it to Sebastian (?) to answer.
One thing to consider here, if we cache whether the page was online, and we take it down later, Adblock Plus would still attempt to open it showing an error message.
One more thing; we probably should also check for Safari >= 12, and don't register any onbeforeload listener then. Otherwise, as far as I understand, the extension would immediately be disabled.
comment:6 Changed on 07/12/2018 at 02:04:31 PM by mario
Well, even if we don't show the message repeatedly during the run time of the extension, it would still show up once the extension gets installed/updated (without requiring a restart of Safari). So if this is the only concern, it's probably not necessary to show the message repeatedly.
Provided the landing page is up and running before the ABP for Safari update is rolled out. And provided we opt for showing the landing page "as soon as possible" instead of during the last few days of Safari 11's lifetime (e.g. by intentionally holding back publishing the landing page). Or am I misunderstanding you?
comment:7 in reply to: ↑ 5 ; follow-up: ↓ 11 Changed on 07/12/2018 at 02:12:04 PM by kzar
Replying to sebastian:
One more thing; we probably should also check for Safari >= 12, and don't register any onbeforeload listener then. Otherwise, as far as I understand, the extension would immediately be disabled.
I thought that, but now I'm not sure.
Firstly, it would be good to keep this change as simple as possible. Secondly, it might actually be worse for the brand to keep Adblock Plus installed but broken, rather than to just let Apple disable it.
comment:8 Changed on 07/12/2018 at 02:35:45 PM by mario
Requested a redirect (internal ticket system; access limited) for the landing page to be used instead of the final URL as proposed by Ops. Once acknowledged and approved, I'll update the description.
comment:9 Changed on 07/12/2018 at 02:51:45 PM by kzar
- Summary changed from Disablay Safari app extension migration page to Display Safari app extension migration page
comment:10 Changed on 07/12/2018 at 03:12:38 PM by mario
- Description modified (diff)
Requested redirect (internal ticket system; access limited) was approved and is in code review. Thus updating the description to reflect that.
comment:11 in reply to: ↑ 7 ; follow-up: ↓ 13 Changed on 07/12/2018 at 04:17:30 PM by sebastian
Replying to kzar:
Firstly, it would be good to keep this change as simple as possible.
As far as I understand it wouldn't take much to prevent the extension from being disabled (i.e. just disable any code that registers an onbeforeload listener or calls canLoad if on Safari 12).
Secondly, it might actually be worse for the brand to keep Adblock Plus installed but broken, rather than to just let Apple disable it.
Interesting thought, on the other hand they will see a page explaining the situation. Moreover, if we go ahead with the plan here, the update will unlikely be rolled out before Safari 12 comes out anyway, due to the delay by Apple. So if we rather have Adblock Plus disabled, the attempts here are obsolete.
Replying to mario:
Provided the landing page is up and running before the ABP for Safari update is rolled out. And provided we opt for showing the landing page "as soon as possible" instead of during the last few days of Safari 11's lifetime (e.g. by intentionally holding back publishing the landing page). Or am I misunderstanding you?
Right, if the update gets rolled out before the landing page is online (which IMO is unlikely), the page would only show after restarting Safari. But latest when upgrading to Safari 12, they have to restart.
comment:12 Changed on 07/13/2018 at 04:46:21 AM by dzhang
- Cc dzhang added
comment:13 in reply to: ↑ 11 Changed on 07/13/2018 at 03:22:01 PM by kzar
- Owner set to kzar
- Ready set
Replying to sebastian:
...the update will unlikely be rolled out before Safari 12 comes out anyway, due to the delay by Apple.
Yea, if that is the case IMO there is no point crippling the extension in the attempt to avoid being disabled. Users who re-enable the extension will at least have a working one, and it won't make any difference to who sees the migration page.
Either way, since we don't have much time I'm going to make a start on the logic to display the migration page now. Seems like we have enough details for that part at least.
comment:14 follow-up: ↓ 28 Changed on 07/13/2018 at 04:50:49 PM by kzar
Mario I've just noticed that https://eyeo.to/adblockplus/safari-app-extension-migration redirects to https://adblockplus.org. It needs to redirect to a page that doesn't exist, otherwise the window will open.
comment:15 Changed on 07/13/2018 at 05:42:29 PM by kzar
comment:16 Changed on 07/13/2018 at 06:13:26 PM by sebastian
- Milestone set to Adblock-Plus-1.12.5-for-Safari
comment:17 Changed on 07/13/2018 at 06:20:39 PM by kzar
On second thoughts I've had a go at switching Safari 12+ users to the content blocking mode.
comment:18 Changed on 07/13/2018 at 06:26:18 PM by kzar
- Description modified (diff)
comment:19 Changed on 07/13/2018 at 07:17:19 PM by kzar
- Description modified (diff)
comment:20 Changed on 07/13/2018 at 07:41:18 PM by kzar
- Description modified (diff)
comment:21 Changed on 07/13/2018 at 07:48:12 PM by abpbot
Some commits referencing this issue have landed:
comment:22 Changed on 07/13/2018 at 07:53:58 PM by sebastian
- Resolution set to fixed
- Status changed from reviewing to closed
comment:23 Changed on 07/13/2018 at 08:14:08 PM by kzar
- Cc Ross rscott added
comment:24 Changed on 07/13/2018 at 08:28:24 PM by kzar
- Description modified (diff)
comment:25 Changed on 07/13/2018 at 08:31:34 PM by kzar
- Description modified (diff)
comment:26 Changed on 07/13/2018 at 08:37:06 PM by kzar
- Description modified (diff)
comment:27 Changed on 07/15/2018 at 11:28:45 AM by kzar
- Resolution fixed deleted
- Review URL(s) modified (diff)
- Status changed from closed to reopened
comment:28 in reply to: ↑ 14 ; follow-up: ↓ 31 Changed on 07/16/2018 at 10:38:31 AM by mario
Replying to kzar:
Mario I've just noticed that https://eyeo.to/adblockplus/safari-app-extension-migration redirects to https://adblockplus.org. It needs to redirect to a page that doesn't exist, otherwise the window will open.
Sorry, I didn't see your comment on Friday before I left. In the meantime this has moved forward and it's been addressed internally already, but I just wanted to state here as well, that the redirect is not yet fully set up and will work as intended once this code review is done and the change landed.
comment:29 Changed on 07/16/2018 at 10:59:06 AM by abpbot
A commit referencing this issue has landed:
Issue 6786 - Avoid running YouTube code on Safari 12+
comment:30 Changed on 07/16/2018 at 11:00:17 AM by kzar
- Resolution set to fixed
- Status changed from reopened to closed
comment:31 in reply to: ↑ 28 Changed on 07/16/2018 at 01:08:56 PM by kzar
Replying to mario:
Sorry, I didn't see your comment on Friday before I left.
That's OK, I didn't expect you to reply right away!
...the redirect is not yet fully set up and will work as intended once this code review is done and the change landed.
Ace, thanks. It seems to work now.
comment:32 follow-up: ↓ 33 Changed on 07/28/2018 at 02:45:03 AM by traynard
- Verified working set
Couldnt verify Safari 12 as I was unable to get a VM with it and downloading the dev version wouldnt allow me to install extensions. I will verify this once I can get access to Safari 12.
comment:33 in reply to: ↑ 32 Changed on 07/30/2018 at 03:22:41 PM by kzar
- Verified working unset
Replying to traynard:
Couldnt verify Safari 12 as I was unable to get a VM with it and downloading the dev version wouldnt allow me to install extensions.
I see what you mean, Safari 12 doesn't seem to allow you to install the development build of our extension any more. I think you'll have to test by using the Extension Builder tool (click Develop, then Extension builder, then +, then select the directory containing the unpacked extension).
There are some instructions on building the extension in the README, but it's a little tricky. I know Ross has experience doing it, so perhaps he can help. Alternatively I've built you a (untested!) copy, zipped it and attached that to this issue. If you extract the zip it should give you a working copy of the unpacked extension.
I will verify this once I can get access to Safari 12.
I don't think there is a problem with your VM, or copy of Safari 12.
Some questions: