Elbart Reporter
	Description • 11 years ago

Firefox 23, 24, Nightly 2013-09-27 on XP, Vista and 7
Adobe Flash 11.8.800.168 set to "Never activate"

---

When trying to close a "This plugin is disabled"-field, the placeholder cannot be closed in at least Windows XP, Vista and 7. Check the URL for a site with a Flash-element.

While this is a minor problem for standalone-elements, it becomes a UX-problem when such an obstacle is hiding the menu of a website.

For example http://wiesenmarkt.at/news.php
Move the cursor over the row with "Home", "Wiesn Infos", "Aktuell" etc.
Because the placeholder cannot be closed, the menu becomes unusable.

Please let 24ESR also get the fix and, please test if the other placeholders, like click-to-play, are affected too.

	B.J. Herbison
	Comment 1 • 11 years ago

I see the same issue on Linux with the 27 September Nightly. Using the browser console I see the menu items switch from hidden to visible on mouse-over, but the items are behind the "plugin blocked" message.

The same issue exists for "Never Activate" and "Ask to Activate".

	Georg Fritzsche [:gfritzsche]
	Comment 2 • 11 years ago

I can confirm that the "plugin disabled" overlay can't be closed on Fx 24 and Nightly.

However, the click-to-play overlay ("ask to activate") is fine. B.J., with the menu effects this sounds like the sites setup prevents you from clicking it. Does it work on [1] for you?

[1] http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html

	Elbart Reporter
	Comment 3 • 11 years ago

With Nightly of 2013-10-01:
"ask to activate": closing with the X works; closing with the link works
"never activate": closing with the X does NOT work; closing with the link works.

---

I think B.J. Herbison means that the mouseover-menu-items are behind the plugin-placeholder for both "never activate" and "ask to activate". IIRC I also had the problem when the placeholder was showing that the Flash-plugin was outdated.

Another example of this is http://www.toolgal.com/ by going over "Products".

Maybe this should be filed as another bug?

	Elbart Reporter
	Comment 4 • 11 years ago

Another placeholder I cannot close is "A plugin is needed to display this content.", tested on http://benjamin.smedbergs.us/tests/ctptests/java-solo.html with no Java installed.

But I can close the placeholder with "Activate XYZ. This plugin is vulnerable and should be updated.".

	Georg Fritzsche [:gfritzsche]
	Comment 5 • 11 years ago

Apparently we currently only attach an event handler to the close-button for click-to-play/blocked/vulnerable plugins [1]; maybe we just need to move that handler into handleEvent() [2].


