539848 - Make in-content plugin problem UI look better

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

For ages we've had in-content UI that's shown when a plugin is not found (and more recently for a blocked/disabled plugin). Bug 538910 is adding similar UI for when a plugin has crashed, and given the frequency of which plugins crash users are going to be seeing this a lot more often.

The current UI is kind of ugly... 1990 style outset border, funky spacing of icon and text, etc. With a little sprucing up this would look a lot better.

Alex made some initial suggestions -- lose the border, use a semitransparent background with rounded borders, increase the icon size.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Screenshot: Old vs. Patch v.1

Icon's fuzzy, because it's being scaled up, but this is the general idea.

Comment 2

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

How about "_Download missing plugin_" as a hyperlink instead of "click here"  The current text kind of feels like early 1990s web design when people weren't used to writing in hypertext yet.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (WIP)

WIP, Windows-only, updated to reasonably match Steven's (now slightly-outdated) mockups eg http://www.stephenhorlander.com/images/mockups/plugin-state-glyphs/Missing-Plugin-Glyph-%28Blocked%29-v01.png

I think we definitely need to improve the visual UI like this for Lorentz -- people are going to see this UI a lot for crashed plugins (whereas the missing/disabled/blocked flavors are not commonly seen by most people), and the current styling is fairly ugly.

I was concerned that we might not be able to manage this for Lorentz due to concerns with 3rd party themes (which would make the updated UI effectively look unstyled), but we should be able to do this change by moving the style/images out of the theme (/skin) and into /content. Existing themes would simply get the same styling as the default theme, and I *think* themes updated for Lorentz can change things if they really want to. [Of course, it will be themable-as-usual on trunk].

Comment 4

[Justin Dolske [:Dolske]]

(I still need to done some further fixup to replace some of the click handler stuff I removed from the bindings.)

