Closed Bug 844098 Opened 9 years ago Closed 7 years ago

Update about:rights content for Safe Browsing based on Toolkit bug 514817 and separate inline scripts

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.29

People

(Reporter: philip.chee, Assigned: rsx11m.pub)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 2 obsolete files)

Bug Bug 477718 implemented Safe Browsing in SeaMonkey. We should update about:rights to reflect this. Toolkit Bug 514817 has some useful strings we can adapt from e.g.:

<!-- safe browsing points for branded builds -->
<!ENTITY rights.safebrowsing-a "SafeBrowsing: ">
<!ENTITY rights.safebrowsing-b "Disabling the Safe Browsing feature is not recommended as it may result in you going to unsafe sites.  If you wish to disable the feature completely, follow these steps:">
<!ENTITY rights.safebrowsing-term1 "Open the application preferences">
<!ENTITY rights.safebrowsing-term2 "Select the Security selection">
<!ENTITY rights.safebrowsing-term3 "Uncheck the options to &quot;&blockAttackSites.label;&quot; and &quot;&blockWebForgeries.label;&quot;">
<!ENTITY rights.safebrowsing-term4 "Safe Browsing is now disabled">
Component: General → UI Design
Whiteboard: [good first bug][lang=xhtml/dtd][mentor=?]
I'll take care of bug 960566 in the process, a small inline script and some event handling has to be moved into a separate aboutRights.js file.
Assignee: nobody → rsx11m.pub
Blocks: 960566
Status: NEW → ASSIGNED
Summary: Update about:rights content for Safe Browsing (Bug 477718) see Toolkit Bug 514817 for example. → Update about:rights content for Safe Browsing based on Toolkit bug 514817 and separate inline scripts
Whiteboard: [good first bug][lang=xhtml/dtd][mentor=?]
Comparing the current Firefox and SeaMonkey versions of about:rights, a couple of design decisions will have to be made:

(1) Firefox has a 2-step opening of the web-services parts, first the general terms and then the disable instructions. I assume that we want to do it the same way in SeaMonkey, or should everything open in a single step?

(2) Firefox has another section for Location Aware Browsing on how to flip geo.enabled; do we want that as well? (I'm tempted to open a new bug to expose that pref in the Privacy & Security pane, in which case the instructions on hacking about:config can be avoided, which shouldn't go into a generic page).
Depends on: 994093
Re (1), I'll go with whatever Firefox has now for the sake of simplicity (and nobody voiced preference for any different approach to be considered).

Re (2), that UI extension is up for review in bug 994093 but depends on bug 903439 being solved, which seems to have stalled. Thus, I'll not defer this bug here further to wait for Geolocation UI to become available but just go with the Safe Browsing additions as initially filed. Once the Location Aware Browsing part is working for SeaMonkey again and enabled for releases it should be straight-forward to just add those few strings in a follow-up bug.
This will also need updates for bug 606482 and bug 639968 on the paragraph handling add-ons, given that the preferences have changed substantially since this was written. Actually, I think I'll convert this into another set of bullet points similar to the Safe Browsing options to make it clearer.