(In reply to elbart from comment #3)
> Another example of this is http://www.toolgal.com/ by going over "Products".
> 
> Maybe this should be filed as another bug?

No, we force the plugin overlay to be always on top as it was blocked/hidden in other use-cases.
We may emulate the z-index behavior of Flash specifically more closely in later releases though (can't find the bug right now).
[1] http://hg.mozilla.org/mozilla-central/annotate/e3c84e9f2490/browser/base/content/browser-plugins.js#l537
[2] http://hg.mozilla.org/mozilla-central/annotate/e3c84e9f2490/browser/base/content/browser-plugins.js#l215

	Benjamin Smedberg
	Comment 6 • 11 years ago

> Apparently we currently only attach an event handler to the close-button for
> click-to-play/blocked/vulnerable plugins [1]; maybe we just need to move
> that handler into handleEvent() [2].

Yes, this sounds correct. I don't think we need the custom handler at all.

	Georg Fritzsche [:gfritzsche]
	Comment 7 • 11 years ago

Simple steps to reproduce:
* go to Tools->Addons
* set Flash to disabled
* navigate to [1]
* click on the (x)
=> it should hide the "plugin disabled" overlay, but doesn't

	Georg Fritzsche [:gfritzsche]
	Comment 8 • 11 years ago

(With [1] being http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html)

	B.J. Herbison
	Comment 9 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> However, the click-to-play overlay ("ask to activate") is fine. B.J., with
> the menu effects this sounds like the sites setup prevents you from clicking
> it. Does it work on [1] for you?
> 
> [1] http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html

elbart was correct. Flash works correctly, but I can't use the menus on the original page without activating Flash.

	Sahil Chelaramani
	Comment 10 • 11 years ago

Made changes as suggested in above comments

	Georg Fritzsche [:gfritzsche]
	Comment 11 • 11 years ago

Comment on attachment 814356 [details] [diff] [review]
Moved event handler to handleEvent

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

Thanks, that looks close! Besides the issues mentioned, did you check it is working for some variations? In particular:
* Does the close button work when the plugin is set to disabled or "ask to activate" in Tools->Addons->Plugins?
* Does it also work for crashed plugins? To test that you can kill the plugin container process (e.g. plugin-container.exe on Windows).

::: browser/base/content/browser-plugins.js
@@ +331,5 @@
>            }
>          });
>        }
>      }
> +    if (overlay) {

Minor nitpick: I think we should have an empty line before the if block.

@@ +336,5 @@
> +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);
> +      let closeIcon = this.getPluginUI(aPlugin, "closeIcon");
> +      closeIcon.addEventListener("click", function(aEvent) {
> +        if (aEvent.button == 0 && aEvent.isTrusted)
> +          gPluginHandler.hideClickToPlayOverlay(aPlugin);

I think "aPlugin" needs to be "plugin" when you move the code here.

@@ +343,1 @@
>      // Only show the notification after we've done the isTooSmall check, so

Same here, i think we should have an empty line after the if-block closing brace.

	Sahil Chelaramani
	Comment 12 • 11 years ago

I made the above mentioned changes. No, I haven't. Will try them out and let you know here.

	Georg Fritzsche [:gfritzsche]
	Comment 13 • 11 years ago

That sounds good. If you need any help getting a build working or your changes in it, you can always ask in #introduction [1] or mail me.

[1] https://developer.mozilla.org/en-US/docs/Introduction

	Georg Fritzsche [:gfritzsche]
	Comment 14 • 11 years ago

Comment on attachment 814446 [details] [diff] [review]
Made changes

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

Great, let me know when you had a chance to test.

::: browser/base/content/browser-plugins.js
@@ +336,5 @@
> +    if (overlay) {
> +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);
> +      let closeIcon = this.getPluginUI(plugin, "closeIcon");
> +      closeIcon.addEventListener("click", function(aEvent) {
> +        if (aEvent.button == 0 && aEvent.isTrusted)

I think you have a similar problem as with "plugin" on the two lines here - "aEvent" should be "event".

	Sahil Chelaramani
	Comment 15 • 11 years ago

I added a line to initialize the overlay variable.Also, I made the changes from aEvent to event(I think it was a formal parameter in the anonymous function) This works for all the test cases mentioned above.

	Elbart Reporter
	Comment 16 • 11 years ago

Will ESR get the patch?

	Georg Fritzsche [:gfritzsche]
	Comment 17 • 11 years ago

Comment on attachment 818947 [details] [diff] [review]
made change from aEvent to event

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

Cool, this looks good now, behaves right for me locally too. There are only whitespace nitpicks now, if you check out the "splinter review" for your patch you will probably see better what i mean.
I pushed this to our try server so we can see if this affects any tests:
https://tbpl.mozilla.org/?tree=Try&rev=a8ae29dcde76

After fixing the whitespace, please request review from jaws (Jared Wein), as i'm not a peer for the browser code.

::: browser/base/content/browser-plugins.js
@@ +332,5 @@
>          });
>        }
>      }
>  
> +	let overlay = this.getPluginUI(plugin, "main");

I think this is indented with a tab instead of spaces - we only use spaces for indentation.

@@ +333,5 @@
>        }
>      }
>  
> +	let overlay = this.getPluginUI(plugin, "main");
> +    

There are left-over spaces at the start of this line, please remove them.

	Georg Fritzsche [:gfritzsche]
	Comment 18 • 11 years ago

(In reply to elbart from comment #16)
> Will ESR get the patch?

The patch missed the normal train for ESR24, so we'll have to ask release management if they are willing to take this after it was verified in Firefox Nightly (not sure as it's not a security or other high priority fix).

	Sahil Chelaramani
	Comment 19 • 11 years ago

Minor changes as asked. Thanks a lot for your help gfritzsche :)

	Jared Wein [:jaws] (please needinfo? me)
	Comment 20 • 11 years ago

Comment on attachment 819073 [details] [diff] [review]
Minor changes

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

Thanks for the patch, just a couple things to address.

