Last Comment Bug 545070 - plugin-problem UI shouldn't say "click here"
: plugin-problem UI shouldn't say "click here"
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Justin Dolske [:Dolske]
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 538910
Blocks: 535078 667201 724499
  Show dependency treegraph
 
Reported: 2010-02-08 20:19 PST by Justin Dolske [:Dolske]
Modified: 2012-02-06 12:21 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (WIP) (22.59 KB, patch)
2010-02-08 20:19 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (14.68 KB, patch)
2010-05-30 17:56 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.3 (22.77 KB, patch)
2011-04-14 20:27 PDT, Justin Dolske [:Dolske]
gavin.sharp: review-
Details | Diff | Splinter Review
Patch v.4 (24.45 KB, patch)
2011-05-17 16:06 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.5 (26.33 KB, patch)
2011-05-20 17:14 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2010-02-08 20:19:16 PST
Created attachment 425926 [details] [diff] [review]
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) 2010-02-09 17:22:39 PST
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] (use needinfo?) 2010-02-10 19:29:04 PST
This is not Core code.
Comment 3 Alfred Kayser 2010-02-11 05:39:32 PST
This seems wrong:
:-moz-handler-disabled .msgDisabled1,
:-moz-handler-disabled .msgDisabled2,
Comment 4 Justin Dolske [:Dolske] 2010-05-30 17:56:13 PDT
Created attachment 448312 [details] [diff] [review]
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 2011-01-30 13:54:37 PST
Why is this patch in review since a half year?
Comment 6 Justin Dolske [:Dolske] 2011-04-14 20:27:09 PDT
Created attachment 526176 [details] [diff] [review]
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: gavin@gavinsharp.com] 2011-05-06 12:21:57 PDT
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] 2011-05-17 16:05:34 PDT
(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] 2011-05-17 16:06:04 PDT
Created attachment 533105 [details] [diff] [review]
Patch v.4
Comment 10 Justin Dolske [:Dolske] 2011-05-18 16:46:19 PDT
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] 2011-05-20 17:14:41 PDT
Created attachment 534160 [details] [diff] [review]
Patch v.5

Fix a test that was expecting the whole plugin to be clickable.
Comment 12 Justin Dolske [:Dolske] 2011-05-23 13:46:03 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/6c3b228838e5
Comment 13 Simona B [:simonab ] 2011-07-27 08:49:24 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.