new_audit(plugins): avoid plugins #4218

kdzwinel · 2018-01-09T17:01:19Z

Success:

Failure:

As I found out, world of embedding external content is very messy. There are many non-standard ways of embedding that still work (multiplied by the number of different browsers 😵).

My approach here was to focus on documented ways of embedding (docs from adobe, ms, oracle, mdn) and real-life uses on popular sites (video, games, showcases). It should cover most of the real-life uses, but I'm pretty sure there are some false positives and false negatives.

Some notes:

it's common to do feature detection when embedding such content (esp. for flash with swfobject.js, but there is also silverlight.js and some code from oracle doing the same for java). We won't be able to detect that unless user has these plugins enabled.
flash is often embedded as <object …><embed …></object>. I'm listing both the object and embed as separate entries on the details list as they potentially can have different contents.

patrickhulce

nicely done! very thorough as always 😄

patrickhulce · 2018-01-09T17:10:05Z

lighthouse-core/audits/seo/plugins.js

+const JAVA_BEAN_TYPE = 'application/x-java-bean';
+const TYPE_BLOCKLIST = new Set([
+  'application/x-shockwave-flash',
+  // https://docs.oracle.com/cd/E19683-01/816-0378/using_tags/index.html


nit: at a quick scan I thought these were part of the blocklist, maybe throw a "// See ..." in front of there?

patrickhulce · 2018-01-09T17:13:10Z

lighthouse-core/audits/seo/plugins.js

+ */
+function isPluginURL(url) {
+  try {
+    const filePath = new URL(url, 'http://example.com').pathname;


nit: throw a comment here that we need example.com to support relative URLs, almost looks like a bug at first :)

patrickhulce · 2018-01-09T17:16:35Z

lighthouse-core/audits/seo/plugins.js

+      category: 'Content Best Practices',
+      name: 'plugins',
+      description: 'Document avoids plugins.',
+      failureDescription: 'Document doesn\'t avoid plugins',


the double negative gets a bit wonky, wdyt about Document uses plugins I only found one other case of the "doesn't avoid" we should fix

the overall pattern we favor elsewhere seems to be just Avoids plugins and Uses plugins

Agreed, and since Rick also OKed it, I changed it to Document uses plugins as suggested.

patrickhulce · 2018-01-09T17:18:23Z

lighthouse-core/audits/seo/plugins.js

+        }
+
+        const failingParams = item.params.filter(param =>
+          SOURCE_PARAMS.has(param.name.trim().toLowerCase()) && isPluginURL(param.value)


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

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

patrickhulce · 2018-01-09T17:25:51Z

lighthouse-core/gather/gatherers/seo/external-content.js

+  }
+}
+
+module.exports = ExternalContent;


🚲 🏠 I'm not 100% sold on ExternalContent, wdyt about EmbeddedContent?

MDN seems like it goes back and forth but titles it "Multimedia and embedding" https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Other_embedding_technologies

kdzwinel

@patrickhulce All feedback addressed! Thank you for a super-quick response on this ⚡️

kdzwinel · 2018-01-09T21:03:30Z

lighthouse-core/audits/seo/plugins.js

+      category: 'Content Best Practices',
+      name: 'plugins',
+      description: 'Document avoids plugins.',
+      failureDescription: 'Document doesn\'t avoid plugins',


Agreed, and since Rick also OKed it, I changed it to Document uses plugins as suggested.

kdzwinel · 2018-01-09T21:04:33Z

lighthouse-core/audits/seo/plugins.js

+        }
+
+        const failingParams = item.params.filter(param =>
+          SOURCE_PARAMS.has(param.name.trim().toLowerCase()) && isPluginURL(param.value)


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

patrickhulce

looks a-ok by me! over to @paulirish if his knowledge on plugins is much better than mine 😄

patrickhulce · 2018-01-10T14:32:42Z

lighthouse-core/audits/seo/plugins.js

+   */
+  static get meta() {
+    return {
+      category: 'Content Best Practices',


I think we've fully nixed category at this point, so should be safe to remove :)

paulirish · 2018-01-10T18:48:03Z

lighthouse-core/audits/seo/plugins.js

+          }, '');
+        const params = plugin.params
+          .filter(param => SOURCE_PARAMS.has(param.name.trim().toLowerCase()))
+          .map(param => `<param ${param.name}="${param.value}" />`)


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. ;)

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}" />

paulirish · 2018-01-10T18:53:41Z

lighthouse-core/audits/seo/plugins.js

+      description: 'Document avoids plugins.',
+      failureDescription: 'Document uses plugins',
+      helpText: 'Some types of media or content are not playable on mobile devices. ' +
+      '[Learn more](https://developers.google.com/speed/docs/insights/AvoidPlugins).',


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

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)

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.

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

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

paulirish · 2018-01-10T18:59:02Z

lighthouse-core/audits/seo/plugins.js

+      name: 'plugins',
+      description: 'Document avoids plugins.',
+      failureDescription: 'Document uses plugins',
+      helpText: 'Some types of media or content are not playable on mobile devices. ' +


'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.

paulirish · 2018-01-10T19:01:02Z

lighthouse-core/audits/seo/plugins.js

+      });
+
+    const headings = [
+      {key: 'source', itemType: 'code', text: 'Source'},


Source => Element source

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

paulirish

LGTM. I'll address my comments myself. ;)

Initial commit.

3aaddde

devtools-bot added the waiting4reviewer label Jan 9, 2018

patrickhulce reviewed Jan 9, 2018

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 9, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Jan 9, 2018

Address feedback.

4ca3233

kdzwinel commented Jan 9, 2018

View reviewed changes

patrickhulce approved these changes Jan 10, 2018

View reviewed changes

Remove category field from audit metadata.

f94b048

paulirish requested changes Jan 10, 2018

View reviewed changes

paulirish approved these changes Jan 10, 2018

View reviewed changes

paulirish merged commit 8436c03 into GoogleChrome:master Jan 10, 2018

paulirish removed the waiting4reviewer label Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit(plugins): avoid plugins #4218

new_audit(plugins): avoid plugins #4218

kdzwinel commented Jan 9, 2018 •

edited

Loading

patrickhulce left a comment

patrickhulce Jan 9, 2018

patrickhulce Jan 9, 2018

patrickhulce Jan 9, 2018

kdzwinel Jan 9, 2018

patrickhulce Jan 9, 2018

kdzwinel Jan 9, 2018

patrickhulce Jan 9, 2018

kdzwinel left a comment

kdzwinel Jan 9, 2018

kdzwinel Jan 9, 2018

patrickhulce left a comment

patrickhulce Jan 10, 2018

paulirish Jan 10, 2018

kdzwinel Jan 10, 2018

paulirish Jan 10, 2018

rviscomi Jan 10, 2018

paulirish Jan 10, 2018

kdzwinel Jan 10, 2018

paulirish Jan 10, 2018

paulirish Jan 10, 2018

paulirish Jan 10, 2018

kdzwinel Jan 10, 2018

paulirish left a comment

new_audit(plugins): avoid plugins #4218

new_audit(plugins): avoid plugins #4218

Conversation

kdzwinel commented Jan 9, 2018 • edited Loading

patrickhulce left a 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

kdzwinel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

kdzwinel commented Jan 9, 2018 •

edited

Loading