Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new_audit(plugins): avoid plugins #4218

Merged
merged 3 commits into from
Jan 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions lighthouse-cli/test/fixtures/seo/seo-failure-cases.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,25 @@ <h2>Anchor text</h2>
<a href='https://example.com'>click this</a>
<a href='/test.html'> click this </a>
<a href='/test.html'>CLICK THIS</a>

<!-- FAIL(plugins): java applet on page -->
<applet code=HelloWorld.class width="200" height="200" id='applet'>
Your browser does not support the <code>applet</code> tag.
</applet>

<!-- FAIL(plugins): flash content on page -->
<object classid="clsid:d27cdb6e-ae6d-11cf-96b8-444553540000" width="590" height="90" id="adobe-embed" align="middle">
<param name="movie" value="movie_name.swf"/>
<!--[if !IE]>-->
<object type="application/x-shockwave-flash" data="movie_name.swf" width="590" height="90">
<param name="movie" value="movie_name.swf"/>
<!--<![endif]-->
<a href="http://www.adobe.com/go/getflash">
<img src="http://www.adobe.com/images/shared/download_buttons/get_flash_player.gif" alt="Get Adobe Flash player"/>
</a>
<!--[if !IE]>-->
</object>
<!--<![endif]-->
</object>
</body>
</html>
2 changes: 2 additions & 0 deletions lighthouse-cli/test/fixtures/seo/seo-tester.html
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,7 @@ <h2>Small text</h2>
<script class='small'>
// text from SCRIPT tags should be ignored by the font-size audit
</script>
<!-- PASS(plugins): external content does not require java, flash or silverlight -->
<object data="test.pdf" type="application/pdf" width="300" height="200"></object>
</body>
</html>
11 changes: 11 additions & 0 deletions lighthouse-cli/test/smokehouse/seo/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ module.exports = [
'hreflang': {
score: true,
},
'plugins': {
score: true,
},
},
},
{
Expand Down Expand Up @@ -110,6 +113,14 @@ module.exports = [
},
},
},
'plugins': {
score: false,
details: {
items: {
length: 3,
},
},
},
},
},
];
147 changes: 147 additions & 0 deletions lighthouse-core/audits/seo/plugins.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Audit = require('../audit');
const URL = require('../../lib/url-shim');

const JAVA_APPLET_TYPE = 'application/x-java-applet';
const JAVA_BEAN_TYPE = 'application/x-java-bean';
const TYPE_BLOCKLIST = new Set([
'application/x-shockwave-flash',
// See https://docs.oracle.com/cd/E19683-01/816-0378/using_tags/index.html
JAVA_APPLET_TYPE,
JAVA_BEAN_TYPE,
// See https://msdn.microsoft.com/es-es/library/cc265156(v=vs.95).aspx
'application/x-silverlight',
'application/x-silverlight-2',
]);
const FILE_EXTENSION_BLOCKLIST = new Set([
'swf',
'flv',
'class',
'xap',
]);
const SOURCE_PARAMS = new Set([
'code',
'movie',
'source',
'src',
]);

/**
* Verifies if given MIME type matches any known plugin MIME type
* @param {string} type
*/
function isPluginType(type) {
type = type.trim().toLowerCase();

return TYPE_BLOCKLIST.has(type) ||
type.startsWith(JAVA_APPLET_TYPE) || // e.g. "application/x-java-applet;jpi-version=1.4"
type.startsWith(JAVA_BEAN_TYPE);
}

/**
* Verifies if given url points to a file that has a known plugin extension
* @param {string} url
*/
function isPluginURL(url) {
try {
// in order to support relative URLs we need to provied a base URL
const filePath = new URL(url, 'http://example.com').pathname;
const parts = filePath.split('.');

return parts.length > 1 && FILE_EXTENSION_BLOCKLIST.has(parts.pop().trim().toLowerCase());
} catch (e) {
return false;
}
}

