Closed Bug 539848 Opened 12 years ago Closed 12 years ago

Make in-content plugin problem UI look better

Categories

(Toolkit :: Themes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: Dolske, Assigned: Dolske)

References

(Depends on 1 open bug)

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 file, 7 obsolete files)

Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
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.
Attached image Screenshot: Old vs. Patch v.1 (obsolete) —
Icon's fuzzy, because it's being scaled up, but this is the general idea.
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.
Attached patch Patch v.2 (WIP) (obsolete) — Splinter Review
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].
Attachment #421745 - Attachment is obsolete: true
Attachment #421746 - Attachment is obsolete: true
(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).
Attached patch Patch v.3 (obsolete) — Splinter Review
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
Attachment #424196 - Attachment is obsolete: true
Attachment #424738 - Flags: review?(gavin.sharp)
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 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.
Attachment #424738 - Flags: review?(gavin.sharp) → review-
(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.]
(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.
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...)
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.
Attachment #424738 - Attachment description: Patch v.1 → Patch v.3
Attached patch Patch v.4 (obsolete) — Splinter Review
Attachment #424738 - Attachment is obsolete: true
Attachment #425565 - Flags: review?(gavin.sharp)
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.
Attachment #425565 - Flags: review?(gavin.sharp) → review-
Attached patch Patch v.5 (obsolete) — Splinter Review
Ok, it's now focusable, tabbale, and responds to clicks and enter.
Attachment #425565 - Attachment is obsolete: true
Attachment #425873 - Flags: review?(gavin.sharp)
Attached patch Patch v.6 (obsolete) — Splinter Review
Oops, forgot to sync up pinstripe.
Attachment #425873 - Attachment is obsolete: true
Attachment #425875 - Flags: review?(gavin.sharp)
Attachment #425873 - Flags: review?(gavin.sharp)
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.
Attachment #425875 - Flags: review?(gavin.sharp) → review+
Attached patch Patch v.7Splinter Review
Removed gnomestripe image.
Attachment #425875 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/4df5c9eb3ea2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
Is the stripes image even necessary at all now that we have -moz-repeating-linear-gradient at our disposal?
Good point, the thought never even crossed my mind. Seems like it should work fine.

File a followup, maybe with a patch? :)
Depends on: 545304
Depends on: 545306
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
You need to log in before you can comment on or make changes to this bug.