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

VERIFIED FIXED in mozilla6

Status

()

Core
Plug-ins
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
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)
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.
Component: Preferences: Backend → General
Product: Core → Firefox
QA Contact: preferences-backend → general
(Assignee)

Updated

8 years ago
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
This is not Core code.
Component: Plug-ins → Plugin Finder Service
Product: Core → Toolkit
QA Contact: plugins → plugin.finder
Assignee: dolske → nobody
Component: Plugin Finder Service → Plug-ins
Product: Toolkit → Core
QA Contact: plugin.finder → plugins

Comment 3

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

Updated

8 years ago
Blocks: 535078
Depends on: 538910
(Assignee)

Comment 4

7 years ago
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.
Assignee: nobody → dolske
Attachment #425926 - Attachment is obsolete: true
Attachment #448312 - Flags: review?(gavin.sharp)

Comment 5

7 years ago
Why is this patch in review since a half year?
(Assignee)

Comment 6

6 years ago
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.
Attachment #448312 - Attachment is obsolete: true
Attachment #526176 - Flags: review?(gavin.sharp)
Attachment #448312 - Flags: review?(gavin.sharp)
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.
Attachment #526176 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 533105 [details] [diff] [review]
Patch v.4
Attachment #526176 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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.
(Assignee)

Comment 11

6 years ago
Created attachment 534160 [details] [diff] [review]
Patch v.5

Fix a test that was expecting the whole plugin to be clickable.
Attachment #533105 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/6c3b228838e5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
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.
Status: RESOLVED → VERIFIED
Blocks: 667201
Blocks: 724499
You need to log in before you can comment on or make changes to this bug.