class Plugins extends Audit {
/**
* @return {!AuditMeta}
*/
static get meta() {
return {
name: 'plugins',
description: 'Document avoids plugins.',
failureDescription: 'Document uses plugins',
helpText: 'Some types of media or content are not playable on mobile devices. ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Some types of media or content are not playable on mobile devices.'
=>
Most mobile devices do not support plugins, and many desktop browsers restrict them.

'[Learn more](https://developers.google.com/speed/docs/insights/AvoidPlugins).',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This URL is a dead 404 now, and I can't find a true replacement. MFT says this:
image

cached link is https://webcache.googleusercontent.com/search?q=cache:rke54rGwIMcJ:https://developers.google.com/speed/docs/insights/AvoidPlugins+&cd=1&hl=en&ct=clnk&gl=us

also https://developers.google.com/speed/docs/insights/UseLegibleFontSizes is gone, but they set up a redirect to WF.

@rviscomi do you have a preference where this links to? (i cc'd you on the recent CL where these docs were removed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd make a new doc on https://developers.google.com/web/tools/lighthouse/. I'd be ok removing "Learn more" from the help text for now until we have that doc online.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WFM!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked that URL when I opened this PR (a day ago) and it was up ¯\_(ツ)_/¯

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i looked and they had made the change this week. :)

requiredArtifacts: ['EmbeddedContent'],
};
}

/**
* @param {!Artifacts} artifacts
* @return {!AuditResult}
*/
static audit(artifacts) {
const plugins = artifacts.EmbeddedContent
.filter(item => {
if (item.tagName === 'APPLET') {
return true;
}

if (
(item.tagName === 'EMBED' || item.tagName === 'OBJECT') &&
item.type &&
isPluginType(item.type)
) {
return true;
}

const embedSrc = item.src || item.code;
if (item.tagName === 'EMBED' && embedSrc && isPluginURL(embedSrc)) {
return true;
}

if (item.tagName === 'OBJECT' && item.data && isPluginURL(item.data)) {
return true;
}

const failingParams = item.params.filter(param =>
SOURCE_PARAMS.has(param.name.trim().toLowerCase()) && isPluginURL(param.value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a param always guaranteed to have a name and value? if not let's either guard these accesses or throw each item into a try/catch we can recover from or maybe we can just do an || '' in the gatherer

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - these can be nulls. I updated the gatherer.

);

return failingParams.length > 0;
})
.map(plugin => {
const tagName = plugin.tagName.toLowerCase();
const attributes = ['src', 'data', 'code', 'type']
.reduce((result, attr) => {
if (plugin[attr] !== null) {
result += ` ${attr}="${plugin[attr]}"`;
}
return result;
}, '');
const params = plugin.params
.filter(param => SOURCE_PARAMS.has(param.name.trim().toLowerCase()))
.map(param => `<param ${param.name}="${param.value}" />`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically its not
<param ${param.name}="${param.value}" />
but
<param name="${param.name}" value="${param.value}" />

but since we're already simplifying the display of these elements, i don't really think it's too important. ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch! I agree that we are already simplifying, but it might be a bit too much. I'd change it to <param name="${param.name}" value="${param.value}" />

.join('');

return {
source: {
type: 'node',
snippet: `<${tagName}${attributes}>${params}</${tagName}>`,
},
};
});

const headings = [
{key: 'source', itemType: 'code', text: 'Source'},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source => Element source

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: we are already using 'Source' in hreflang and is-crawlable

];

const details = Audit.makeTableDetails(headings, plugins);

return {
rawValue: plugins.length === 0,
details,
};
}
}

module.exports = Plugins;
3 changes: 3 additions & 0 deletions lighthouse-core/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ module.exports = {
'seo/crawlable-links',
'seo/meta-robots',
'seo/hreflang',
'seo/embedded-content',
],
},
{
Expand Down Expand Up @@ -171,6 +172,7 @@ module.exports = {
'seo/link-text',
'seo/is-crawlable',
'seo/hreflang',
'seo/plugins',
'seo/manual/mobile-friendly',
'seo/manual/structured-data',
],
Expand Down Expand Up @@ -386,6 +388,7 @@ module.exports = {
{id: 'is-crawlable', weight: 1, group: 'seo-crawl'},
{id: 'hreflang', weight: 1, group: 'seo-content'},
{id: 'font-size', weight: 1, group: 'seo-mobile'},
{id: 'plugins', weight: 1, group: 'seo-content'},
{id: 'mobile-friendly', weight: 0, group: 'manual-seo-checks'},
{id: 'structured-data', weight: 0, group: 'manual-seo-checks'},
],
Expand Down
41 changes: 41 additions & 0 deletions lighthouse-core/gather/gatherers/seo/embedded-content.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Gatherer = require('../gatherer');
const DOMHelpers = require('../../../lib/dom-helpers.js');

class EmbeddedContent extends Gatherer {
/**
* @param {{driver: !Driver}} options Run options
* @return {!Promise<Array<{tagName: string, type: ?string, src: ?string, data: ?string, code: ?string, params: Array<{name: string, value: string}>}>>} All <object>s, <embed>s and <applet>s with list of relevant attributes and child properties
*/
afterPass(options) {
const expression = `(function() {
${DOMHelpers.getElementsInDocumentFnString}; // define function on page
const selector = 'object, embed, applet';
const elements = getElementsInDocument(selector);
return elements
.map(node => ({
tagName: node.tagName,
type: node.getAttribute('type'),
src: node.getAttribute('src'),
data: node.getAttribute('data'),
code: node.getAttribute('code'),
params: Array.from(node.children)
.filter(el => el.tagName === 'PARAM')
.map(el => ({
name: el.getAttribute('name') || '',
value: el.getAttribute('value') || '',
})),
}));
})()`;

return options.driver.evaluateAsync(expression);
}
}

module.exports = EmbeddedContent;
Loading