Update styling of Click-to-Play UI to match Photon

VERIFIED FIXED in Firefox 58

Status

()

Core
Plug-ins
P1
normal
VERIFIED FIXED
a month ago
11 days ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Blocks: 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 verified)

Details

(Whiteboard: [reserve-photon-visual], URL)

User Story

Proposed changes as per the latest version + some vidyo conversations with Bram:

- Dynamically shrink the plugin icon and hide the text and close buttons at small sizes, in order to display the activation UI for smaller sizes
- Update the icon to the same SVG one used in the URL bar
- Update the close button to match the tab's close button
- Update colors
- Increase font-size to 13px

Note: except for the close button change, all the other changes will be constrained to the click-to-play UI only. We'll redesign the other states (plugin blocked, plugin vulnerable, plugin crashed, etc.) in a follow-up bug.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 3 obsolete attachments)

55.26 KB, image/png
Details
395.05 KB, image/png
Details
59 bytes, text/x-review-board-request
johannh
: review+
Details | Review
59 bytes, text/x-review-board-request
johannh
: review+
Details | Review
59 bytes, text/x-review-board-request
johannh
: review+
Details | Review
59 bytes, text/x-review-board-request
johannh
: review+
Details | Review
59 bytes, text/x-review-board-request
johannh
: review+
dthayer
: review+
Details | Review
59 bytes, text/x-review-board-request
johannh
: review+
Details | Review
119.37 KB, image/png
Details
(Assignee)

Description

a month ago
Created attachment 8919010 [details]
Minimum plugin size to display activation UI

Currently, the plugin activation UI displays for plugins larger than ~119x119 (see screenshot), but this is not hard-coded. What the code does is to check "would this content display a scrollbar here?", and if the answer is yes, then we don't display it. (This means that it varies by font, OS, and e.g. a narrower but taller plugin would also be capable of displaying it)

So it already displays on the minimum possible size for the situation. What we could do here is to create a smaller version of this UI that displays for smaller plugins (for example, not showing the "This site uses..." text; or just showing the icon and nothing else).  Not sure if we can use media queries for that or not.
(Assignee)

Updated

a month ago
Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 1

a month ago
Created attachment 8919027 [details]
Remove warning text and reduce icon to 32x32 for small sizes

Here is my proposal: if the full UI won't fit in the plugin size, we do the following two things (at the same):
  - Remove the warning text
  - Reduce the brick icon size from 48x48 to 32x32

This brings the minimum plugin size (on my machine) to 73x73   (from 119x119 originally, as you can see in the first screenshot)

I tried to reduce the icon further to 16x16 but the benefit wasn't big (minimum became 61x61, and it started to look very ugly)

What's your opinion on this?  I have the code but I haven't written a test yet. If everyone agrees I'll clean up the code and write a test.
Attachment #8919027 - Flags: feedback?(rtestard)
Attachment #8919027 - Flags: feedback?(bram)
(Assignee)

Comment 2

a month ago
Another option would be to entirely drop the warning text. It's a bit scary-sounding (on purpose I suppose), but remember that this is mentioned again on the doorhanger that appears after the user clicks to activate. So it's not clear to me that there's benefit in continuing to display this warning.

With the warning text removed, and the icon preserved at the same size (48x48), the minimum size on my tests become 83x83.

Comment 3

a month ago
Created attachment 8919095 [details]
plugins blocking grey box - i01

Hi Felipe,

You’re right that the scary-sounding warning text is mentioned again on the doorhanger, so there’s no need to repeat them in the box.

It’s still worth it to keep some text in this grey box, although I think it should be drastically shortened to something like “Run Adobe Flash”. Just three short words.

And when the words don’t fit anymore (let’s say, when the box gets smaller than a certain dimension), we can just show the icon – which can gradually scale down from 48px, 32px to 16px (again, depending on the box dimension).

I’m not sure if this is also the right opportunity to redesign the grey box to fit Photon, but I’m putting a little visual change just in case it’s easy to make. It consists of changing the puzzle and close icons, typography and colour – all using our existing guidelines. The specs is attached.

What do you think?
(Assignee)

Comment 4

a month ago
Thanks Bram, this looks nice!

I'll work on this asap to see how simple or big the patch becomes, with the hope we can ask approval for beta 57 on this.

