Closed Bug 989776 Opened 7 years ago Closed 7 years ago

Move inline scripts for SeaMonkey's main about.xhtml page into a separate file

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.28

People

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

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #960566 +++

> With the current plan to harden the security of Firefox, we want to
> disallow internal privileged pages to use inline JavaScript.

The main "about:" is a privileged page too and has a good chunk of JavaScript for controlling the individual strings, so this may be a good starting point to get things rolling.
Attached patch Naive approach (not working) (obsolete) — Splinter Review
This patch simply takes the JavaScript code out of the <script> element and puts it into a new file about.js; this *would* work if there weren't entities like version.appendChild(document.createTextNode("&about.version; " + versionNum)); used which are only resolved within the XHTML context. Here, they are just added verbatim as strings - bummer.

I'm posting this patch anyway to allow easier review of the JavaScript changes.
Attached patch Proposed patch (obsolete) — Splinter Review
This follows to some extent bug 948879 attachment 8349774 [details] [diff] [review] which is the Toolkit counterpart of the about: page. It moves the localizable entities back into the XHTML part while the JavaScript part just appends the values to be displayed.

This differs in a couple of items from the Toolkit patch, though:

 - I didn't like hiding the <script> node at the very end of the list to ensure
   that all the DOM nodes are available before it is executed. Instead, I've
   opted for a proper window.onload function definition, thus allowing the
   <script> node to be moved into the <head> part where it's easily seen.

 - I've used version.textContent += " " + versionNum; to add the version number
   similar to bug 948879 as it was like this before anyway, but retained the
   appendChild() calls for the UA string and the buildID. I'm not exactly sure
   what the consequences are either way for localization, especially for RTL
   locales where string concatenations vs. node order may make a difference.

 - I've also retained the workarounds for bug 319141 which still may or may no
   longer be necessary. They were removed in the Toolkit version of the patch.

The inter-diff relative to attachment 8400399 [details] [diff] [review] should show the actual JavaScript changes made by this patch.
Attachment #8400405 - Flags: review?(neil)
Comment on attachment 8400405 [details] [diff] [review]
Proposed patch

>+  else {
>+    uaItem.setAttribute("hidden", "true");
>+  }

>+  else {
>+    buildItem.setAttribute("hidden", "true");
>+  }

Forgot to mention: This is another difference relative to the Toolkit version. If any of the components isn't available then the respective <li> node is hidden. This visually corresponds to the current behavior where the <li> node isn't created in the first place if the value it's to be filled with is missing.
Comment on attachment 8400405 [details] [diff] [review]
Proposed patch

I was going to reply to your comments but I'll just interleave them in the appropriate places in the review instead.

>+    relnotes.setAttribute("href", releaseNotesURL);
[Not sure why the old code used setAttribute, I'd prefer setting the .href property directly.]

>+  version.textContent += " " + versionNum;
I'd prefer it if you stuck with appending text nodes.

>+    uaItem.setAttribute("hidden", "true");
The "correct" value for the "hidden" attribute is "", although of course you should prefer the .hidden property (which is a boolean).
You may wish to hide the elements in the XHTML and reveal them in the script.

>+      <p id="version">&about.version;</p>
You can add an extra space before the </p> to avoid having to do it in script.

>+    <li id="userAgent">&about.userAgent;</li>
>+    <li id="buildID">&about.buildIdentifier;</li>
What's not obvious here is that you're including "\n    " between the list items in your XHTML document, so you don't need to add it in your script. (In fact your script was adding the whitespace to the end of the list where it was getting ignored.)
Attachment #8400405 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk from comment #4)
> >+    relnotes.setAttribute("href", releaseNotesURL);
> [Not sure why the old code used setAttribute, I'd prefer setting the .href
> property directly.]

Done in both instances.

> >+  version.textContent += " " + versionNum;
> I'd prefer it if you stuck with appending text nodes.

Done with the space added in the XHTML code now.

> >+    uaItem.setAttribute("hidden", "true");
> The "correct" value for the "hidden" attribute is "", although of course you
> should prefer the .hidden property (which is a boolean).
> You may wish to hide the elements in the XHTML and reveal them in the script.

Done - I still think so it's [hidden="true"] the CSS selectors are looking for, thus left it that way in the XHTML part (and it's more self-explanatory).

> >+    <li id="userAgent">&about.userAgent;</li>
> >+    <li id="buildID">&about.buildIdentifier;</li>
> What's not obvious here is that you're including "\n    " between the list
> items in your XHTML document, so you don't need to add it in your script.

I see, I've removed those workarounds.
Attachment #8400399 - Attachment is obsolete: true
Attachment #8400405 - Attachment is obsolete: true
Attachment #8400599 - Flags: review?(neil)
(In reply to rsx11m from comment #5)
> Done - I still think so it's [hidden="true"] the CSS selectors are looking
> for, thus left it that way in the XHTML part

You're thinking of xul.css which does typically require a "true" value on boolean attributes, however boolean attributes in HTML only depend on the presence of the attribute. In particular GetBoolAttr is simply a wrapper around HasAttr, SetBoolAttr sets the attribute to empty or removes it, and MapCommonAttributesInfo again only checks for the presence of the attribute.
Attachment #8400599 - Flags: review?(neil) → review+
You are right, I've looked at the specs which say to just use <foo hidden> to set the attribute, that doesn't work for XHTML, though. I've searched for other occurrences on mozilla-central and see <foo hidden="true"> frequently used, thus I'll stick with it here too as it doesn't appear to be wrong.

Push for comm-central, please.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/e16ffc0bb97c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
(In reply to rsx11m from comment #7)
> You are right, I've looked at the specs which say to just use <foo hidden>
> to set the attribute, that doesn't work for XHTML, though. I've searched for
> other occurrences on mozilla-central and see <foo hidden="true"> frequently
> used, thus I'll stick with it here too as it doesn't appear to be wrong.
> 
> Push for comm-central, please.

IIRC, where HTML uses <ELEMENT ATTRIBUTE>, XHTML specs recommend to use <element attribute="attribute">, repeating the name, inside quotes, as the value. See for instance the first example under http://www.w3.org/TR/xhtml1/#h-4.5
Thanks for the link and the additional information. I'll stick with blah="true" as the boolean "true" equivalent given that the value doesn't seem to matter, this should be most intuitive.
You need to log in before you can comment on or make changes to this bug.