831921 - Make the plugin click-to-play UI look less like 'plugin broken/crashed' UI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Benjamin Smedberg]

One consistent piece of feedback about the click-to-play UI is that it makes webpages look broken, especially when they have lots of ads. This is probably fine when the plugin is blocked for security reasons, but if users chose to make plugins CtP we'd like to make the UI less obtrusive.

Shorlander says he has mockups already and will attach them! 

Related, but the close box from bug 774315 will be included.

Comment 1

[Justin Dolske [:Dolske]]

We may also want to use such calmer UI for cases when the plugin is user-disabled or not-yet-installed, for similar reasons. If the same style can work for these cases, great (and if not: new bug!).

In hindsight, we probably should have kept the noisy/stripy UI just for crashed plugins, since that's what it was originally made for.

Comment 2

[Benjamin Smedberg]

shorlander, will you be able to paste your work today? I'm trying to get the test builds for the UR study done for early next week.

Comment 3

[Stephen Horlander [:shorlander]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> shorlander, will you be able to paste your work today? I'm trying to get the
> test builds for the UR study done for early next week.

Yes. Will post something in a little while.

Comment 4

[Stephen Horlander [:shorlander]]

Attachment: Click-to-Play placeholder area

This is an updated click-to-play placeholder style.

Goals:
- Lighter
- Less in your face
- Doesn't look broken
- Should work on most backgrounds

Going to have to use new icons since the inverted icons won't work on the lighter more translucent box.

There might also be cases where it might not be as visible as desired if it is overlaying complex content. So we might want to consider make it more opaque on hover for those scenarios.

Comment 5

[Stephen Horlander [:shorlander]]

Specs:

background-color: hsla(0,0%,70%,.1)
border-color: hsla(0,0%,70%,.5)

Comment 6

[Dirkjan Ochtman (:djc)]

I think this work could obviate bug 775020. Looks great!

Comment 8

[Benjamin Smedberg]

shorlander, I spent some time with this and discovered some issues. This works really well when the underlying page has a white/light background, but for sites such as hulu that have dark backgrounds, I couldn't get the text to be readable and the border looked weird because it's lighter than everything surrounding.

I was playing with the colors and opacities and came up with this:

http://screencast.com/t/lSLQGUyx

* no border, because I couldn't get the border to look right across light/dark
* background: hsla(0,0%,85%,.4);
* text-shadow: hsla(0,0%,85%,0.65) 0 0 4.5px; invisible on light-colored sites, makes text readable on dark-colored sites
* hover: background: hsla(0,0%,75%,.55)

Your mockup doesn't have the "click to play" text. Was that intentional? I think we need to keep the text for the cases where a plugin is insecure, but we could certainly drop it in the "normal CtP" case.

Comment 9

[Robert Kaiser]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Your mockup doesn't have the "click to play" text.

Given the feedback we get from the Java CTP blocks, I wonder if we actually should include some text on how to add permanent exceptions for sites, but that may belong in yet another bug.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Robert Kaiser (:[e-mail]) from comment #9)
> Given the feedback we get from the Java CTP blocks, I wonder if we actually
> should include some text on how to add permanent exceptions for sites, but
> that may belong in yet another bug.

Yes, another bug please.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Your mockup doesn't have the "click to play" text. Was that intentional? I
> think we need to keep the text for the cases where a plugin is insecure, but
> we could certainly drop it in the "normal CtP" case.

Benjamin, I talked with Stephen over IRC regarding the text, and the response that I got was that we should have two different views of this UI. If the plugin is known to be insecure, then it should continue to have the warning stripes and text stating which plugin will run as well as the vulnerability state that is known (this is text that already appears on insecure CTP overlays). Otherwise it can have this cleaner view without the text.

Comment 11

[Benjamin Smedberg]

Attachment: Winstripe changes, for the study at least, rev. 1

After IRC discussion, I got the border to behave reasonably by making it very lightly transparent black, for the same effect when on a light background, and mostly invisible on dark backgrounds.

I'm planning on including this in the custom build for the UR study (which is Windows only). Assuming this is generally acceptable, I'll make the equivalent changes to pinstripe. Note that this does not change the icons, since I don't have the tools to do that and the existing icons actually look pretty much ok. If an icon change is required for "real" checkin, please let me know.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 706099 [details] [diff] [review]
Winstripe changes, for the study at least, rev. 1

Review of attachment 706099 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
@@ -67,2 @@
>    cursor: default;
> -  text-shadow: rgba(0,0,0,0.8) 0 0 3.5px;

Why does this need to move to .mainBox? Are there other changes to the XBL that aren't part of this patch that require this move? Looking at the XBL, all user-facing text already has the .msg class.

@@ +131,5 @@
> +
> +:-moz-handler-clicktoplay .mainBox {
> +  background: hsla(0,0%,85%,.3);
> +  border: 1px dashed rgba(0,0,0,0.1);
> +  color: black;

We should use something a little brighter than solid black, maybe #444.

@@ +147,5 @@
> +}
> +
> +:-moz-handler-clicktoplay .msgClickToPlay {
> +  display: none;
> +}

This is confusing to me. Why is the click-to-play message hidden when the plugin is in click-to-play state?

Comment 13

[Benjamin Smedberg]

(In reply to Jared Wein [:jaws] from comment #12)
> Comment on attachment 706099 [details] [diff] [review]
> Winstripe changes, for the study at least, rev. 1
> 
> Review of attachment 706099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
> @@ -67,2 @@
> >    cursor: default;
> > -  text-shadow: rgba(0,0,0,0.8) 0 0 3.5px;
> 
> Why does this need to move to .mainBox? Are there other changes to the XBL
> that aren't part of this patch that require this move? Looking at the XBL,
> all user-facing text already has the .msg class.

There's no reason for it to be on the subclass (fonts inherit); this rule is more specific and therefore performs better.

> @@ +131,5 @@
> > +
> > +:-moz-handler-clicktoplay .mainBox {
> > +  background: hsla(0,0%,85%,.3);
> > +  border: 1px dashed rgba(0,0,0,0.1);
> > +  color: black;
> 
> We should use something a little brighter than solid black, maybe #444.

I disagree; using a lighter color means that you'd end up with a light border when the plugin is on a dark background. The quite-transparent black is basically a "darken" border, which works well across all the examples I've seen.

> 
> @@ +147,5 @@
> > +}
> > +
> > +:-moz-handler-clicktoplay .msgClickToPlay {
> > +  display: none;
> > +}
> 
> This is confusing to me. Why is the click-to-play message hidden when the
> plugin is in click-to-play state?

Because of what was said in comment 10. When the plugin is blocked, we still display the message (and the darker stripy UI).

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 706099 [details] [diff] [review]
Winstripe changes, for the study at least, rev. 1

Review of attachment 706099 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
@@ +132,5 @@
> +:-moz-handler-clicktoplay .mainBox {
> +  background: hsla(0,0%,85%,.3);
> +  border: 1px dashed rgba(0,0,0,0.1);
> +  color: black;
> +  text-shadow: hsla(0,0%,85%,0.65) 0 0 4.5px;

The rules about text-shadow and color should be removed since there is no text visible and thus they don't really matter.

@@ +138,5 @@
> +  border-radius: 0;
> +}
> +
> +:-moz-handler-clicktoplay html|a {
> +  color: black;

Is this rule useful? I think this can go too if the there is no text for non-vulnerable CTP.

@@ +142,5 @@
> +  color: black;
> +}
> +
> +:-moz-handler-clicktoplay .mainBox:hover {
> +  background: hsla(0,0%,80%,.45);

Please use the more specific background-color property for this rule and for the |:-moz-handler-clicktoplay .mainBox| rule.

@@ +147,5 @@
> +}
> +
> +:-moz-handler-clicktoplay .msgClickToPlay {
> +  display: none;
> +}

Ok, I see now re: comment #10. This change should be made to the pluginProblemContent.css file.

Comment 15

[Benjamin Smedberg]

Shorlander posted a new visual http://cl.ly/image/0N2y1C3B0R2G/o based on feedback from user research.

I've tried to implement this here: http://screencast.com/t/Y6wnrz6vaY9W

shorlander, can you verify that this is what you wanted?

Details:
* plugin background: rgba(255,255,255,0.4) (invisible on white backgrounds, but allows the text and buttons to pop on dark backgrounds)
* border, button, and text: rbga(0,0,0,0.2) (#CCC as per your mockup on white backgrounds, darkens atop the background on dark backgrounds)
* button and text on hover: rgba(0,0,0,0.4)

Comment 16

[Dão Gottwald [:dao]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> Shorlander posted a new visual http://cl.ly/image/0N2y1C3B0R2G/o based on
> feedback from user research.
> 
> I've tried to implement this here: http://screencast.com/t/Y6wnrz6vaY9W
...
> * border, button, and text: rbga(0,0,0,0.2) (#CCC as per your mockup on
> white backgrounds, darkens atop the background on dark backgrounds)

That text is basically unreadable over here if I look with a non-ideal angle at my screen such that the contrast decreases.

Comment 17

[Benjamin Smedberg]

Attachment: New patch WIP

Comment 18

[Benjamin Smedberg]

Please don't change my bugs components.

Comment 19

[Benjamin Smedberg]

Attachment: Final visuals for review

I haven't tested the pinstripe adaptation of this yet, so I'm going to hold off the review request until I've at least built that. Shorlander sent me some tweaks on top of the original patch and the icon UI is visible on all backgrounds. The text is not visible on some medium-grey backgrounds, but I don't think that is a problem in this case.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 710293 [details] [diff] [review]
Final visuals for review

Review of attachment 710293 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if not much has to change, but please request review again if the patch has to change a bit between this version and the one that lands.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +124,5 @@
>  activatePluginsMessage.always.accesskey=c
>  activatePluginsMessage.never=Never activate plugins for this site
>  activatePluginsMessage.never.accesskey=N
>  activateSinglePlugin=Activate
> +PluginClickToPlay=Activate %S.

The string name here needs to change since the string value changed. You can change it to PluginClickToPlay2 if you can't think of a better name.

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +34,5 @@
>  
>  <!ENTITY missingPlugin                                       "A plugin is needed to display this content.">
>  <!-- LOCALIZATION NOTE (tapToPlayPlugin): Mobile (used for touch interfaces) only has one type of plugin possible. -->
>  <!ENTITY tapToPlayPlugin                                     "Tap here to activate plugin.">
> +<!ENTITY clickToPlayPlugin                                   "Activate plugin.">

Same thing here, please update the entity name.

@@ +57,5 @@
>  <!ENTITY report.failed                                       "Submission failed.">
>  <!ENTITY report.unavailable                                  "No report available.">
>  
>  <!ENTITY plugin.file                                         "File">
> +<!ENTITY plugin.mimeTypes                                    "MIME Types">

What changed here? Added a newline at the end of the file?

::: toolkit/mozapps/plugins/content/pluginProblem.xml
@@ +26,5 @@
>              <html:button class="closeIcon" anonid="closeIcon"/>
> +            <xul:vbox class="hoverBox" flex="1">
> +                <xul:spacer flex="1"/>
> +                <html:div>
> +                <html:button class="icon" anonid="icon"/>

The anonid here can be removed since it isn't used.

Why does this need to be placed in a div? Is this just to make the icon appear as block level? display:block on the .icon should be enough if display:inline-block isn't working for you. (Note also that this line should be indented 4 more spaces)

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +84,2 @@
>    cursor: default;
> +  width: 100%;

Is the 100% width needed here? The .msg class is used on DIVs, which are display:block by default and hence already have full width.

::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
@@ +90,3 @@
>  :-moz-handler-vulnerable-updatable .msgClickToPlay,
>  :-moz-handler-vulnerable-no-update .msgClickToPlay {
>    cursor: pointer;

The pointer cursor used to be displayed only on the text of the click-to-play overlay. With this patch it is displayed everywhere except for the text and the close icon on the overlay.

Please add a rule for |:-moz-handler-clicktoplay .msgClickToPlay| to this ruleset so that the text will continue to have the pointer cursor.

I am OK with leaving the default cursor for the close icon.

Comment 21

[Benjamin Smedberg]

> The string name here needs to change since the string value changed. You can
> change it to PluginClickToPlay2 if you can't think of a better name.

made it PluginClickToActivate


> >              <html:button class="closeIcon" anonid="closeIcon"/>
> > +            <xul:vbox class="hoverBox" flex="1">
> > +                <xul:spacer flex="1"/>
> > +                <html:div>
> > +                <html:button class="icon" anonid="icon"/>
> 
> The anonid here can be removed since it isn't used.
> 
> Why does this need to be placed in a div? 

The icon itself has to be a 48x48 box (because we are now using background-position to do the hover/active states). So it cannot be a block, so it's an inline-block wrapped in this <div>.

> ::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
> @@ +84,2 @@
> >    cursor: default;
> > +  width: 100%;
> 
> Is the 100% width needed here?

Oddly, yes. Because of how the XUL box model interacts with block-and-inline, the block-level elements only expanded to fit the width of their contents. This forces that block to fill the entire width of its container.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/187e78781793

Comment 23

[Paul Silaghi, QA [:pauly]]

Why I can't see any changes in nightly 21.0a1 (2013-02-07) regarding this ?

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Paul Silaghi [QA] from comment #23)
> Why I can't see any changes in nightly 21.0a1 (2013-02-07) regarding this ?

I think it just missed the 2-7 build somehow. It is in the 02-08 build today though.

Comment 25

[Paul Silaghi, QA [:pauly]]

Couple of suggestions:
1. Make the X button whiter for out-of-date plugins (just like for up-to-date plugins) https://bugzilla.mozilla.org/show_bug.cgi?id=774315#c18
2. Make the new overlay writing whiter for up-to-date plugins (just like for out-of-date plugins), it's hard to notice on dark backgrounds (youtube)

Comment 26

[Christian Ascheberg]

(In reply to Jared Wein [:jaws] from comment #20)
> ::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
> @@ +90,3 @@
> >  :-moz-handler-vulnerable-updatable .msgClickToPlay,
> >  :-moz-handler-vulnerable-no-update .msgClickToPlay {
> >    cursor: pointer;
> 
> The pointer cursor used to be displayed only on the text of the
> click-to-play overlay. With this patch it is displayed everywhere except for
> the text and the close icon on the overlay.
> 
> Please add a rule for |:-moz-handler-clicktoplay .msgClickToPlay| to this
> ruleset so that the text will continue to have the pointer cursor.
> 
> I am OK with leaving the default cursor for the close icon.

The play icon |:-moz-handler-clicktoplay .icon| also has the default cursor. Is that intended as well?

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Christian Ascheberg from comment #26)
> (In reply to Jared Wein [:jaws] from comment #20)
> > ::: toolkit/themes/winstripe/mozapps/plugins/pluginProblem.css
> > @@ +90,3 @@
> > >  :-moz-handler-vulnerable-updatable .msgClickToPlay,
> > >  :-moz-handler-vulnerable-no-update .msgClickToPlay {
> > >    cursor: pointer;
> > 
> > The pointer cursor used to be displayed only on the text of the
> > click-to-play overlay. With this patch it is displayed everywhere except for
> > the text and the close icon on the overlay.
> > 
> > Please add a rule for |:-moz-handler-clicktoplay .msgClickToPlay| to this
> > ruleset so that the text will continue to have the pointer cursor.
> > 
> > I am OK with leaving the default cursor for the close icon.
> 
> The play icon |:-moz-handler-clicktoplay .icon| also has the default cursor.
> Is that intended as well?

No, it is not intended. I saw that and never got around to filing a follow-up bug. Can you please file a bug for that?