[bug 1307567] Remove inline styles from legacy pages #4406

alexgibson · 2016-10-11T10:33:23Z

Description

Removes a bunch of inline styles from legacy pages.
The only thing that should be left after this PR in terms of inline styles are those contained in SVG files.
There are a couple of pages that we're loading web fonts from fonts.googleapis.com. As it stands I haven't found replacements for these fonts, just removed references to them (they're being blocked by CSP already). I'm open to suggestions for what we should do here, should we house the fonts on mozorg, find suitable system fallback fonts, or do nothing?

Update: It looks like both Crimson Text and Lora web font's both have SIL OFL licenses, so we can't redistribute them. I don't think we should add fonts.googleapis.com to the CSP whitelist.

alexgibson · 2016-10-11T10:34:57Z

bedrock/firefox/templates/firefox/geolocation.html

-    &ndash; and is done in a way that totally respects your privacy.
-    <a href="{{ url }}" id="{{ id }}" style="{{ style }}">Give it a try!</a>
-  {% endtrans %}
+    {% if l10n_has_tag('remove-inline-styles') %}


@flodolo - this is a very old page to which I imagine traffic is pretty low. How bad would it be to break a string here if we we're to remove the l10n tag?

The only change is the removal of style="{{ style }}", right? I think I can script that, without using a new l10n tag.

Correct, thanks @flodolo 👍

craigcook · 2016-10-21T21:27:57Z

bedrock/newsletter/templates/newsletter/hacks.mozilla.org.html

@@ -6,80 +6,12 @@
 <html>


I don't think this page is used at all now? At one time it was shown in an iframe on the Hacks blog but they don't have it in the new design (and I'm not sure this newsletter is still alive at all). I think we can probably just delete this (including the related JS/CSS bits).

Ah I had no idea this wasn't in use any longer, will remove it 👍

craigcook · 2016-10-21T21:37:28Z

bedrock/privacy/templates/privacy/archive/firefox-2008-06.html

@@ -37,8 +37,8 @@ <h1 itemprop="name">Mozilla Firefox Privacy Policy</h1>
        <ul class="spaced">
          <li><em>Crash-Reporting Feature</em>. Firefox has a crash-reporting feature that sends a report to Mozilla when Firefox crashes. Mozilla uses the information in the crash reports to diagnose and correct the problems in Firefox that caused the crash. Though this feature starts automatically after Firefox crashes, it does not send information to Mozilla until you explicitly authorize it to do so. By default, this feature sends a variety of Non-Personal Information to Mozilla, including the stack trace (a detailed description of which parts of the Firefox code were active at the time of the crash) and the type of computer you are using. Additional information is collected by the crash reporting feature. Which crash reporting feature is used and additional information collected by Firefox depends on which version of Firefox you’re using.
            <dl>
-              <dd><span style="text-decoration: underline;">Firefox 1.0 – 2.x</span>. For these earlier versions of Firefox, “Talkback” is Firefox’s crash reporting feature. Talkback also collects Personal Information (including your name, email address) and Potentially Personal Information (including your IP address, your computer’s name, and the processes you were running at the time of the crash). You can selectively disable the sending of this information. Additionally, you have the option to include the URL of the site you were visiting when Firefox crashed, a comment, and your email address in the report. Mozilla only makes Non-Personal Information and Potentially Personal Information in the public reports available online at <a href="http://www.talkback-public.mozilla.org/">www.talkback-public.mozilla.org/</a>.</dd>
-              <dd><span style="text-decoration: underline;">Firefox 3.0 to 3.x</span>. For the current versions of Firefox, the Firefox Crash Reporter is Firefox’s crash reporting feature. With this feature, you have the option to include the URL of the site you were visiting when Firefox crashed, a comment, and your email address in the report. Firefox Crash Reporter also sends Potentially Personal Information to Mozilla in the form of a unique numeric value to distinguish individual Firefox installs. Mozilla only makes Non-Personal Information and Potentially Personal Information in the public reports available online at <a href="http://crash-stats.mozilla.com/">http://crash-stats.mozilla.com/</a>.</dd>
+              <dd><span class="underline">Firefox 1.0 – 2.x</span>. For these earlier versions of Firefox, “Talkback” is Firefox’s crash reporting feature. Talkback also collects Personal Information (including your name, email address) and Potentially Personal Information (including your IP address, your computer’s name, and the processes you were running at the time of the crash). You can selectively disable the sending of this information. Additionally, you have the option to include the URL of the site you were visiting when Firefox crashed, a comment, and your email address in the report. Mozilla only makes Non-Personal Information and Potentially Personal Information in the public reports available online at <a href="http://www.talkback-public.mozilla.org/">www.talkback-public.mozilla.org/</a>.</dd>


Ouch 😬 I wonder if the underline is really essential? If the point is just to set this off visually but it doesn't specifically have to be underlined, then <b> or <i> would suffice here.

Also, the <u> element is back in HTML5 (after being removed in XHTML) but now it's meant for annotations and mispellings, not for emphasis. So visually a <u> would have the same effect but it's probably not semantically correct.

craigcook · 2016-10-21T21:44:26Z

bedrock/security/templates/security/older-vulnerabilities.html

@@ -715,7 +715,7 @@ <h3 id="nov-2003">November 2003 Update</h3>
      <td valign="top">60</td>
      <td valign="top">Networking<br>
      </td>
-      <td style="vertical-align: top;">1.5 1.4.2</td>
+      <td valign="top">1.5 1.4.2</td>


The alignment on these table cells is inconsistent throughout the page. I'd vote for removing the valigns everywhere rather than replacing these random inline styles. Maybe just set td { vertical-align: top; } for the whole table?

craigcook · 2016-10-21T22:01:31Z

media/css/mozorg/mpl-2-0.scss

+
+@import '../pebbles/includes/lib';
+
+body {


I know this was all just copied over but there's some silly/redundant styling here. Is it worth tidying it up or should we just leave it alone? I might even be inclined to remove the CSS entirely and just let browser defaults handle it.

craigcook · 2016-10-21T22:11:21Z

bedrock/firefox/templates/firefox/geolocation.html

    Firefox can tell websites where you’re located so you can find info
    that’s more relevant and more useful. It’s about making the Web smarter
    &ndash; and is done in a way that totally respects your privacy.
-    <a href="{{ url }}" id="{{ id }}" style="{{ style }}">Give it a try!</a>
+    <a href="{{ url }}" id="{{ id }}">Give it a try!</a>
  {% endtrans %}
  </p>
 </div>


Just noticed the demo popup at the bottom of this page has style="display:none", we can replace that with class="hidden"

Nice catch!

craigcook · 2016-10-21T22:16:19Z

Looks good, just a few minor nitpicks. It's tempting to overhaul some of that ancient code but perhaps that can of worms should remain closed for now. The only thing that really needs fixing is that last inline style on the geolocation page. r+wc 🍗

alexgibson · 2016-10-22T17:16:01Z

@craigcook I started down the route of trying to fix up old CSS and improve the markup, but quickly came to realise it would be considerably more work than was highlighted in this diff. I think just concentrating on fixing the bug at hand is probably the best course of action for now. As these pages are very old I don't think it's worth spending much time trying to fix things, unless there is something obviously broken. We could always file some good first bugs for contributors to take on (kind of a wontfix, but patches welcome kind of thing).

alexgibson · 2016-10-24T08:36:02Z

@craigcook updated

alexgibson commented Oct 11, 2016

View reviewed changes

alexgibson force-pushed the remove-inline-styles branch 2 times, most recently from ff04c10 to e2413ed Compare October 11, 2016 10:44

flodolo added the L10N label Oct 11, 2016

alexgibson added the Durable Team Sprint label Oct 11, 2016

alexgibson force-pushed the remove-inline-styles branch 2 times, most recently from 960daf8 to 5231cc4 Compare October 11, 2016 14:15

alexgibson changed the title ~~Remove inline styles from legacy pages~~ [bug 1307567] Remove inline styles from legacy pages Oct 11, 2016

alexgibson force-pushed the remove-inline-styles branch from 5231cc4 to 0f47627 Compare October 12, 2016 10:26

Bug 1307567 - Remove inline styles from legacy pages

e2ec19c

alexgibson force-pushed the remove-inline-styles branch from 0f47627 to e2ec19c Compare October 14, 2016 15:19

craigcook self-assigned this Oct 20, 2016

alexgibson mentioned this pull request Oct 21, 2016

Remove inline JavaScript from templates #4429

Merged

2 tasks

craigcook requested changes Oct 21, 2016

View reviewed changes

craigcook reviewed Oct 21, 2016

View reviewed changes

Review fixes

f3a1c9b

alexgibson force-pushed the remove-inline-styles branch from cedeefc to f3a1c9b Compare October 24, 2016 09:17

craigcook approved these changes Oct 24, 2016

View reviewed changes

craigcook merged commit a4a75ce into mozilla:master Oct 24, 2016

alexgibson deleted the remove-inline-styles branch October 24, 2016 15:40

flodolo mentioned this pull request Oct 24, 2016

firefox/geolocation.lang: remove inline style from existing string mozilla-l10n/www.mozilla.org#114

Merged

jpetto mentioned this pull request Oct 31, 2016

[fix bug 1313508] Update /firefox/developer/ newsletter. #4444

Merged

1 task

flodolo removed the L10N label Nov 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug 1307567] Remove inline styles from legacy pages #4406

[bug 1307567] Remove inline styles from legacy pages #4406

alexgibson commented Oct 11, 2016 •

edited

alexgibson Oct 11, 2016

flodolo Oct 11, 2016

alexgibson Oct 11, 2016

craigcook Oct 21, 2016

alexgibson Oct 22, 2016

craigcook Oct 21, 2016

craigcook Oct 21, 2016

craigcook Oct 21, 2016

craigcook Oct 21, 2016

craigcook Oct 21, 2016

alexgibson Oct 22, 2016

craigcook commented Oct 21, 2016

alexgibson commented Oct 22, 2016

alexgibson commented Oct 24, 2016


		@import '../pebbles/includes/lib';

		body {

[bug 1307567] Remove inline styles from legacy pages #4406

[bug 1307567] Remove inline styles from legacy pages #4406

Conversation

alexgibson commented Oct 11, 2016 • edited

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigcook commented Oct 21, 2016

alexgibson commented Oct 22, 2016

alexgibson commented Oct 24, 2016

alexgibson commented Oct 11, 2016 •

edited