The default bug view has changed. See this FAQ.

Need the command to hide placeholder for click to play

RESOLVED FIXED in mozilla21

Status

()

Core
Plug-ins
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: marvin, Assigned: jaws)

Tracking

(Depends on: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 642606 [details]
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.
(Reporter)

Updated

5 years ago
Blocks: 711552
Summary: When click to play is activated, large popup ads become even more annoying. Because they can't be dismissed without activating the plugin. → When click to play is activated, large popup ads can't be dismissed without activating the plugin. Please add option to hide gray box.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Untriaged → Plug-ins
Product: Firefox → Core
Summary: When click to play is activated, large popup ads can't be dismissed without activating the plugin. Please add option to hide gray box. → Need the command to hide placeholder for click to play
Version: 15 Branch → Trunk

Comment 2

5 years ago
Makes sense to me, thanks for filing the issue !
Created attachment 643133 [details] [diff] [review]
patch

Jared - what do you think of this approach?
Attachment #643133 - Flags: review?(jaws)
Blocks: 738698
No longer blocks: 711552
Duplicate of this bug: 773121
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.
Attachment #643133 - Flags: review?(jaws) → review-
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
OS: Windows 8 → All
Hardware: x86_64 → All
(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|.
David, do you think you will find the time to finish this?
Flags: needinfo?(dkeeler)
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?
Flags: needinfo?(dkeeler)
For the study is sufficient.
I'll finish this bug.
Assignee: dkeeler → jaws
Created attachment 703456 [details] [diff] [review]
Patch v1.1

This patch should address the previous review comments, and adds context context menu items for activating and hiding the plugin.
Attachment #643133 - Attachment is obsolete: true
Attachment #703456 - Flags: review?(dkeeler)
Created attachment 703465 [details] [diff] [review]
Patch v1.2

Forgot to update pinstripe.
Attachment #703456 - Attachment is obsolete: true
Attachment #703456 - Flags: review?(dkeeler)
Attachment #703465 - Flags: review?(dkeeler)
Priority: -- → P2
Blocks: 831921
Attachment #703465 - Flags: review?(felipc)
Created attachment 703560 [details] [diff] [review]
Patch v1.3

Fixed a minor bug where hiding through the context menu was hiding the plugin element and not just the overlay.
Attachment #703465 - Attachment is obsolete: true
Attachment #703465 - Flags: review?(felipc)
Attachment #703465 - Flags: review?(dkeeler)
Attachment #703560 - Flags: review?(felipc)
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
Attachment #703560 - Flags: review?(felipc) → review+
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

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/e082e9b65e53
https://hg.mozilla.org/mozilla-central/rev/fa8e691ed6bc
https://hg.mozilla.org/mozilla-central/rev/0238a6bf8f85
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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
Depends on: 844858
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?
(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.

Updated

4 years ago
Depends on: 867730

Updated

4 years ago
Depends on: 902919
No longer depends on: 902919
You need to log in before you can comment on or make changes to this bug.