774315 - Need the command to hide placeholder for click to play

Status: RESOLVED FIXED

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

Description

[marvin]

Attachment: Screenshot showing the problem.

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:15.0) Gecko/20120714 Firefox/15.0a2
Build ID: 20120714042007

Steps to reproduce:

I went to http://sat1.de. On there homepage a large flash based ad overlays the content. Please see attached screenshot.


Actual results:

Because I set click_to_play to true, flash is not loaded, and it is replaced with a gray "click to activate flash" box. This makes the page useless unless I activate flash and then dismiss the ad.


Expected results:

When I right click the box, I should have been given the option to hide it.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

This problem occurs on Google Chrome's click-to-play and Opera's one.

But Chrome's one can hide the placeholder which click-to-play added via context-menu or close button on right-up corner.

I agree that we should implement the command to hide placeholder.

Comment 2

[Ian Melven :imelven]

Makes sense to me, thanks for filing the issue !

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Jared - what do you think of this approach?

Comment 5

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

Comment on attachment 643133 [details] [diff] [review]
patch

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

Should we provide a context menu item to hide the overlay? I don't think this close button will be accessible for keyboard navigation.

::: browser/base/content/browser-plugins.js
@@ +286,5 @@
>            gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);
>        }, true);
> +
> +      let topIcons = doc.getAnonymousElementByAttribute(aPlugin, "class", "msg msgTopIcons");
> +      topIcons.style.display = "block";

Why does the div need to have |display:block;| set on it? It seems to me that it would have |display:block;| by default.

It's unfortunate that pairing getAnonymousElementByAttribute and the class attribute creates an awkward situation where adding another class name will break the functioning of this script in a non-obvious way. I would rather that (if this is even needed) you set an anonid attribute of "msgTopIcons" and then use getAnonymousElementByAttribute(aPlugin, "anonid", "msgTopIcons").

@@ +288,5 @@
> +
> +      let topIcons = doc.getAnonymousElementByAttribute(aPlugin, "class", "msg msgTopIcons");
> +      topIcons.style.display = "block";
> +      let closeIcon = doc.getAnonymousElementByAttribute(aPlugin, "class", "closeIcon");
> +      closeIcon.addEventListener("click", function(aEvent) { overlay.style.visibility = "hidden"; }, true);

This event listener isn't removed, and pulling the |overlay| in to the closure has the possibilities of a memory leak (I'm not 100% sure here). I'm not sure at what time you'd want to remove the event listener though.

I think an alternative would be if you got to the overlay by doing |aEvent.target.parentElement.parentElement| (although that's a bit uglier).

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +97,5 @@
> +  position: relative;
> +}
> +
> +.closeIcon {
> +  display: inline-block;

Can we use an html:button here or a different element that is more semantic and also doesn't require us to change the display?

@@ +100,5 @@
> +.closeIcon {
> +  display: inline-block;
> +  position: absolute;
> +  top: 0px;
> +  right: 0px;

This will place the close button on the start (right) side in RTL mode, which I don't think we'd want to do. I think you can get similar look if you use a display:block element with -moz-margin-start:0 and -moz-margin-end:auto;.

Looking at the CSS here, it looks like the close button will be directly touching the edge of the box. Can we give it a few pixels of padding between the button and the edge of the box?

nit: omit the unit for values of 0.

@@ +102,5 @@
> +  position: absolute;
> +  top: 0px;
> +  right: 0px;
> +  min-width: 16px;
> +  min-height: 16px;

Can we use width/height here instead of min-*? I don't foresee scenarios where we'd want to expand the button to be larger than 16px.

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jared Wein [:jaws] from comment #5)
> Comment on attachment 643133 [details] [diff] [review]
> patch
> 
> Review of attachment 643133 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +288,5 @@
> > +
> > +      let topIcons = doc.getAnonymousElementByAttribute(aPlugin, "class", "msg msgTopIcons");
> > +      topIcons.style.display = "block";
> > +      let closeIcon = doc.getAnonymousElementByAttribute(aPlugin, "class", "closeIcon");
> > +      closeIcon.addEventListener("click", function(aEvent) { overlay.style.visibility = "hidden"; }, true);
> 
> 
> I think an alternative would be if you got to the overlay by doing
> |aEvent.target.parentElement.parentElement| (although that's a bit uglier).
> 

I think, David should be use |aEvent.currentTarget| instead of |aEvent.target.parentElement.parentElement|.

Comment 7

[Georg Fritzsche [:gfritzsche]]

David, do you think you will find the time to finish this?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I'm a bit busy this week, but there isn't a lot of work necessary to finish this anyway. Do you just need a patch that works for the study, or are you wanting this to actually ship?

Comment 9

[Benjamin Smedberg]

For the study is sufficient.

Comment 10

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

I'll finish this bug.

Comment 11

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

Attachment: Patch v1.1

This patch should address the previous review comments, and adds context context menu items for activating and hiding the plugin.

Comment 12

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

Attachment: Patch v1.2

Forgot to update pinstripe.

Comment 13

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

Attachment: Patch v1.3

Fixed a minor bug where hiding through the context menu was hiding the plugin element and not just the overlay.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 703560 [details] [diff] [review]
Patch v1.3

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

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ +112,5 @@
>    display: block;
>    visibility: hidden;
>  }
>  
> +.closeIcon,

I think for consistency with the tabs' close buttons, we shouldn't make this cursor: pointer

::: toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css
@@ +93,5 @@
>    min-height: 16px;
>    background: url(chrome://mozapps/skin/plugins/pluginHelp-16.png) no-repeat;
>  }
> +
> +.closeIcon {

you should also cover hi-dpi here. Easiest will be to only set the background-image on .closeIcon and adjust background-position for :hover and :active

Comment 15

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

Thanks for the speedy review. Addressed your feedback and was also able to remove the duplicate code for hiding the plugin overlay.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e082e9b65e53
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa8e691ed6bc

Comment 16

[Frank Yan (:fryn)]

Followup to fix the followup in HiDPI mode.
(No worries; that HiDPI workaround I came up with is tricky.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0238a6bf8f85

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/e082e9b65e53
https://hg.mozilla.org/mozilla-central/rev/fa8e691ed6bc
https://hg.mozilla.org/mozilla-central/rev/0238a6bf8f85

Comment 18

[Paul Silaghi, QA [:pauly]]

Couple of issues:
1. Right click options (activate/hide this plugins) are N/A for outdated plugins, but the X button is
2. The X button is so dark, I didn't even notice it the first time.
Nightly 21.0a1 (2013-02-07) Win 7

Comment 19

[Paul Silaghi, QA [:pauly]]

I noticed that if you hover the cursor over the X button, the color changes on the main Activate button. Are you ok with that?

Comment 20

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

(In reply to Paul Silaghi [QA] from comment #19)
> I noticed that if you hover the cursor over the X button, the color changes
> on the main Activate button. Are you ok with that?

Yes, that is by design. The color of the Activate button changes because it is no longer the action that will be started when the user clicks.