::: browser/base/content/browser-plugins.js
@@ +332,5 @@
>          });
>        }
>      }
>  
> +    let overlay = this.getPluginUI(plugin, "main");

There is already a variable named |overlay| declared about 10 lines above that can be moved and reused.

@@ +334,5 @@
>      }
>  
> +    let overlay = this.getPluginUI(plugin, "main");
> +    if (overlay) {
> +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);

Adding event listeners here seems quite indeterminate (they should at least be moved to one of the blocks within the event switch).

	Georg Fritzsche [:gfritzsche]
	Comment 21 • 11 years ago

(In reply to Jared Wein [:jaws] from comment #20)
> @@ +334,5 @@
> >      }
> >  
> > +    let overlay = this.getPluginUI(plugin, "main");
> > +    if (overlay) {
> > +      overlay.addEventListener("click", gPluginHandler._overlayClickListener, true);
> 
> Adding event listeners here seems quite indeterminate (they should at least
> be moved to one of the blocks within the event switch).

Right, this triggered the failures in the try run [1] as well. You can test this locally by running e.g. single tests:
./mach mochitest-browser browser/base/content/test/general/browser_CTP_drag_drop.js
./mach mochitest-browser browser/base/content/test/general/browser_pluginnotification.js

... or all the general browser tests (takes a bit though):
./mach mochitest-browser browser/base/content/test/general/

The line adding the _overlayClickListener should probably just stay where the code originally was in PH_handleClickToPlayEvent().

[1] https://tbpl.mozilla.org/?tree=Try&rev=a8ae29dcde76

	Elbart Reporter
	Comment 22 • 11 years ago

I assume this won't make its way into 26, right?

	Georg Fritzsche [:gfritzsche]
	Comment 23 • 11 years ago

(In reply to Elbart from comment #22)
> I assume this won't make its way into 26, right?

No - even when it was finished it is not critical enough to warrant putting it into a Beta.

	Anup Kumar [:reznord] Assignee
	Comment 24 • 11 years ago

Hi,

I am interested in working on this bug. Can you please assign it to me?

Thanks in advance.

	Georg Fritzsche [:gfritzsche]
	Comment 25 • 11 years ago

Sure, done :)

	Anup Kumar [:reznord] Assignee
	Comment 26 • 11 years ago

working for almost all the tests.

	Georg Fritzsche [:gfritzsche]
	Comment 27 • 11 years ago

Could you expand on which test failed with which error?

	Anup Kumar [:reznord] Assignee
	Comment 28 • 11 years ago

> ... or all the general browser tests (takes a bit though):
> ./mach mochitest-browser browser/base/content/test/general/

When I ran the following test in my browser I got the following error saying that

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_CTP_crashreporting.js | Submission UI visibility should be correct - Got none, expected block

	Anup Kumar [:reznord] Assignee
	Comment 29 • 11 years ago

> > ... or all the general browser tests (takes a bit though):
> > ./mach mochitest-browser browser/base/content/test/general/
> 
> When I ran the following test in my browser I got the following error saying
> that
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_CTP_crashreporting.js | Submission UI visibility should be correct -
> Got none, expected block

In addition to this error I am getting the following error also:

 5:31.55 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_CTP_crashreporting.js | Test timed out
 5:31.55 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_popupNotification.js | Test timed out

	Georg Fritzsche [:gfritzsche]
	Comment 30 • 11 years ago

Comment on attachment 8361111 [details] [diff] [review]
bug-921730.patch

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

This looks good so far. Let's fix the suspected timing error in the test that we talked about, then this should be good for a review!

::: browser/base/content/browser-plugins.js
@@ +379,5 @@
>          plugin.addEventListener("overflow", resizeListener);
>          plugin.addEventListener("underflow", resizeListener);
>        }
>      }
> +    

You have some trailing spaces (whitespace) on this and other lines.
If you click the "review" link for your patch you will see those spaces highlighted in red.

	Anup Kumar [:reznord] Assignee
	Comment 31 • 11 years ago

removed the trailing white spaces.

	Anup Kumar [:reznord] Assignee
	Comment 32 • 11 years ago

Changes made

	Anup Kumar [:reznord] Assignee
	Comment 33 • 11 years ago

white spaces fixed

	Anup Kumar [:reznord] Assignee
	Comment 34 • 11 years ago

