Closed Bug 545070 Opened 15 years ago Closed 14 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla6

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch v.1 (WIP) (obsolete) (deleted) — Splinter Review
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
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
This seems wrong: :-moz-handler-disabled .msgDisabled1, :-moz-handler-disabled .msgDisabled2,
Blocks: 535078
Depends on: 538910
Attached patch Patch v.2 (obsolete) (deleted) — Splinter Review
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)
Why is this patch in review since a half year?
Attached patch Patch v.3 (obsolete) (deleted) — Splinter Review
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-
(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.
Attached patch Patch v.4 (obsolete) (deleted) — Splinter Review
Attachment #526176 - Attachment is obsolete: true
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.
Attached patch Patch v.5 (deleted) — Splinter Review
Fix a test that was expecting the whole plugin to be clickable.
Attachment #533105 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 14 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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: