Opened 5 months ago

Closed 5 months ago

Last modified 4 months ago

#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@…, 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/
https://codereview.adblockplus.org/29829615/
https://codereview.adblockplus.org/29830555/

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.
  • 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)

adblockplussafari-1.12.4.2081-unpacked.zip (564.7 KB) - added by kzar 4 months ago.

Download all attachments as: .zip

Change History (34)

comment:1 follow-up: Changed 5 months ago by kzar

Some questions:

  • Should we display the page when the extension starts, and every 24 hours afterwards, or just once the extension starts?
  • 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.
  • Is the URL and are the other details OK?

comment:2 Changed 5 months ago by kzar

  • Description modified (diff)

comment:3 in reply to: ↑ 1 ; follow-ups: Changed 5 months ago 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 5 months ago 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: Changed 5 months ago 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 5 months ago 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: Changed 5 months ago 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 5 months ago 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 5 months ago by kzar

  • Summary changed from Disablay Safari app extension migration page to Display Safari app extension migration page

comment:10 Changed 5 months ago 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: Changed 5 months ago 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 5 months ago by dzhang

  • Cc dzhang added

comment:13 in reply to: ↑ 11 Changed 5 months ago 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: Changed 5 months ago 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 5 months ago by kzar

  • Description modified (diff)
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:16 Changed 5 months ago by sebastian

  • Milestone set to Adblock-Plus-1.12.5-for-Safari

comment:17 Changed 5 months ago by kzar

  • Description modified (diff)
  • Review URL(s) modified (diff)

On second thoughts I've had a go at switching Safari 12+ users to the content blocking mode.

comment:18 Changed 5 months ago by kzar

  • Description modified (diff)

comment:19 Changed 5 months ago by kzar

  • Description modified (diff)

comment:20 Changed 5 months ago by kzar

  • Description modified (diff)

comment:22 Changed 5 months ago by sebastian

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:23 Changed 5 months ago by kzar

  • Cc Ross rscott added

comment:24 Changed 5 months ago by kzar

  • Description modified (diff)

comment:25 Changed 5 months ago by kzar

  • Description modified (diff)

comment:26 Changed 5 months ago by kzar

  • Description modified (diff)

comment:27 Changed 5 months ago by kzar

  • Resolution fixed deleted
  • Review URL(s) modified (diff)
  • Status changed from closed to reopened

comment:28 in reply to: ↑ 14 ; follow-up: Changed 5 months ago 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 5 months ago by abpbot

A commit referencing this issue has landed:
Issue 6786 - Avoid running YouTube code on Safari 12+

comment:30 Changed 5 months ago by kzar

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:31 in reply to: ↑ 28 Changed 5 months ago 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: Changed 5 months ago 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 4 months ago 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.

Note: See TracTickets for help on using tickets.