White space fixed

	Anup Kumar [:reznord] Assignee
	Comment 35 • 11 years ago

white spaces removed

	Georg Fritzsche [:gfritzsche]
	Comment 36 • 11 years ago

Comment on attachment 8365525 [details] [diff] [review]
new.patch

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

Looks good and works fine for me now!
One small issue left that i missed before.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ee75433d00eb

::: browser/base/content/browser-plugins.js
@@ +381,5 @@
>        }
>      }
>  
> +    let closeIcon = this.getPluginUI(plugin, "closeIcon");
> +    if (closeIcon != null) {

This should be: if (closeIcon)

	Georg Fritzsche [:gfritzsche]
	Comment 37 • 11 years ago

The above try run was from a broken repository, pushed again: 
https://tbpl.mozilla.org/?tree=Try&rev=8662f4d53053

	Jared Wein [:jaws] (please needinfo? me)
	Comment 38 • 11 years ago

Comment on attachment 8365525 [details] [diff] [review]
new.patch

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

Anup, can you add a test for this to make sure that it doesn't break again? Also please make the change that Georg requested.

	Anup Kumar [:reznord] Assignee
	Comment 39 • 11 years ago

> Anup, can you add a test for this to make sure that it doesn't break again?
> Also please make the change that Georg requested.

I tested it before I have submitted the patch and also made the changes made which all are required i.e removed the white spaces and the trailing spaces.

	Georg Fritzsche [:gfritzsche]
	Comment 40 • 11 years ago

Talked with Anup about what we need tests for, we're looking into it.

	Anup Kumar [:reznord] Assignee
	Comment 41 • 11 years ago

Included the test for that also.

	Georg Fritzsche [:gfritzsche]
	Comment 42 • 11 years ago

Comment on attachment 8377615 [details] [diff] [review]
bug-921730.patch

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

This looks good to me, i only noticed minor issues now.

::: browser/base/content/browser-plugins.js
@@ +380,5 @@
>          plugin.addEventListener("underflow", resizeListener);
>        }
>      }
>  
> +	let closeIcon = this.getPluginUI(plugin, "closeIcon");

This is indented a bit too much.

::: browser/base/content/test/general/browser.ini
@@ +101,5 @@
>    test_no_mcb_on_http_site_font2.html
>    test_no_mcb_on_http_site_font2.css
>    xul_tooltiptext.xhtml
>  
> +[browser_CTP_hide_overlay.js]

I think we want to have those entries alphabetically sorted, so this should probably go after [browser_CTP_drag_drop.js].

::: browser/base/content/test/general/browser_CTP_hide_overlay.js
@@ +60,5 @@
> +    elems[0].clientTop;
> +    executeSoon(func);
> +  };
> +}
> +

Looking at it now, i think it would be good to put a comment here to describe what the test is for.

	Anup Kumar [:reznord] Assignee
	Comment 43 • 11 years ago

Added the comment for the test and removed the trailing white spaces.

	Georg Fritzsche [:gfritzsche]
	Comment 44 • 11 years ago

Comment on attachment 8378126 [details] [diff] [review]
bug_921730.patch

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

::: browser/base/content/test/general/browser_CTP_hide_overlay.js
@@ +21,5 @@
> +  gTestBrowser = gBrowser.selectedBrowser;
> +  gTestBrowser.addEventListener("load", pageLoad, true);
> +
> +  Services.prefs.setBoolPref("plugins.click_to_play", true);
> +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY);

I completely missed this before: This should set to STATE_DISABLED, not STATE_CLICKTOPLAY.

@@ +61,5 @@
> +    executeSoon(func);
> +  };
> +}
> +
> +// Test that checks the overlay visibility of the closeIcon(x) when the user is prompted to activate the flash plugin. 

How about something like:
Test that the overlay can be hidden for disabled plugins using the close icon.

	Anup Kumar [:reznord] Assignee
	Comment 45 • 11 years ago

Changes made according to the comment 44. 

Sorry for that, I forgot about that before.

	Georg Fritzsche [:gfritzsche]
	Comment 46 • 11 years ago

Comment on attachment 8378219 [details] [diff] [review]
bug_921730.patch

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

This looks good to me now, thanks!
Only a minor whitespace issue that can be fixed after jaws reviewed.