We can't make the string change (from "Activate" to "Run") though because of the string freeze, so that will have to wait until 58.

A couple questions about the proposal:

1) do you know if we already have this close button and plugin icon assets in the product (as .svg)? If not, can you provide them?

I know that the plugin icon in the url bar looks very similar to this one, so that might be it, right? I haven't tested yet if the spacing on it scales well to the bigger 48x48 size.


2) In your proposal, the close icon was not displayed in the large plugin area, just on the smaller one. I assume that wasn't intentional, right?
(Assignee)

Comment 5

a month ago
Created attachment 8919132 [details]
rough 1st draftA

Alright, here's a rough first pass on this, with the colors as spec'ed, and using the plugin icon from the URL bar, and the close X icon from the tab bar.

I think the positioning of the X looks a bit off, but I haven't touched it yet.. I just changed the image and set the right color on it.
(Assignee)

Comment 6

a month ago
One thing to note is that this UI has a lot more variation than this for other cases, such as vulnerable plugin, outdated, non-existing plugins..  It's a bit hard to trigger them from code, so I'll look into how to do that so that we can compare and see if things will be too mismatched. If yes then we'd probably need to redesign it all at once, and that would be a larger task..

Comment 7

a month ago
Hi Felipe. Your first draft looks nice! Let’s not worry about changing the strings, and save that for later.

By the looks of it, I think you’ve gotten the icon assets and colour values already, but just in case they’re missing, I’ve taken them straight off the Photon Design System:

* Icon: http://design.firefox.com/icons/
* Colour: http://design.firefox.com/photon/visuals/color.html

I must admit that I didn’t take all the states into account when designing the grey box. I thought that these states would only impact what we show on the doorhanger, but now I’ve realised that the grey box must change to suit. This could get complicated. Thanks for bringing it up.

In that case, would you agree that the simplest thing to do would be to only redesign the state that most users will encounter (ie. “Activate Adobe Flash”) and then save all the other states for later?

Comment 8

