545070 - plugin-problem UI shouldn't say "click here"

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

The UI we show for missing/disabled plugins sets the entire plugin box to be clickable, and has "click here" text. That's a little too 1990 style, it would be better to just have specific actions in the text be clickable links, instead of making the whole box an unstyled link.

This WIP does that and has a little cleanup. I seems to be hitting an XPConnect problem between the "UHOH" and "PHEW" when clicking the link to download a plugin...

UHOH
JavaScript error: , line 0: Permission denied for <file://> to call method UnnamedClass.toString on <>.
JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

To clarify only the hyperlink in the message should be a clickable target, making the entire surface a clickable target can lead to unexpected clicks if the the message isn't shown on the page (really big area for the plugin), an the box doesn't visually afford that it can be clicked.

Comment 2

[Reed Loden [:reed]]

This is not Core code.

Comment 3

[Alfred Kayser]

This seems wrong:
:-moz-handler-disabled .msgDisabled1,
:-moz-handler-disabled .msgDisabled2,

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

This gets rid of the "click here" working, hides the in-content UI when it's too small (like crashed plugins do), and lays a bit of groundwork for bug 539832.

Comment 5

[Jonathan Haas]

Why is this patch in review since a half year?

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Updated to trunk.

Just to expand a bit on what this patch is doing (reading top-to-bottom):

* for plugin-not-found <embed>s, set the new "status" attribute to enable display of the "click to install" text. [As noted above, this is the first step towards giving it the smarts to check for something being available _before_ inviting the user to click.]

* moves the notification bar pref checks into the code that's already checking notification bar stuff, for consistency and to avoid the early return that would skip other work (like the "hide in-content UI when too small" thing)

* moves the outdatedNotification check down to the other notification checks (there was a big static data object in between, which made following the code a little awkward)

* updates structure, strings, and style to eliminate whole-plugin "click here to X" in favor of info message + "action noun..." link.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 526176 [details] [diff] [review]
Patch v.3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   handleEvent : function(event) {

>       case "PluginOutdated":
>         self.pluginUnavailable(plugin, event.type);
>         break;
> #ifdef XP_MACOSX
>       case "npapi-carbon-event-model-failure":
>         self.pluginUnavailable(plugin, event.type);
>         break;
> #endif

Seems like you could merge these now.

>+      if (gPrefService.getBoolPref("plugins.hide_infobar_for_carbon_failure_plugin"))
>+        return;

>+      if (gPrefService.getBoolPref("plugins.hide_infobar_for_missing_plugin")) // XXX add a new pref?
>+        return;

>+      if (gPrefService.getBoolPref("plugins.hide_infobar_for_outdated_plugin"))
>+        return;

>+      if (gPrefService.getBoolPref("plugins.hide_infobar_for_missing_plugin"))
>+        return;

I think you still want to do these checks at the top of this function, to avoid doing the getBrowserForDocument/getPluginInfo/getNotificationBox work when you're not going to show a notification anyways. I guess that's annoying since you'd need to duplicate the conditions, though... meh.

>diff --git a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd b/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd

>-<!ENTITY missingPlugin.label                                 "Click here to download plugin.">

(mostly unrelated: missingPlugin.label in plugins.properties looks to be unused, maybe remove it? http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2006-01-03+11%3A44&maxdate=2006-01-03+11%3A44 )

>+<!ENTITY missingPlugin                                       "A plugin is needed to display this content.">
>+<!ENTITY disabledPlugin                                      "This plugin is disabled.">
>+<!ENTITY installPlugin                                       "Install pluginâ¦">
>+<!ENTITY managePlugins                                       "Manage pluginsâ¦">

I wonder whether not using ".label" will mess up some l10n tools. I suppose there are no accesskeys to associate these strings with so I guess it's probably OK.

Do we have any test coverage for this stuff? Or do you just test it all manually?

I tested the missing and blocked cases manually, and noticed that the link doesn't look like a link at all (no underline, no cursor change, etc.). This makes it kind of hard to tell where you need to click. r- because of that, I guess, but this all looks OK otherwise.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to comment #7)

> Seems like you could merge these now.

Fixed.


> (mostly unrelated: missingPlugin.label in plugins.properties looks to be
> unused, maybe remove it?
> http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2006-01-
> 03+11%3A44&maxdate=2006-01-03+11%3A44 )

Fixed.

> Do we have any test coverage for this stuff? Or do you just test it all
> manually?

There are tests for the wizard part of things, some RTL checks, the OS X 32/64 bit thing, and browser_pluginnotification.js checks for correct notification bars. Otherwise it's been mostly manual, I should ponder what kind of automated testing would make sense here...

> I tested the missing and blocked cases manually, and noticed that the link
> doesn't look like a link at all (no underline, no cursor change, etc.). This
> makes it kind of hard to tell where you need to click. r- because of that, I
> guess, but this all looks OK otherwise.

Hmm. Looks fine to me on Windows, and I don't see any obvious reason from the code/style why it would be different on Mac. I'll build and check on OS X to see whats up.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Comment 10

[Justin Dolske [:Dolske]]

Interesting, turns out gavin's issue was from testing with a data: URI, like data:text/html,<embed type="application/x-silverlight">. "Real" pages work fine, and Firefox 4 seems to have the same problem. Problem seems to be that the :-moz-any-link style from ua.css isn't getting applied. I'll file a followup, since that seems to be a content/CSS issue.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Fix a test that was expecting the whole plugin to be clickable.

Comment 12

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/6c3b228838e5

Comment 13

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Windows XP, Windows 7, Ubuntu and Mac OS X 10.6.
After uninstalling the Flash player and disabling the java plug-in, the text "click here" does not appear anymore. Also, the entire plugin box is not clickable all over as it used to be.