888510 - Review theming for the new plugin in-page UI

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Benjamin Smedberg]

The new plugin doorhanger landed in Fx24 with preliminary styling on the doorhanger itself.

lco showed me some mockups where the buttons are "flat" instead of normal buttons, but I was not able to implement this because it hid all the focus handling (outlines) as well as default-button indications. If this is still the desired behavior, I need some more detailed understanding of what it should look like on each platform, especially in terms of keyboard/focus handling.

We also need to fix the in-page UI for click-to-play plugins. lco showed me http://cl.ly/image/1K2s3I050X2u but I really don't think we should be using a "play" triangle for this... can we use the lego-brick icon for this case?

Feel free to reassign this to me to write a patch when I have the icons and clear direction on any other tweaks that are needed to ship.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #0)
> lco showed me some mockups where the buttons are "flat" instead of normal
> buttons, but I was not able to implement this because it hid all the focus
> handling (outlines) as well as default-button indications. If this is still
> the desired behavior, I need some more detailed understanding of what it
> should look like on each platform, especially in terms of keyboard/focus
> handling.

On Windows and Linux, I would recommend keeping the OS-controlled look. It's the simplest way to ensure the different states (default-button highlight/hover/focus/pressed) don't feel alien with various OS themes. Incidentally, on Windows 8 the default button look is "flat" anyway.

Comment 2

[Benjamin Smedberg]

Attachment: In-content theming fixup for the new plugin click-to-activate. Remove the "lightweight" transparent click-to-play theming and make the normal styling plain grey. Switch from a CSS gradient to an image gradient because of performance issues. Highlight the

Feedback: :dolske

Comment 3

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

Comment on attachment 774030 [details] [diff] [review]
In-content theming fixup for the new plugin click-to-activate. Remove the "lightweight" transparent click-to-play theming and make the normal styling plain grey. Switch from a CSS gradient to an image gradient because of performance issues. Highlight the

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

There were a lot of hunk failures when I tried applying the patch. Is this patch on top of some other UI changes for ctp? I'd like to test this out before r+'ing. Can you help me out with what patches need to be applied to apply this?

::: toolkit/mozapps/plugins/content/pluginProblem.xml
@@ +28,5 @@
>                      <html:button class="icon"/>
> +                    <html:div class="msg msgTapToPlay">&tapToPlayPlugin;</html:div>
> +                    <html:div class="msg msgClickToPlay">&clickToActivatePlugin;</html:div>
> +                    <html:div class="msg msgVulnerabilityStatus" anonid="vulnerabilityStatus"><!-- set at runtime --></html:div>
> +                </html:label>

What is the <label> used for here? I would have expected to see an <input> here to associated the content with the input field.

Also, <div>s inside of a <label> are not supposed to happen. See https://developer.mozilla.org/en-US/docs/Web/HTML/Content_categories#Phrasing_content for the permitted content of a <label>.

@@ +41,5 @@
>                      <html:div class="msgReload">&reloadPlugin.pre;<html:a class="reloadLink" href="">&reloadPlugin.middle;</html:a>&reloadPlugin.post;</html:div>
>                  </html:div>
>  
>                  <html:div class="installStatus">
> +                    <html:div class="msg msgInstallPlugin"><html:a class="action-link" anonid="installPluginLink" href="">&installPlugin;</html:a></html:div>

Can you break up these long lines?

::: toolkit/themes/shared/plugins/pluginProblem.css
@@ +12,5 @@
>    text-align: center;
> +  display: table;
> +  width: 100%;
> +  height: 100%;
> +  background-color: rgb(72, 72, 72);

nit, no spaces after commas in color macros.

@@ +94,5 @@
>  :-moz-handler-vulnerable-updatable .msgClickToPlay,
>  :-moz-handler-vulnerable-no-update .msgClickToPlay {
>    cursor: pointer;
>  }
> +*/

Can this commented out code be deleted?

Comment 4

[Benjamin Smedberg]

Jared, this is https://github.com/bsmedberg/mozilla-central/compare/ctpfixup which is sitting on top of bug 888908, bug 889788, and bug 888292.

The <label> is there for accessibility, so that in a screen reader the <html:button> is read with the label below. I can certainly change the <div class="msg"> into <span class="msg">, although I'm surprised that we actually care about HTML document structure to that degree.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 774030 [details] [diff] [review]
In-content theming fixup for the new plugin click-to-activate. Remove the "lightweight" transparent click-to-play theming and make the normal styling plain grey. Switch from a CSS gradient to an image gradient because of performance issues. Highlight the

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