(In reply to comment #2)
> How about "_Download missing plugin_" as a hyperlink instead of "click here" 
> The current text kind of feels like early 1990s web design when people weren't
> used to writing in hypertext yet.

We want to minimize string changes for Lorentz, so I'm not going to change any existing strings until a future trunk-only followup patch. But I'll make the plugin-crashed UI work like this (WIP patches in bug 538910 already are).

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Cleaned up patch, renamed "missingPlugin.*" to "pluginProblem.*" since this UI hasn't been used just for missing plugins for a long time.

I don't really understand the purpose of the <handler>s that I removed from the original XBL. The events they were sending are already generated (as trusted events) in nsObjectLoadingContent.cpp's nsPluginErrorEvent::Run(). Code in browser.js already listens for then and sets up click handlers. So I'm not sure what the point of the XBL code is -- deadwood that just never got removed?

It also seems that the existing code is already broken on trunk. Even with a stock nightly build, I'm unable to get the disabled-plugin UI to come up. Maybe I'm just doing it wrong? I just get empty white boxes where the element should be, and poking at it with DOMi seems to make the browser hang. O_o

Comment 6

[Alfred Kayser]

Good thing to rename the .css file, as the content dramatically changed (for the better!). This makes it easier for us themers to keep compatibility.

Comment 7

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

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

>diff --git a/toolkit/mozapps/plugins/content/missingPlugin.xml b/toolkit/mozapps/plugins/content/pluginProblem.xml

>+<binding id="pluginProblem" inheritstyle="false">

>-      <!-- This uses html:a instead of something like a div so that it can be
>-           tabbed to and just generically behaves more like something clickable
>-           (i.e. for a11y reasons. see Bug 245349).
>-        -->
>-      <html:a href="#">

I think just removing this breaks screenreader access, as discussed on IRC. Maybe we can use ARIA attributes instead?

>+        <xul:vbox class="mainBox" flex="1">
>+            <xul:spacer flex="1"/>
...
>+            <xul:spacer flex="1"/>
>         </xul:vbox>

Use pack="center" on the vbox instead of the spacers?

>-    <handlers>
>-      <handler event="click" button="0">

Looks like these are only useful if the binding is somehow attached without the relevant events being fired from nsObjectLoadingContent, which doesn't appear to be possible.

>diff --git a/toolkit/themes/pinstripe/mozapps/plugins/missingPlugin.css b/toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css

>-/*
>- * This file's style only applies to broken objects, not the rest
>- * of the page.
>- */

Why remove this comment? Still seems useful (comment applies to both files).

r- for the a11y issue but looks fine once that's solved.

Comment 8

[Justin Dolske [:Dolske]]

(In reply to comment #7)

> >+        <xul:vbox class="mainBox" flex="1">
> >+            <xul:spacer flex="1"/>
> ...
> >+            <xul:spacer flex="1"/>
> >         </xul:vbox>
> 
> Use pack="center" on the vbox instead of the spacers?

Bug 538910 is adding a string at the bottom of the box, so 2 spacers seemed the easiest way to prepare for that.

> >-/*
> >- * This file's style only applies to broken objects, not the rest
> >- * of the page.
> >- */
> 
> Why remove this comment? Still seems useful (comment applies to both files).

It seems fairly common to include a CSS file for a binding, but this is the only case I remember seeing that's had such a notice. [So you pause, read it, think about it, and realize it's not telling you anything new.]

Comment 9

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

(In reply to comment #8)
> It seems fairly common to include a CSS file for a binding, but this is the
> only case I remember seeing that's had such a notice. [So you pause, read it,
> think about it, and realize it's not telling you anything new.]

It's not that common that we bind XBL to stuff in content, though, and the fact that there are "html|[embed|object|applet]" selectors in the file might confuse people into thinking that it applies to all content, when it only really applies to the bound object and its anonymous children. I don't feel strongly about it, but I'd leave it in if I were writing the patch.

Comment 10

[Alfred Kayser]

Please replace:
<xul:box class="icon"/>
with:
<xul:image class="icon"/>

The icon is clearly an image, and by using image, one can then use 'list-style-image/-moz-image-region' to put the different icons into one file, which is a clear performance benefit (less files, less images in cache, etc...)

Comment 11

[Justin Dolske [:Dolske]]

No, using <xul:image> will result in load events being fired, which might end up confusing scripts in the page. Same issue as bug 464371.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Comment 13

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

Comment on attachment 425565 [details] [diff] [review]
Patch v.4

>diff --git a/toolkit/mozapps/plugins/content/missingPlugin.xml b/toolkit/mozapps/plugins/content/pluginProblem.xml

>+        <xul:vbox class="mainBox" role="link" flex="1">

This still seems to break focus-ability (i.e. the ability to tab into the plugin and activate with "enter"). -moz-user-focus: normal on the box seems to handle the focus, but still doesn't activate the click handler on Enter, presumably because it isn't a link :( Maybe we should just also add a key handler? Alternatively, go back to using an html:a - you mentioned styling issues with that, but I forget what they are.

Looks good apart from this issue.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Ok, it's now focusable, tabbale, and responds to clicks and enter.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

Oops, forgot to sync up pinstripe.

Comment 16

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

Comment on attachment 425875 [details] [diff] [review]
Patch v.6

>diff --git a/toolkit/themes/gnomestripe/mozapps/jar.mn b/toolkit/themes/gnomestripe/mozapps/jar.mn

>++ skin/classic/mozapps/plugins/backgroundStripes.png       (plugins/backgroundStripes.png)

Just realized you can just omit this, since gnomestripe just overrides winstripe, and you're adding the same image there. r=me with that change.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.7

Removed gnomestripe image.

Comment 18

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4df5c9eb3ea2

Comment 19

[Michael Ventnor]

Is the stripes image even necessary at all now that we have -moz-repeating-linear-gradient at our disposal?

Comment 20

[Justin Dolske [:Dolske]]

Good point, the thought never even crossed my mind. Seems like it should work fine.

File a followup, maybe with a patch? :)

Comment 21

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/5bcfcbe4f7cc

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 23

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430