::: browser/base/content/test/general/browser_CTP_hide_overlay.js
@@ +61,5 @@
> +    executeSoon(func);
> +  };
> +}
> +
> +// Tests that the overlay can be hidded for disabled plugins using the close icon. 

There is a trailing space here.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 47 • 11 years ago

Comment on attachment 8378219 [details] [diff] [review]
bug_921730.patch

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

With this patch applied, I am still unable to close the plugin placeholder on the website linked to in comment #0.

Instead, I actually see no plugin placeholder overlay content (just the a blank grey box). This may be already present as people who haven't applied this patch have encountered this too. See http://screencast.com/t/Om5TyCDcS for a screenshot of what I'm seeing.

	Georg Fritzsche [:gfritzsche]
	Comment 48 • 11 years ago

Hm, for me the overlay is showing & closing fine with the patch applied for both disabled and click-to-play Flash.
I tried:
http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html
http://wiesenmarkt.at/news.php
http://ce.uwex.edu/techsupport/flashtest.aspx

	Georg Fritzsche [:gfritzsche]
	Comment 49 • 11 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) [PTO 2/20-2/23] from comment #47)
> See http://screencast.com/t/Om5TyCDcS for a screenshot of what I'm seeing.

Hm, looking at this again i'm pretty sure we have some other issue:
The notification bar is visible & the overlay hidden, so it looks like the browser-plugins.js code decided that the plugin was not visible for some reason.

	Elbart Reporter
	Comment 50 • 11 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) [PTO 2/20-2/23] from comment #47)
> See http://screencast.com/t/Om5TyCDcS
> for a screenshot of what I'm seeing.

Was the embed not fully visible upon loading the website?
If yes, that's probably covered by bug 968762.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 51 • 11 years ago

Comment on attachment 8378219 [details] [diff] [review]
bug_921730.patch

Redoing the review since the issue I have is not related to this patch.

	Georg Fritzsche [:gfritzsche]
	Comment 52 • 11 years ago

Anup, can you fix the whitespace and change the commit message to use r=jaws (as jaws did the actual review, not me)?
Then this should be good to go.

	Anup Kumar [:reznord] Assignee
	Comment 53 • 11 years ago

Removed the white spaces and changed the reviewer as requested in the above comment.

	Georg Fritzsche [:gfritzsche]
	Comment 54 • 11 years ago

Comment on attachment 8381388 [details] [diff] [review]
bug_921730.patch

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

Thanks, this looks good. This was already reviewed and only addresses previous comments, so it's good to go.

	Ryan VanderMeulen [:RyanVM]
	Comment 55 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2965b5214b

	Elbart Reporter
	Comment 56 • 11 years ago

Testing the URL of comment #0 with https://hg.mozilla.org/integration/mozilla-inbound/rev/395177ab859b and Flash being either set to "ask" or disabled or not installed, clicking the X hides the the placeholder-text and the X-button, but not the gray area itself.

Is this intended behavior? 

Because this results in the javascript-menu remaining unreachable. See the arrow pointing at the bottom border of the menu being hidden by the placeholder.

	Georg Fritzsche [:gfritzsche]
	Comment 57 • 11 years ago

(In reply to Elbart from comment #56)
> Testing the URL of comment #0 with
> https://hg.mozilla.org/integration/mozilla-inbound/rev/395177ab859b and
> Flash being either set to "ask" or disabled or not installed, clicking the X
> hides the the placeholder-text and the X-button, but not the gray area
> itself.
> 
> Is this intended behavior?

Yes, the actual close behaviour didn't change - it just hides the overlay but not the actual plugin element.
Can you file a new bug about that issue?

	Elbart Reporter
	Comment 58 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #57)
> Yes, the actual close behaviour didn't change - it just hides the overlay
> but not the actual plugin element.
> Can you file a new bug about that issue?

Done: bug 976769

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 59 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/3c2965b5214b

	Paul Silaghi, QA [:pauly]
	Comment 60 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #48)
> Hm, for me the overlay is showing & closing fine with the patch applied for
> both disabled and click-to-play Flash.
Confirmed. Verified fixed in nightly 30.0a1(2014-02-26) Win 7, Mac OS X 10.8.5, Ubuntu 13.04.
Further work continues in bug 976769.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 61 • 10 years ago

I'm marking this as won't fix for ESR24 based on the low severity.