Closed Bug 888510 Opened 11 years ago Closed 11 years ago

Review theming for the new plugin in-page UI

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox23 --- unaffected
firefox24 + verified

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
(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.
Attachment #774030 - Attachment description: 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 → 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
Attachment #774030 - Flags: review?(jaws)
Attachment #774030 - Flags: feedback?(dolske)
Assignee: shorlander → benjamin
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?
Attachment #774030 - Flags: review?(jaws)
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 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.
Attachment #774030 - Flags: feedback?(dolske) → feedback+
(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.
Tryserver builds are available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bsmedberg@mozilla.com-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 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.
Attachment #777085 - Flags: review?(jaws)
(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.
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.
Maybe obvious, but looks like you're missing the icon in the urlbar for the doorhanger anchor? (between back button and grey globe)
Yeah, that's fixed as of the review comment at bug 888292 comment 6.
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.
Flags: needinfo?(benjamin)
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.
Attachment #777085 - Flags: review?(jaws) → review+
Attachment #774030 - Attachment is obsolete: true
Summary: Review theming for the new plugin doorhanger and in-page UI → Review theming for the new plugin in-page UI
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 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.
Attachment #777085 - Flags: approval-mozilla-aurora?
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.
> 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.
Attached patch 147999.diffSplinter Review
Alternate patch: avoid string changes by not adding an accessible title to the hide-plugin button.
Attachment #783260 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bbajaj)
Thanks for the alternate Benjamin, lets go with this for now.
Flags: needinfo?(bbajaj)
Attachment #783260 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #777085 - Flags: approval-mozilla-aurora?
Blocks: 901871
(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
(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.
(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
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.