a month ago
(In reply to :Felipe Gomes (needinfo me!) from comment #1)

> What's your opinion on this?  I have the code but I haven't written a test
> yet. If everyone agrees I'll clean up the code and write a test.

I agree with the approach proposed by Bram on comment 3
The design Bram looks really good, makes the current one feel so old!

Updated

a month ago
Attachment #8919027 - Flags: feedback?(rtestard) → feedback-

Comment 9

a month ago
Created attachment 8919487 [details]
plugins blocking grey box - i02

I realise that my responsive design specs wasn’t complete. It missed a couple of things, such as:

* Should the close button always be shown?
* What happens if the box is too small to contain an icon?

Those concerns are addressed in this iteration and marked with a green sparkle.

Lastly, I also proposed an alternate, light-coloured design. I think our dark theme looks great already, but it never hurts to have an option to consider.
Attachment #8919095 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8919027 - Flags: feedback?(bram)
(Assignee)

Comment 10

a month ago
Bram, is there any change to apply to close button when the mouse is over it? color/background-color?
Flags: needinfo?(bram)

Comment 11

a month ago
Felipe and I had met on Video about this, and made a few decisions that should help resolve this bug.

1. Add a close button similar in style and properties to the close button on the tab bar.

2. Made sure that the responsive breakpoints can handle longer strings in non-English locales (ie. the strings don’t bleed and will look good inside the box).

3. Agreed that we should only redesign this very specific – but very common – use case where Flash is both up to date and working properly (not crashing). This is so we will be able to ship by 57. We will ship the design for other cases for 58 and future versions, as they contain variations of the puzzle icon, different backgrounds, etc. – all needing to be Photonised.
Flags: needinfo?(bram)
(Assignee)

Updated

a month ago
User Story: (updated)
Summary: Display plugin activation UI for smaller sized plugins → Update styling of Click-to-Play UI to match Photon
Whiteboard: [photon-visual]
(Assignee)

Updated

a month ago
Attachment #8919027 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8919132 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a month ago
I made these two pages to test the plugin at all sizes:

http://felipc.github.io/flashtest/fixedsizes.html

http://felipc.github.io/flashtest/index.html (dynamic resizer)
(Assignee)

Comment 19

a month ago
Created attachment 8920415 [details]
Before and After, as implemented by the current patch
(Assignee)

Comment 20

a month ago
tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81fb743ecd5c01d8550df497073a935b6827e5a6

builds:
https://tools.taskcluster.net/index/artifacts/gecko.v2.try.revision.81fb743ecd5c01d8550df497073a935b6827e5a6

Updated

a month ago
Priority: -- → P1
Whiteboard: [photon-visual] → [reserve-photon-visual]
We should also update the SUMO pages about Flash CTA to use the new plugin icon and CTA text:

https://support.mozilla.org/kb/why-do-i-have-click-activate-plugins
... when Firefox 58 is released on 2018-01-16.

Comment 23

a month ago
mozreview-review
Comment on attachment 8920406 [details]
Bug 1409148 - Remove the activation warning on the flash click-to-play overlay.

https://reviewboard.mozilla.org/r/191372/#review197148
Attachment #8920406 - Flags: review?(jhofmann) → review+

Comment 24

a month ago
mozreview-review
Comment on attachment 8920407 [details]
Bug 1409148 - Drop the old contentPluginClose.png icon and replace it with the modern svg version.

https://reviewboard.mozilla.org/r/191374/#review197152

::: toolkit/themes/shared/plugins/pluginProblem.css:10
(Diff revision 1)
>  @namespace html url(http://www.w3.org/1999/xhtml);
>  
>  /* These styles affect only the bound element, not other page content. */
>  /* Keep any changes to these styles in sync with plugin-doorhanger.inc.css */
>  .mainBox {
> + --grey-10: #f9f9fa;

Nit: I don't think it's necessary to use a CSS variable here. There's no consistent use of CSS variables for colors in Firefox UI unless they are shared in multiple places (and there were performance costs associated with CSS variables, not sure to what extent those were solved).
Attachment #8920407 - Flags: review?(jhofmann) → review+

Comment 25

a month ago
mozreview-review-reply
Comment on attachment 8920407 [details]
Bug 1409148 - Drop the old contentPluginClose.png icon and replace it with the modern svg version.

https://reviewboard.mozilla.org/r/191374/#review197152

> Nit: I don't think it's necessary to use a CSS variable here. There's no consistent use of CSS variables for colors in Firefox UI unless they are shared in multiple places (and there were performance costs associated with CSS variables, not sure to what extent those were solved).

Ah, nevermind, it's used in the other commits. Duh :D

Comment 26

a month ago
mozreview-review
Comment on attachment 8920408 [details]
Bug 1409148 - Drop the old contentPluginActivate.png icon and replace it with the modern svg version.

https://reviewboard.mozilla.org/r/191376/#review197160

::: toolkit/themes/shared/plugins/pluginProblem.css:79
(Diff revision 1)
>  :-moz-handler-blocked .icon {
>    background-image: url(chrome://mozapps/skin/plugins/contentPluginBlocked.png);
>  }
>  a .icon,
>  :-moz-handler-clicktoplay .icon {
> -  background-image: url(chrome://mozapps/skin/plugins/contentPluginActivate.png);
> +  background-image: url(chrome://browser/skin/notification-icons/plugin.svg);

Shouldn't this SVG live in toolkit? Is the plugin code used by XUL apps?

Comment 27

a month ago
mozreview-review
Comment on attachment 8920409 [details]
Bug 1409148 - Improve styling of the plugin Click-to-Play overlay.

https://reviewboard.mozilla.org/r/191378/#review197162
Attachment #8920409 - Flags: review?(jhofmann) → review+

Comment 28

a month ago
mozreview-review
Comment on attachment 8920410 [details]
Bug 1409148 - Add dynamic sizing on the plugin overlay.

https://reviewboard.mozilla.org/r/191380/#review197182

::: commit-message-28763:1
(Diff revision 1)
> +Bug 1409148 - Add dinamic sizing on the plugin overlay. r=johannh

nit: dynamic
Attachment #8920410 - Flags: review?(jhofmann) → review+

Comment 29

a month ago
mozreview-review
Comment on attachment 8920411 [details]
Bug 1409148 - Add test to ensure that the plugin overlay is displayed in the correct style.

https://reviewboard.mozilla.org/r/191382/#review197186

::: browser/base/content/test/plugins/browser_CTP_overlay_styles.js:11
(Diff revision 1)
> +
> +/* This test ensures that the click-to-play "Activate Plugin" overlay
> + * is shown in the right style (which is dependent on its size).
> + */
> +
> +var rootDir = getRootDirectory(gTestPath);

Nit: make this a const

::: browser/base/content/test/plugins/browser_CTP_overlay_styles.js:56
(Diff revision 1)
> +  },
> +}
> +
> +
> +add_task(async function() {
> +  registerCleanupFunction(function() {

You could make this

add_task(function cleanup() {
  clearAllPluginPermissions();
  ...
}

and put it at the bottom of the file. I've seen people do that a lot recently (and it's how I write cleanup functions myself), but I don't mind this style either.

::: browser/base/content/test/plugins/browser_CTP_overlay_styles.js:66
(Diff revision 1)
> +  });
> +});
> +
> +add_task(async function() {
> +  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> +  gTestBrowser = gBrowser.selectedBrowser;

I don't really understand why you need to have a gTestBrowser in this file, but I wouldn't say it's an issue either.

::: browser/base/content/test/plugins/browser_CTP_overlay_styles.js:79
(Diff revision 1)
> +
> +  // Work around for delayed PluginBindingAttached
> +  await promiseUpdatePluginBindings(gTestBrowser);
> +
> +  await ContentTask.spawn(gTestBrowser, {testcases}, async function({testcases}) {
> +      let doc = content.document;

Nit: indendation
Attachment #8920411 - Flags: review?(jhofmann) → review+
This looks all good to me, I'm simply unsure about the toolkit - browser relationship with plugins overlays. On one hand it seems like these overlays only get shown by browser code, on the other hand their styles and resources live in toolkit. It would be great if we could clarify this point. :)

Thanks!
(Assignee)

Comment 31

a month ago
Bram, could you take a look at the try builds here [1], to see the overall impression, and also to give a suggestion for one last thing that I did not realize existed.  When *clicking* on the click-to-play plugin, the :active style has a background color change, and also the text turns _red_. I never noticed before, but that's already the case today.  Do you have better color suggestions for that?

You can use the links from comment 18 to test it easily.




[1] https://index.taskcluster.net/v1/task/gecko.v2.try.revision.81fb743ecd5c01d8550df497073a935b6827e5a6.firefox.macosx64-opt/artifacts/public/build/target.dmg
Flags: needinfo?(bram)

Comment 32

a month ago
(In reply to :Felipe Gomes (needinfo me!) from comment #31)
> Bram, could you take a look at the try builds here [1], to see the overall
> impression, and also to give a suggestion for one last thing that I did not
> realize existed.  When *clicking* on the click-to-play plugin, the :active
> style has a background color change, and also the text turns _red_. I never
> noticed before, but that's already the case today.  Do you have better color
> suggestions for that?

I just had a look at the try build you provided. Generally, things looks very good!

This is what I would suggest for the onclick style:

* background-color: grey-50 (#737373) or grey-60 (#4a4a4f)
  * I have a feeling like grey-50 will provide the best contrast between the two states, but you can compare the two
* text color: same as non-click state
  * No need to change it to red, blue, or any other colour. The background change alone will be enough

Thanks a lot for making these changes!
Flags: needinfo?(bram)

Comment 33

a month ago
Just a small last note: I’m still seeing a shift-down on the brick svg, when it’s sized to 16x16px, so that it looks visually imbalanced (too far down the grey box). I’m not sure why this is, but the problem still persists after we last chatted.

Updated

a month ago
Flags: needinfo?(felipc)

Comment 34

a month ago
mozreview-review
Comment on attachment 8920408 [details]
Bug 1409148 - Drop the old contentPluginActivate.png icon and replace it with the modern svg version.

https://reviewboard.mozilla.org/r/191376/#review197962

Cancelling review while the pending issues are getting resolved :)
Attachment #8920408 - Flags: review?(jhofmann)
status-firefox57: --- → wontfix
(Assignee)

Comment 35

a month ago
FWIW, bug 1398972 touches some of the same code, so I'll wait for that to land and then I'll rebase these patches

Updated

29 days ago
Blocks: 1325171

Updated

29 days ago
Flags: qe-verify+
Bug 1398972 landed, time to rebase?
(Assignee)

Comment 37

22 days ago
I'm finishing this rebase. There are some non-trivial changes, so I'll post for re-review
Flags: needinfo?(felipc)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

21 days ago
Johann: I moved the plugin.svg and plugin-blocked.svg icons to toolkit, where it should belong.

Doug: Can you please take a look at the part that I tagged you for review? Johann had already reviewed, but there were some significant changes on top of the PopupNotifications work.
Originally I was keeping all that style setting in setVisibility, following the pattern from your changes.. But I ran into a problem so I had to move it back to the computeOverlayDisplayState function. I added a comment there explaining the reason.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

21 days ago
mozreview-review
Comment on attachment 8920410 [details]
Bug 1409148 - Add dynamic sizing on the plugin overlay.

https://reviewboard.mozilla.org/r/191380/#review201546

Looks good with the below answered/addressed.

::: browser/modules/PluginContent.jsm:296
(Diff revision 3)
>    },
>  
>    /**
>     * Update the visibility of the plugin overlay.
>     */
>    setVisibility(plugin, overlay, overlayDisplayState) {

Since setVisibility is called after every occurrence of computeOverlayDisplayState, should we just move it inside computeOverlayDisplayState? Additionally, if computeOverlayDisplayState has side effects I think the name should be adjusted to indicate that. "computeAndAdjustDisplayState", maybe?

::: browser/modules/PluginContent.jsm:947
(Diff revision 3)
>        if (!overlay) {
>          continue;
>        }
>        let overlayDisplayState = this.computeOverlayDisplayState(plugin, overlay);
>        this.setVisibility(plugin, overlay, overlayDisplayState);
> -      if (overlayDisplayState == OVERLAY_DISPLAY_VISIBLE) {
> +      if (overlayDisplayState > OVERLAY_DISPLAY.BLANK) {

This doesn't break any tests in browser/base/content/test/plugins?
Attachment #8920410 - Flags: review?(dothayer) → review+

Comment 48

19 days ago
mozreview-review
Comment on attachment 8920408 [details]
Bug 1409148 - Drop the old contentPluginActivate.png icon and replace it with the modern svg version.

https://reviewboard.mozilla.org/r/191376/#review201844

Looks great now, thank you!
Attachment #8920408 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 55

15 days ago
(In reply to Doug Thayer [:dthayer] from comment #47)
> Since setVisibility is called after every occurrence of
> computeOverlayDisplayState, should we just move it inside
> computeOverlayDisplayState? Additionally, if computeOverlayDisplayState has
> side effects I think the name should be adjusted to indicate that.
> "computeAndAdjustDisplayState", maybe?

I didn't merge setVisibility because, although after every call to computeOverlayDisplayState there's also a setVisibility call, there are some setVisibility calls that are alone by themselves.

> 
> ::: browser/modules/PluginContent.jsm:947
> (Diff revision 3)
> >        if (!overlay) {
> >          continue;
> >        }
> >        let overlayDisplayState = this.computeOverlayDisplayState(plugin, overlay);
> >        this.setVisibility(plugin, overlay, overlayDisplayState);
> > -      if (overlayDisplayState == OVERLAY_DISPLAY_VISIBLE) {
> > +      if (overlayDisplayState > OVERLAY_DISPLAY.BLANK) {
> 
> This doesn't break any tests in browser/base/content/test/plugins?

This doesn't, but there was some other breakage that I spent some time figuring out. What happened is that, previously, "hidden" didn't mean actually hidden, it meant just the contents of the overlay hidden (that is, just the grey box).

With your patch, this changed, and that became "minimal" (and "hidden" is now _really_ hidden). However, at the end of computeOverlayDisplayState, it was still returning HIDDEN, where I think it should have been MINIMAL (and HIDDEN is only for the QUIET state). This was already fixed in the patch as reviewed, but some tests needed adjustment for that.

Now I changed the name MINIMAL to BLANK, and adjusted some tests that were relying in the incorrect behavior. They now check for sizing=blank instead of checking for the .hidden class.

I pushed to try one last time to make sure nothing else needs adjustment.
(Assignee)

Updated

15 days ago
Depends on: 1415978
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 60

15 days ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/59532da5ffff
Remove the activation warning on the flash click-to-play overlay. r=johannh
https://hg.mozilla.org/integration/autoland/rev/9159635ae827
Drop the old contentPluginClose.png icon and replace it with the modern svg version. r=johannh
https://hg.mozilla.org/integration/autoland/rev/86d7f89e9cfb
Drop the old contentPluginActivate.png icon and replace it with the modern svg version. r=johannh
https://hg.mozilla.org/integration/autoland/rev/331d0da78f3e
Improve styling of the plugin Click-to-Play overlay. r=johannh
https://hg.mozilla.org/integration/autoland/rev/bd2913f16345
Add dynamic sizing on the plugin overlay. r=dthayer,johannh
https://hg.mozilla.org/integration/autoland/rev/86e066b54fc3
Add test to ensure that the plugin overlay is displayed in the correct style. r=johannh
Backed out for ESlint failure at browser/base/content/test/plugins/browser_CTP_overlay_styles.js:73:

https://hg.mozilla.org/integration/autoland/rev/a91a0cde388fd500bf49ff37ba9f152f6e7cced4

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=86e066b54fc308c358f2dc2954662a74dc36eff5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=143423414&repo=autoland
Flags: needinfo?(felipc)
Comment hidden (mozreview-request)
(Assignee)

Updated

15 days ago
Flags: needinfo?(felipc)

Comment 63

15 days ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0da5d54d003c
Remove the activation warning on the flash click-to-play overlay. r=johannh
https://hg.mozilla.org/integration/autoland/rev/4d486418cd55
Drop the old contentPluginClose.png icon and replace it with the modern svg version. r=johannh
https://hg.mozilla.org/integration/autoland/rev/acfd8cf8c4ec
Drop the old contentPluginActivate.png icon and replace it with the modern svg version. r=johannh
https://hg.mozilla.org/integration/autoland/rev/78eebd668bf1
Improve styling of the plugin Click-to-Play overlay. r=johannh
https://hg.mozilla.org/integration/autoland/rev/0b032b6e0655
Add dynamic sizing on the plugin overlay. r=dthayer,johannh
https://hg.mozilla.org/integration/autoland/rev/6fe2a24e51ca
Add test to ensure that the plugin overlay is displayed in the correct style. r=johannh
Backed out for failing mochitest caps/tests/mochitest/test_bug292789.html on Android:

https://hg.mozilla.org/integration/autoland/rev/ed12aa2eac651f3a9c874a96e26471c4966e865a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6fe2a24e51cad359aa5d42f8792b434f61a38bb6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=143467311&repo=autoland

[task 2017-11-09T23:03:47.948Z] 23:03:47     INFO -  5 INFO TEST-START | caps/tests/mochitest/test_bug292789.html
[task 2017-11-09T23:03:58.139Z] 23:03:58     INFO -  Buffered messages logged at 23:03:46
[task 2017-11-09T23:03:58.139Z] 23:03:58     INFO -  6 INFO TEST-PASS | caps/tests/mochitest/test_bug292789.html | content can still load <script> from chrome://global
[task 2017-11-09T23:03:58.139Z] 23:03:58     INFO -  7 INFO TEST-PASS | caps/tests/mochitest/test_bug292789.html | content should not be able to load <script> from chrome://mozapps
[task 2017-11-09T23:03:58.143Z] 23:03:58     INFO -  8 INFO TEST-PASS | caps/tests/mochitest/test_bug292789.html | xpinstallConfirm.js has not moved unexpectedly
[task 2017-11-09T23:03:58.143Z] 23:03:58     INFO -  Buffered messages logged at 23:03:47
[task 2017-11-09T23:03:58.144Z] 23:03:58     INFO -  9 INFO TEST-PASS | caps/tests/mochitest/test_bug292789.html | content should be able to load chrome://global/skin/media/error.png
[task 2017-11-09T23:03:58.144Z] 23:03:58     INFO -  10 INFO TEST-PASS | caps/tests/mochitest/test_bug292789.html | content should not be allowed to load chrome://mozapps/skin/profile/profileicon.png
[task 2017-11-09T23:03:58.144Z] 23:03:58     INFO -  Buffered messages finished
[task 2017-11-09T23:03:58.144Z] 23:03:58     INFO -  11 INFO TEST-UNEXPECTED-FAIL | caps/tests/mochitest/test_bug292789.html | content should not be allowed to load resource://gre/chrome/toolkit/skin/classic/mozapps/profile/profileicon.png - got "fail", expected "success"
[task 2017-11-09T23:03:58.144Z] 23:03:58     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-11-09T23:03:58.144Z] 23:03:58     INFO -      fail@caps/tests/mochitest/test_bug292789.html:79:5
Flags: needinfo?(felipc)
(Assignee)

Comment 65

14 days ago
What happened is that I had removed an image file that was used by this test. I did notice that and replaced that image by another one from the tree. However, the one that I had replaced was not packaged on Android, only on Desktop.. Thus, this test failed on Android..
Flags: needinfo?(felipc)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 71

14 days ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/28d0928fdfa5
Remove the activation warning on the flash click-to-play overlay. r=johannh
https://hg.mozilla.org/integration/autoland/rev/b2be5cae24f4
Drop the old contentPluginClose.png icon and replace it with the modern svg version. r=johannh
https://hg.mozilla.org/integration/autoland/rev/c21d184abbb0
Drop the old contentPluginActivate.png icon and replace it with the modern svg version. r=johannh
https://hg.mozilla.org/integration/autoland/rev/b150d1cc8dff
Improve styling of the plugin Click-to-Play overlay. r=johannh
https://hg.mozilla.org/integration/autoland/rev/8c35f0e7de97
Add dynamic sizing on the plugin overlay. r=dthayer,johannh
https://hg.mozilla.org/integration/autoland/rev/1c1fd21f5280
Add test to ensure that the plugin overlay is displayed in the correct style. r=johannh
https://hg.mozilla.org/mozilla-central/rev/28d0928fdfa5
https://hg.mozilla.org/mozilla-central/rev/b2be5cae24f4
https://hg.mozilla.org/mozilla-central/rev/c21d184abbb0
https://hg.mozilla.org/mozilla-central/rev/b150d1cc8dff
https://hg.mozilla.org/mozilla-central/rev/8c35f0e7de97
https://hg.mozilla.org/mozilla-central/rev/1c1fd21f5280
Status: ASSIGNED → RESOLVED
Last Resolved: 14 days ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I verified this on Mac OS X 10.12 and Windows 10 with FF Nightly 58.0a1(2017-11-12) following this steps:


When the size is >240x160, you see:
Expected results:

• Icon (48x48)
• Text
• Quit button

Actual results: 

• Icon (42x42)
• Text
• Quit button



When the size is >120x80, you see:

Expected results:

• Icon (48x48)
• Quit button

Actual results:

• Icon (42x42)
• Quit button

When the size is >80x60, you see:

Expected results:
• Icon (32x32)

Actual results:
• Icon (28x28)


When the size is >32x32, you see:

Expected results:
• Icon (16x16)

Actual results:
• The icon is not displayed, I see a black box, but if I click on that box the icon is displayed. 


When the size is <32x32, you see:

This is the only scenario where the actual result is the same with the expected one.
• Nothing, just the grey box background

Felipe please tell me your opinion about this, thanks.
Flags: needinfo?(felipc)
(Assignee)

Comment 74

11 days ago
Hi Ovidiu, two questions:

- what page did you use as testcase?

- how are you determining the icon size?
Flags: needinfo?(felipc)
Hi Felipe,

(In reply to :Felipe Gomes (needinfo me!) from comment #74)
> Hi Ovidiu, two questions:
> 
> - what page did you use as testcase?

I used http://felipc.github.io/flashtest/index.html   
> 
> - how are you determining the icon size?

I used xScope2 on Mac and on Windows I used MB-Ruler.

Comment 76

11 days ago
Hi Ovidiu, thanks for your very careful measurements! You’re correct that the icon dimension didn’t match what you’ve measured on xScope.

I should’ve been more clear in my specs. I was referring to the whole icon dimension, which includes empty space boundaries on all sides, and not just the dimension of the filled-in icon.

So if you were to set the SVG to be 48x48, the actual icon inside the surrounding empty space would be 42x42, and so on.

I hope that clarifies it. Your measurement was correct, but my spec measures the total SVG dimension, which includes both the icon itself + the empty space around it.

In other words, I think we’re set and ready to go!
Thanks Bram for your clarifications, based on your comment 76 and my results from comment 73 I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.