Issue 29339192: Issue 3880 - Improve behavior of Safari content blocker option (Closed)
|
Created:
March 31, 2016, 12:12 p.m. by Sebastian Noack Modified:
March 31, 2016, 5:54 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 3880 - Improve behavior of Safari content blocker option
Patch Set 1 #
Total comments: 15
Patch Set 2 : Made CSS comply with coding styles #Patch Set 3 : Don't persists whether Safari needs to be restarted #
MessagesTotal messages: 7
https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... locale/en-US/options.json:160: "message": "Please restart Safari" Detail: Even though this message is shown right after disabling the option it might still come as a surprise for people that we're asking them to restart Safari. Therefore I'd suggest to give a short explanation such as "Please restart Safari to apply the changes". https://codereview.adblockplus.org/29339192/diff/29339193/options.js File options.js (right): https://codereview.adblockplus.org/29339192/diff/29339193/options.js#newcode1100 options.js:1100: E("restart-safari").setAttribute("aria-hidden", !message.args[0]); Why do we even need the background page for that? The options page already gets notified whenever the preference changes so it can do the check itself. https://codereview.adblockplus.org/29339192/diff/29339193/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29339192/diff/29339193/skin/options.css#ne... skin/options.css:623: font-weight: bold; Detail: We're using numeric values for `font-weight` for consistency. https://codereview.adblockplus.org/29339192/diff/29339193/skin/options.css#ne... skin/options.css:625: -webkit-margin-start: 20px; Detail: According to our coding style, box model styles should be placed above colors and typography.
https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... locale/en-US/options.json:160: "message": "Please restart Safari" On 2016/03/31 14:24:57, Thomas Greiner wrote: > Detail: Even though this message is shown right after disabling the option it > might still come as a surprise for people that we're asking them to restart > Safari. > > Therefore I'd suggest to give a short explanation such as "Please restart Safari > to apply the changes". That's not accurrate. If you don't restart Safari (or enable content blockers again), Adblock Plus will essentially remain in a broken state. So the user has to restart Safari, not only to apply the changes. https://codereview.adblockplus.org/29339192/diff/29339193/options.js File options.js (right): https://codereview.adblockplus.org/29339192/diff/29339193/options.js#newcode1100 options.js:1100: E("restart-safari").setAttribute("aria-hidden", !message.args[0]); On 2016/03/31 14:24:57, Thomas Greiner wrote: > Why do we even need the background page for that? The options page already gets > notified whenever the preference changes so it can do the check itself. We should only indicate that restarting Safari is necessary, if Content Blockers got disabled after the extension got loaded. However, without any persistence in the background page we cannot distinguish whether it has been disabled since the last restart, or whether it was already disabled when the extension was loaded. https://codereview.adblockplus.org/29339192/diff/29339193/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29339192/diff/29339193/skin/options.css#ne... skin/options.css:623: font-weight: bold; On 2016/03/31 14:24:57, Thomas Greiner wrote: > Detail: We're using numeric values for `font-weight` for consistency. Not sure, if I agree to that practice. But done. https://codereview.adblockplus.org/29339192/diff/29339193/skin/options.css#ne... skin/options.css:625: -webkit-margin-start: 20px; On 2016/03/31 14:24:57, Thomas Greiner wrote: > Detail: According to our coding style, box model styles should be placed above > colors and typography. Done.
https://codereview.adblockplus.org/29339192/diff/29339193/options.js File options.js (right): https://codereview.adblockplus.org/29339192/diff/29339193/options.js#newcode1100 options.js:1100: E("restart-safari").setAttribute("aria-hidden", !message.args[0]); On 2016/03/31 14:45:37, Sebastian Noack wrote: > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > Why do we even need the background page for that? The options page already > gets > > notified whenever the preference changes so it can do the check itself. > > We should only indicate that restarting Safari is necessary, if Content Blockers > got disabled after the extension got loaded. However, without any persistence in > the background page we cannot distinguish whether it has been disabled since the > last restart, or whether it was already disabled when the extension was loaded. Frankly, I don't have a strong opinion here. Patch set #3 removed the persistent logic. Hence when reloading or opening a new instance of the options page we don't indicate to restart Safari, anymore. I'm not sure which is better. Let me know what you think.
https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... locale/en-US/options.json:160: "message": "Please restart Safari" On 2016/03/31 14:45:37, Sebastian Noack wrote: > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > Detail: Even though this message is shown right after disabling the option it > > might still come as a surprise for people that we're asking them to restart > > Safari. > > > > Therefore I'd suggest to give a short explanation such as "Please restart > Safari > > to apply the changes". > > That's not accurrate. If you don't restart Safari (or enable content blockers > again), Adblock Plus will essentially remain in a broken state. So the user has > to restart Safari, not only to apply the changes. Then what about communicating that to the user somehow? https://codereview.adblockplus.org/29339192/diff/29339193/options.js File options.js (right): https://codereview.adblockplus.org/29339192/diff/29339193/options.js#newcode1100 options.js:1100: E("restart-safari").setAttribute("aria-hidden", !message.args[0]); On 2016/03/31 15:14:31, Sebastian Noack wrote: > On 2016/03/31 14:45:37, Sebastian Noack wrote: > > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > > Why do we even need the background page for that? The options page already > > gets > > > notified whenever the preference changes so it can do the check itself. > > > > We should only indicate that restarting Safari is necessary, if Content > Blockers > > got disabled after the extension got loaded. However, without any persistence > in > > the background page we cannot distinguish whether it has been disabled since > the > > last restart, or whether it was already disabled when the extension was > loaded. > > Frankly, I don't have a strong opinion here. Patch set #3 removed the persistent > logic. Hence when reloading or opening a new instance of the options page we > don't indicate to restart Safari, anymore. I'm not sure which is better. Let me > know what you think. I do agree that we should somehow indicate to the user that Adblock Plus is broken until they restart Safari since that's quite significant. It might be more effective to do that elsewhere though (e.g. in the icon popup?). What do you think? Not sure how common or helpful it might be considering that the user already ignored the message and closed the options page but, in general, I see your point.
https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... locale/en-US/options.json:160: "message": "Please restart Safari" On 2016/03/31 16:29:17, Thomas Greiner wrote: > On 2016/03/31 14:45:37, Sebastian Noack wrote: > > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > > Detail: Even though this message is shown right after disabling the option > it > > > might still come as a surprise for people that we're asking them to restart > > > Safari. > > > > > > Therefore I'd suggest to give a short explanation such as "Please restart > > Safari > > > to apply the changes". > > > > That's not accurrate. If you don't restart Safari (or enable content blockers > > again), Adblock Plus will essentially remain in a broken state. So the user > has > > to restart Safari, not only to apply the changes. > > Then what about communicating that to the user somehow? Well, space it limited here, and nobody reads long texts anyway. IMO, all the user needs to know is that they have to restart Safari. I'm open to suggestions, but I find "Please restart Safari" certainly less confusing then "You just broke Adblock Plus, in order to make it work again restart Safari or enable Content Blockers again". https://codereview.adblockplus.org/29339192/diff/29339193/options.js File options.js (right): https://codereview.adblockplus.org/29339192/diff/29339193/options.js#newcode1100 options.js:1100: E("restart-safari").setAttribute("aria-hidden", !message.args[0]); On 2016/03/31 16:29:17, Thomas Greiner wrote: > On 2016/03/31 15:14:31, Sebastian Noack wrote: > > On 2016/03/31 14:45:37, Sebastian Noack wrote: > > > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > > > Why do we even need the background page for that? The options page already > > > gets > > > > notified whenever the preference changes so it can do the check itself. > > > > > > We should only indicate that restarting Safari is necessary, if Content > > Blockers > > > got disabled after the extension got loaded. However, without any > persistence > > in > > > the background page we cannot distinguish whether it has been disabled since > > the > > > last restart, or whether it was already disabled when the extension was > > loaded. > > > > Frankly, I don't have a strong opinion here. Patch set #3 removed the > persistent > > logic. Hence when reloading or opening a new instance of the options page we > > don't indicate to restart Safari, anymore. I'm not sure which is better. Let > me > > know what you think. > > I do agree that we should somehow indicate to the user that Adblock Plus is > broken until they restart Safari since that's quite significant. It might be > more effective to do that elsewhere though (e.g. in the icon popup?). What do > you think? > > Not sure how common or helpful it might be considering that the user already > ignored the message and closed the options page but, in general, I see your > point. FWIW, I just begun to prefer the new patch set without the persistence, only showing a message to restart Safari once after Content Blockers got disabled, if it's just because it's way simpler. Also, AFAIK, that is what Dave has implemented for the old options page. Please let's not overthink the problem for now. We are talking about an edge case, that is Safari 9 users, that use the options page to disable an experimental feature they have previously turned on. And even those will restart Safari sooner or later anyway, and everything will be fine again. Not to mention that even before the new options page is release, Safari 10 might be released and might not have the old mechanism anyway anymore.
LGTM https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... File locale/en-US/options.json (right): https://codereview.adblockplus.org/29339192/diff/29339193/locale/en-US/option... locale/en-US/options.json:160: "message": "Please restart Safari" On 2016/03/31 17:09:24, Sebastian Noack wrote: > On 2016/03/31 16:29:17, Thomas Greiner wrote: > > On 2016/03/31 14:45:37, Sebastian Noack wrote: > > > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > > > Detail: Even though this message is shown right after disabling the option > > it > > > > might still come as a surprise for people that we're asking them to > restart > > > > Safari. > > > > > > > > Therefore I'd suggest to give a short explanation such as "Please restart > > > Safari > > > > to apply the changes". > > > > > > That's not accurrate. If you don't restart Safari (or enable content > blockers > > > again), Adblock Plus will essentially remain in a broken state. So the user > > has > > > to restart Safari, not only to apply the changes. > > > > Then what about communicating that to the user somehow? > > Well, space it limited here, and nobody reads long texts anyway. IMO, all the > user needs to know is that they have to restart Safari. I'm open to suggestions, > but I find "Please restart Safari" certainly less confusing then "You just broke > Adblock Plus, in order to make it work again restart Safari or enable Content > Blockers again". Indeed it's tricky and I can't think of anything better than that either so fine with me. https://codereview.adblockplus.org/29339192/diff/29339193/options.js File options.js (right): https://codereview.adblockplus.org/29339192/diff/29339193/options.js#newcode1100 options.js:1100: E("restart-safari").setAttribute("aria-hidden", !message.args[0]); On 2016/03/31 17:09:24, Sebastian Noack wrote: > On 2016/03/31 16:29:17, Thomas Greiner wrote: > > On 2016/03/31 15:14:31, Sebastian Noack wrote: > > > On 2016/03/31 14:45:37, Sebastian Noack wrote: > > > > On 2016/03/31 14:24:57, Thomas Greiner wrote: > > > > > Why do we even need the background page for that? The options page > already > > > > gets > > > > > notified whenever the preference changes so it can do the check itself. > > > > > > > > We should only indicate that restarting Safari is necessary, if Content > > > Blockers > > > > got disabled after the extension got loaded. However, without any > > persistence > > > in > > > > the background page we cannot distinguish whether it has been disabled > since > > > the > > > > last restart, or whether it was already disabled when the extension was > > > loaded. > > > > > > Frankly, I don't have a strong opinion here. Patch set #3 removed the > > persistent > > > logic. Hence when reloading or opening a new instance of the options page we > > > don't indicate to restart Safari, anymore. I'm not sure which is better. Let > > me > > > know what you think. > > > > I do agree that we should somehow indicate to the user that Adblock Plus is > > broken until they restart Safari since that's quite significant. It might be > > more effective to do that elsewhere though (e.g. in the icon popup?). What do > > you think? > > > > Not sure how common or helpful it might be considering that the user already > > ignored the message and closed the options page but, in general, I see your > > point. > > FWIW, I just begun to prefer the new patch set without the persistence, only > showing a message to restart Safari once after Content Blockers got disabled, if > it's just because it's way simpler. Also, AFAIK, that is what Dave has > implemented for the old options page. > > Please let's not overthink the problem for now. We are talking about an edge > case, that is Safari 9 users, that use the options page to disable an > experimental feature they have previously turned on. And even those will restart > Safari sooner or later anyway, and everything will be fine again. Not to mention > that even before the new options page is release, Safari 10 might be released > and might not have the old mechanism anyway anymore. You're right. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