As for Sync, strictly speaking those are web services as well, but the user has to create an account for that and explicitly opt-in using that service, thus I don't think it's on the same level as the opt-out services for add-ons management, Safe Browsing, and (pending) Geolocation services.
(In reply to rsx11m from comment #1)
> I'll take care of bug 960566 in the process, a small inline script and some
> event handling has to be moved into a separate aboutRights.js file.

Hmm, I'm not exactly sure what's going on here. Extracting the JavaScript code into a separate aboutRights.js file and moving the <script> node inside <head> with the respective reference to src="chrome://branding/content/aboutRights.xhtml" throws me a content-policy error without loading the script. I have to remove the UNTRUSTED flag in nsAbout.js#28 to make it work.

The same didn't apply to blockedSite.js and certError.js, and I've double-checked that those are still working. Thus, something must be different for "chrome://branding/content" URIs vs. those in "chrome://communicator/content" with regard to the content policy rules.

Unless anybody has a better idea I'll go with making about:rights a privileged page then.
Attached patch Proposed patch (obsolete) — Splinter Review
Here we go:

 - The logic is the same as in Firefox, opening webservices section first, then
   the "disable" section in a second step.

 - I've completely rewritten the JavaScript logic, moving from "display: none;"
   to using "hidden" attributes and properties instead with event listeners.

 - The page is now privileged to allow aboutRights.js to run.

 - First bullet-point list addresses add-on services, the second Safe Browsing.

 - Like Firefox, I'm using the actual entities as defined in the prefpanes,
   thus inserting the labels without the need to repeat them here.
Attachment #8424387 - Flags: ui-review?(neil)
Attachment #8424387 - Flags: review?(iann_bugzilla)
Comment on attachment 8424387 [details] [diff] [review]
Proposed patch

>-  <p>&rights.webservices-a;<code>&rights.webservices-b;</code>&rights.webservices-c;<code>&rights.webservices-d;</code>&rights.webservices-e;</p>
>+  <p>&rights2.webservices-a;<a href="about:rights#disabling-webservices" id="link-disabling-webservices">&rights2.webservices-b;</a>&rights2.webservices-c;</p>

Forgot to mention: I've removed the <code> markup here and didn't re-introduce it in the bullet list given that it looks a bit odd, would require additional entities, and also Firefox uses the <code> style only to indicate URLs to be entered into the location bar here (about:config) but not for UI labels.
> +var unhideServices = function () {
> +  document.getElementById("webservices-container").hidden = false;

I think we can use the usual form here e.g.
function unhideServices() { .... }

> +var unhideDisablingServices = function () {
> +  document.getElementById("disabling-webservices-container").hidden = false;

Ditto.

> +window.onload = function () {
> +  document.getElementById("link-webservices")
> +          .addEventListener("click", unhideServices);
> +  document.getElementById("link-disabling-webservices")
> +          .addEventListener("click", unhideDisablingServices);

If this is now trusted (!?!) we could perhaps use:

window.addEventListener("load", ... , true/false)
(In reply to Philip Chee from comment #8)
> I think we can use the usual form here e.g.
> function unhideServices() { .... }

How would I pass this as argument to addEventListener()? Or, alternatively, I could pass the one-line function directly as anonymous function as the argument, thus not defining it at all as variable.

> window.addEventListener("load", ... , true/false)

Well, I've used window.onload = function() { ... } in the other "about:xyz" cases as well, regardless of whether or not they were trusted, thus would prefer to keep those consistent.
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
(In reply to rsx11m from comment #9)
> I could pass the one-line function directly as anonymous function as the
> argument, thus not defining it at all as variable.

Yeah, this works and avoids the additional functions without compromising clarity.
Attachment #8424387 - Attachment is obsolete: true
Attachment #8424387 - Flags: ui-review?(neil)
Attachment #8424387 - Flags: review?(iann_bugzilla)
Attachment #8424497 - Flags: ui-review?(neil)
Attachment #8424497 - Flags: review?(iann_bugzilla)
Maybe because communicator is contentaccessible?
Ah, I see - you are correct, as always.  :-)
> https://developer.mozilla.org/en-US/docs/Chrome_Registration#contentaccessible

Try this one, it should work now without making this a privileged page.
Attachment #8424497 - Attachment is obsolete: true
Attachment #8424497 - Flags: ui-review?(neil)
Attachment #8424497 - Flags: review?(iann_bugzilla)
Attachment #8424507 - Flags: ui-review?(neil)
Attachment #8424507 - Flags: review?(iann_bugzilla)
Comment on attachment 8424507 [details] [diff] [review]
Proposed patch (v3)

I'm not enamoured about making branding contentaccessible but I see Firefox has had it accessible for some time so I'll let it pass for now.
Attachment #8424507 - Flags: ui-review?(neil) → ui-review+
I can understand that extensions shouldn't be able to arbitrarily access items in the main manifests, or even across manifests within the application, but that a file in chrome://branding/content isn't allowed to access other files in chrome://branding/content without being privileged is hard to follow.
chrome://branding/content/aboutRights.xhtml would be allowed to, but we're dealing with about:rights here which counts as an untrusted page (unless as in your first patch you flip the flag in nsAbout.js).
Attachment #8424507 - Flags: review?(iann_bugzilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/abcc4ccdf1ed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.29
Blocks: 1016577
(In reply to rsx11m from comment #3)
> Re (2), that UI extension is up for review in bug 994093 but depends on bug
> 903439 being solved, which seems to have stalled. Thus, I'll not defer this
> bug here further to wait for Geolocation UI to become available but just go
> with the Safe Browsing additions as initially filed. Once the Location Aware
> Browsing part is working for SeaMonkey again and enabled for releases it
> should be straight-forward to just add those few strings in a follow-up bug.

Since we are done here, this part is now spun off as bug 1016577.
No longer depends on: 994093
You need to log in before you can comment on or make changes to this bug.