::: toolkit/mozapps/plugins/content/pluginProblem.xml
@@ +21,5 @@
>          <stylesheet src="chrome://mozapps/skin/plugins/pluginProblem.css"/>
>      </resources>
>  
>      <content>
> +        <html:div class="mainBox" chromedir="&locale.dir;">

Since this is going all-HTML now (yay!), would be nice to dump the namespace cruft entirely. We do this in videocontrols.xml.

<content xmlns="http://www.w3.org/1999/xhtml"> should do it, although I vaguely recall it actually needing to be <xbl:content xmlns="...">  (Probably need to add a xmlns:xbl="http://www.mozilla.org/xbl" in <bindings> too)

Maybe a separate bug, since that would make the diffs here messier.

::: toolkit/themes/shared/plugins/pluginProblem.css
@@ +9,5 @@
>  .mainBox {
>    font: message-box;
>    font-size: 12px;
>    text-align: center;
> +  display: table;

flexbox might be the new hotness to use here, but a table seems fine.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Jared, this is https://github.com/bsmedberg/mozilla-central/compare/ctpfixup
> which is sitting on top of bug 888908, bug 889788, and bug 888292.

Screenshots of the whole stack would probably be helpful for a final review/ui-review.

> The <label> is there for accessibility, so that in a screen reader the
> <html:button> is read with the label below.

Can we drop the <label> entirely? Using aria-labeledby would be perfect here, except it wants an ID and we're in XBL. I think <button aria-label=""> with a bit of JS to copy the relevant msgTapToPlay/msgClickToPlay/msgVulnerabilityStatus textContent into it? Our A11Y folks might have a more refined suggestion.

Comment 7

[Benjamin Smedberg]

Tryserver builds are available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b51fccb2cccd/

I already got a11y to review the current markup, and the JS to copy the markup would be complex. I don't understand what's wrong with the <label> given that it's standard markup.

Comment 8

[Benjamin Smedberg]

Attachment: I shortened the one line that I could, but I couldn't do much because whitespace isn't allowed in most of these.

Comment 9

[Benjamin Smedberg]

Comment on attachment 777085 [details] [diff] [review]
I shortened the one line that I could, but I couldn't do much because whitespace isn't allowed in most of these.

This is on top of bug 888292, which was missing a review request until now.

Comment 10

[Benjamin Smedberg]

Screenshots:
http://screencast.com/t/HqqWwl4joQl
http://screencast.com/t/cwQMizxeb
http://screencast.com/t/W1Rvvdqhxy2
http://screencast.com/t/vDXQziW9
http://screencast.com/t/qE61MV8bJoDw

Comment 11

[Larissa Co [:lco]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)

> http://screencast.com/t/cwQMizxeb
> http://screencast.com/t/W1Rvvdqhxy2

Why does this string say "Activate Java..." when it's vulnerable? I think it should say "The <plugin type> plugin is vulnerable and should be updated" etc.

Comment 12

[Benjamin Smedberg]

I did not change any of this wording. I do think it should be made clear to the user that if they want to activate java anyway, they need to click on the plugin. But I'd also prefer not to mess with it here, since it's not a change and the unsafe/vulnerable discussion is still not finalized.

Comment 13

[Justin Dolske [:Dolske]]

Maybe obvious, but looks like you're missing the icon in the urlbar for the doorhanger anchor? (between back button and grey globe)

Comment 14

[Benjamin Smedberg]

Yeah, that's fixed as of the review comment at bug 888292 comment 6.

Comment 15

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

Can you do a try-push with the required patches? I'd like to test this out as part of the review. I apologize for the slow turn-around-time here.

Comment 16

[Benjamin Smedberg]

Try builds are up: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bfd5f75fed0e/

Comment 17

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

Comment on attachment 777085 [details] [diff] [review]
I shortened the one line that I could, but I couldn't do much because whitespace isn't allowed in most of these.

This looks good but we NEED to update the theming of the doorhanger before this reaches release builds. At the least, we will need to add some more padding between the buttons and the edges of the panel, change the background color in the box surrounding the buttons (likely to match http://screencast.com/t/tUvmSFvy) and we also need to align the close button properly.

Comment 18

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

It is easier to track multiple landings in separate bugs, so I have moved the necessary work to theme the panel to bug 895939.

Comment 19

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a39f63704fa

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a39f63704fa
https://hg.mozilla.org/mozilla-central/rev/79f46ef38047

Comment 21

[Benjamin Smedberg]

Comment on attachment 777085 [details] [diff] [review]
I shortened the one line that I could, but I couldn't do much because whitespace isn't allowed in most of these.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): in-content visual changes followup from bug 880735
User impact if declined: The visual appearance of the click_to_play UI is not consistent, and we don't get the new prominent "check for updates" link for plugins which are blocklisted out of date. The in-content UI is not accessible (but this is not a regression from previous releases)
Testing completed (on m-c, etc.): landed, basic manual verification
Risk to taking this patch (and alternatives if risky): Fairly low risk, and anything should be caught by manual QA verification. The only external risk is breaking 3rd-party themes.
String or IDL/UUID changes made by this patch: one string added; this string is necessary to make the plugin doorhanger properly accessible, otherwise there is an unlabeled button.

Comment 22

[bhavana bajaj [:bajaj]]

Benjamin, if this is not a regression from what we have in Fx23 today I am concerned about the uplift due to 

a) The only external risk is breaking 3rd-party themes.Any idea of how widely this is used and a lot of users may be impacted ?Is this via AMO addons or one's that are externally available by some way ? If its the former, may be we can mitigate this by QA testing ?
b)Regarding string changes : It seems necessary to have the doorhanger accessible from my read.In that case it is late l10n and would need Axel's input here for an exception if possible in this case unless there is any other workaround.

Comment 23

[Benjamin Smedberg]

> a) The only external risk is breaking 3rd-party themes.Any idea of how
> widely this is used and a lot of users may be impacted ?Is this via AMO
> addons or one's that are externally available by some way ? If its the
> former, may be we can mitigate this by QA testing ?

Themes have to be updated every release anyway, so the only real risk here is if a theme author already made the updates for Fx24 and wouldn't notice this change. Given what I know about theme development, that seems unlikely. But I'd certainly mark this with addon-compat for Jorge's attention.

> b)Regarding string changes : It seems necessary to have the doorhanger
> accessible from my read.In that case it is late l10n and would need Axel's
> input here for an exception if possible in this case unless there is any
> other workaround.

The workaround would be to land this patch without the string. That means the close button will not have an accessible label; this is not ideal, but it's not a regression from the current in-content UI.

Comment 24

[Benjamin Smedberg]

Attachment: 147999.diff

Alternate patch: avoid string changes by not adding an accessible title to the hide-plugin button.

Comment 25

[bhavana bajaj [:bajaj]]

Thanks for the alternate Benjamin, lets go with this for now.

Comment 26

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bfd31957404c
https://hg.mozilla.org/releases/mozilla-aurora/rev/1169ba0240dd

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 777085 [details] [diff] [review]
I shortened the one line that I could, but I couldn't do much because whitespace isn't allowed in most of these.

Clearing the approval nom on this one as the alternate patch was approved

Comment 28

[Paul Silaghi, QA [:pauly]]

(In reply to Benjamin Smedberg  [:bsmedberg] PTO 8-Aug until 18-Aug, workweek high latency 19-Aug through 23-Aug from comment #10)
> http://screencast.com/t/W1Rvvdqhxy2
When is this supposed to actually happen ?
I can only see http://screencast.com/t/cwQMizxeb for outdated Java

Comment 29

[Georg Fritzsche [:gfritzsche]]

(In reply to Paul Silaghi [QA] from comment #28)
> (In reply to Benjamin Smedberg  [:bsmedberg] PTO 8-Aug until 18-Aug,
> workweek high latency 19-Aug through 23-Aug from comment #10)
> > http://screencast.com/t/W1Rvvdqhxy2
> When is this supposed to actually happen ?
That looks like the "vulnerable, no update available" case...

> I can only see http://screencast.com/t/cwQMizxeb for outdated Java

... while this is the "vulnerable, update available" case.

Comment 30

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #29)
> (In reply to Paul Silaghi [QA] from comment #28)
> > (In reply to Benjamin Smedberg  [:bsmedberg] PTO 8-Aug until 18-Aug,
> > workweek high latency 19-Aug through 23-Aug from comment #10)
> > > http://screencast.com/t/W1Rvvdqhxy2
> > When is this supposed to actually happen ?
> That looks like the "vulnerable, no update available" case...

... following up on IRC, this is covered here:
http://benjamin.smedbergs.us/tests/ctptests/#single-unsafe

Comment 31

[Paul Silaghi, QA [:pauly]]

The new plugin in-page UI looks ok on FF 24b1, 25.0a2 2013-08-07 Win 7, Mac OS X 10.8, Ubuntu 13.04